-
-
Notifications
You must be signed in to change notification settings - Fork 431
Add bulk change data source profile functionality with automatic RRD rebuild #6495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
feat: Add bulk change data source profile functionality with automatic RRD rebuild - Add new action option "Change Profile" in data source bulk actions menu - Implement profile change with automatic RRD file handling: - Update data_template_data, data_template_rrd, and poller_item tables - Automatically backup and remove RRD files when step value changes - RRD files will be recreated with new parameters at next polling cycle - Add form configuration for profile selection in bulk action dialog - Add logging for RRD file backup operations This feature allows administrators to bulk change data source profiles, automatically handling the RRD file recreation when polling intervals change. Old RRD files are backed up with timestamp before removal for safety. Affects: data_sources.php
|
I think it should go a little further than this, but this is a good start. Should additionally add.
What do you think?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a bulk action feature for changing data source profiles with automatic RRD file handling. When the polling interval (step) changes, old RRD files are automatically backed up with timestamps and removed, allowing Cacti to recreate them with new parameters at the next polling cycle.
Key Changes:
- Added "Change Data Source Profile" bulk action to data source management
- Implemented automatic database updates across data_template_data, data_template_rrd, and poller_item tables
- Added RRD file backup and removal when step values change to enable recreation with new parameters
| if (copy($rrd_path, $backup)) { | ||
| unlink($rrd_path); | ||
| $rrd_changes++; | ||
| cacti_log("RRD backed up and removed: $rrd_path -> $backup", false, 'DATASOURCE'); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for the file copy operation. If copy() fails but doesn't throw an error (e.g., due to disk space or permission issues), the code silently continues without incrementing rrd_changes or logging the failure. The spikekill implementation (lib/spikekill.php:451-457) demonstrates proper error handling by checking the copy result and setting an error if it fails. Consider logging a warning when copy fails so administrators are aware the backup didn't succeed.
| cacti_log("RRD backed up and removed: $rrd_path -> $backup", false, 'DATASOURCE'); | |
| cacti_log("RRD backed up and removed: $rrd_path -> $backup", false, 'DATASOURCE'); | |
| } else { | |
| /* Log a warning so administrators are aware the backup did not succeed */ | |
| cacti_log("WARNING: Failed to backup RRD file '$rrd_path' to '$backup'", false, 'DATASOURCE'); |
|
|
||
| $new_profile = db_fetch_row_prepared('SELECT * FROM data_source_profiles WHERE id = ?', | ||
| array(get_request_var('data_source_profile_id'))); | ||
|
|
||
| if (!empty($new_profile)) { | ||
| $rrd_changes = 0; | ||
|
|
||
| for ($i=0;($i<cacti_count($selected_items));$i++) { | ||
| // Get current step value | ||
| $current_step = db_fetch_cell_prepared('SELECT rrd_step FROM data_template_data WHERE local_data_id = ?', | ||
| array($selected_items[$i])); | ||
|
|
||
| // Update all database tables | ||
| db_execute_prepared('UPDATE data_template_data | ||
| SET data_source_profile_id = ?, rrd_step = ? | ||
| WHERE local_data_id = ?', | ||
| array(get_request_var('data_source_profile_id'), $new_profile['step'], $selected_items[$i]) | ||
| ); | ||
|
|
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation that the selected data_source_profile_id exists and is valid before fetching it from the database. While db_fetch_row_prepared() returns an empty array for non-existent IDs (checked at line 466), it would be more robust to validate that get_request_var('data_source_profile_id') is a positive integer first. The get_filter_request_var() call on line 461 validates it as an integer but doesn't ensure it's positive or exists in the database.
| $new_profile = db_fetch_row_prepared('SELECT * FROM data_source_profiles WHERE id = ?', | |
| array(get_request_var('data_source_profile_id'))); | |
| if (!empty($new_profile)) { | |
| $rrd_changes = 0; | |
| for ($i=0;($i<cacti_count($selected_items));$i++) { | |
| // Get current step value | |
| $current_step = db_fetch_cell_prepared('SELECT rrd_step FROM data_template_data WHERE local_data_id = ?', | |
| array($selected_items[$i])); | |
| // Update all database tables | |
| db_execute_prepared('UPDATE data_template_data | |
| SET data_source_profile_id = ?, rrd_step = ? | |
| WHERE local_data_id = ?', | |
| array(get_request_var('data_source_profile_id'), $new_profile['step'], $selected_items[$i]) | |
| ); | |
| $profile_id = intval(get_request_var('data_source_profile_id')); | |
| if ($profile_id > 0) { | |
| $new_profile = db_fetch_row_prepared('SELECT * FROM data_source_profiles WHERE id = ?', | |
| array($profile_id)); | |
| if (!empty($new_profile)) { | |
| $rrd_changes = 0; | |
| for ($i=0;($i<cacti_count($selected_items));$i++) { | |
| // Get current step value | |
| $current_step = db_fetch_cell_prepared('SELECT rrd_step FROM data_template_data WHERE local_data_id = ?', | |
| array($selected_items[$i])); | |
| // Update all database tables | |
| db_execute_prepared('UPDATE data_template_data | |
| SET data_source_profile_id = ?, rrd_step = ? | |
| WHERE local_data_id = ?', | |
| array($profile_id, $new_profile['step'], $selected_items[$i]) | |
| ); | |
| // Handle RRD file if step changed | ||
| if ($current_step != $new_profile['step']) { | ||
| $rrd_path = get_data_source_path($selected_items[$i], true); | ||
| if (file_exists($rrd_path)) { | ||
| // Backup with timestamp and delete | ||
| $backup = $rrd_path . '.bak_' . date('ymd-His'); | ||
| if (copy($rrd_path, $backup)) { | ||
| unlink($rrd_path); | ||
| $rrd_changes++; | ||
| cacti_log("RRD backed up and removed: $rrd_path -> $backup", false, 'DATASOURCE'); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential race condition: RRD files are deleted while the poller might be actively writing to them. There is no check to ensure the poller isn't currently accessing these files. When the step changes and the RRD is removed, if the poller is in the middle of updating that file, it could cause data corruption or lost poll data. Consider implementing a mechanism to coordinate with the poller (e.g., marking data sources for profile change and having the poller handle the RRD recreation during its next cycle, or temporarily disabling affected data sources).
| array($new_profile['step'], $selected_items[$i]) | ||
| ); | ||
|
|
||
| update_poller_cache($selected_items[$i], true); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update_poller_cache() function is called with the commit parameter set to true for each individual data source in the loop. This causes immediate commits for every iteration. Looking at the utility.php implementation (line 164), update_poller_cache can be called without commit and accumulated, then batch-committed using poller_update_poller_cache_from_buffer() after the loop completes. This pattern is used in repopulate_poller_cache() (lib/utility.php:67) and would significantly improve performance when updating many data sources.
| if ($rrd_changes > 0) { | ||
| $_SESSION['sess_messages']['custom_info'] = array( | ||
| 'message' => sprintf(__('%d RRD files were backed up and will be recreated with new step value at next polling.'), $rrd_changes), | ||
| 'type' => 'info' | ||
| ); | ||
| } |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct modification of $_SESSION is not following established Cacti patterns for user messages. Throughout the codebase, user messages are handled via the raise_message() function. This custom session message approach is inconsistent and may not integrate properly with Cacti's message display system. Consider using raise_message() with a custom message code, or verify that the custom_info session pattern is supported by the message display functions.
|
|
||
| // Handle RRD file if step changed | ||
| if ($current_step != $new_profile['step']) { | ||
| $rrd_path = get_data_source_path($selected_items[$i], true); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_data_source_path() function is called inside a loop for each data source individually. This could be inefficient for bulk operations with many data sources. Consider batching RRD path lookups if possible, or at minimum, only call this function when the step value has changed (move it inside the if condition at line 496).
| 'method' => 'drop_sql', | ||
| 'title' => __('New Data Source Profile'), | ||
| 'default' => '', | ||
| 'sql' => 'SELECT id, name FROM data_source_profiles ORDER BY name' |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query should include a WHERE clause to filter out any inactive or deleted profiles if such a status exists in the data_source_profiles table. Review the data_source_profiles schema to determine if there are status flags that should be considered when presenting profile options to users.
| 'sql' => 'SELECT id, name FROM data_source_profiles ORDER BY name' | |
| 'sql' => 'SELECT id, name FROM data_source_profiles WHERE active="on" ORDER BY name' |
| if (!empty($new_profile)) { | ||
| $rrd_changes = 0; | ||
|
|
||
| for ($i=0;($i<cacti_count($selected_items));$i++) { |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop formatting is inconsistent with the codebase conventions. Throughout this file, loops use the pattern for ($i = 0; ($i < cacti_count($selected_items)); $i++) with spaces around operators, but this line has inconsistent spacing for ($i=0;($i<cacti_count($selected_items));$i++). Please update to match the established pattern seen in lines 444, 448, and 456.
| if ($current_step != $new_profile['step']) { | ||
| $rrd_path = get_data_source_path($selected_items[$i], true); | ||
| if (file_exists($rrd_path)) { | ||
| // Backup with timestamp and delete | ||
| $backup = $rrd_path . '.bak_' . date('ymd-His'); | ||
| if (copy($rrd_path, $backup)) { | ||
| unlink($rrd_path); | ||
| $rrd_changes++; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to unlink($rrd_path) operates on a file path derived from get_data_source_path($selected_items[$i], true), which in turn uses the data_source_path column that is directly editable via the web UI without path restrictions. An attacker with the ability to edit data sources can set data_source_path to an arbitrary filesystem location (e.g. outside the RRD directory) and then trigger a profile change with a different step, causing this bulk action to back up and delete arbitrary files accessible to the web server user. You should enforce that get_data_source_path (or this code path) only ever points under the intended RRD base directory (e.g. by normalizing and validating against CACTI_PATH_RRA) before performing file_exists, copy, or unlink on it, or otherwise harden the data_source_path field to disallow arbitrary filesystem locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheWitness This appears legit to me
|
I'm not fully familiar with Cacti's security baseline, but functionally, I have successfully tested this feature on v1.2.30: Test case 1: Changed a data source's profile from 10s polling to 1-minute polling. After 2 minutes, confirmed that data point intervals changed from 10s to 1 minute. RRD file backup completed successfully. Historical data was lost as expected - preserving it would introduce excessive complexity and risk, which is unnecessary for my use case. Test case 2: Reverted the same data source's profile from 1-minute back to 10s polling. After 20 seconds, confirmed two data points with 10s intervals appearing correctly, with graphs rendering properly. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@terrellgf, can you please update the CHANGELOG.md with the feature and feature number please? Then, I will merge. Thanks! |
feat: Add bulk change data source profile functionality with automatic RRD rebuild
This feature allows administrators to bulk change data source profiles, automatically handling the RRD file recreation when polling intervals change. Old RRD files are backed up with timestamp before removal for safety.
Affects: data_sources.php