Skip to content

issue#133: Automation Networks scheduler, Telnet skip-username, bulk device edit - #197

Merged
TheWitness merged 4 commits into
developfrom
feature/issue-133-schedule-telnet-bulk
Nov 13, 2026
Merged

TheWitness merged 4 commits into
developfrom
feature/issue-133-schedule-telnet-bulk

Conversation

@TheWitness

Copy link
Copy Markdown
Member

Description

This is the first PR in a stacked chain addressing the four open RouterConfigs feature requests (#133 → #117 → #130 → #109). It implements the three remaining parts of #133 (the per-device TFTP server and the add-from-Cacti-Devices import were merged earlier in #183/#185 and #184):

  1. Automation Networks style scheduler — replaces the fixed Daily / Weekly / Monthly (= every 10 days) device schedule with a Schedule Type + Rerun Every model mirroring Cacti core's Automation Networks scheduler (sched_type, recur_every, and the day_of_week / month / day_of_month / monthly_week / monthly_day fields). This delivers both an explicit 10-day cadence (Daily, Rerun Every 10) and a true calendar Monthly backup. Because core's lib/api_scheduler.php ships only on Cacti 1.3+ (it is absent from 1.2.x) and this plugin's compat is 1.2.29, the next-run calendar math is a plugin-local replica (includes/scheduler.php) that delegates to core's api_scheduler_calculate_next_start() when it exists (1.3+) and runs the ported logic otherwise. Existing devices are migrated automatically and idempotently: legacy weekly (7) becomes a true Weekly, every other interval becomes Daily / rerun-every-N, preserving the original cadence (so the old "Monthly" = every 10 days becomes Daily / rerun-every-10).

  2. Telnet skip-username — when a device/account has no username configured, the Telnet login skips the username prompt and waits directly for the password prompt, so password-only Telnet devices can be backed up.

  3. Bulk device edit — a new Edit (bulk change settings) action on the Devices list lets you change Enabled, Schedule Type, Rerun Every, Connection Type, Authentication Account, and Device Type across many devices at once; each field offers a No Change option so untouched settings are left alone.

The day-of-week / day-of-month selectors and date-nested backup directories build on this scheduler in the next PR of the chain (#117).

Related Issue

#133

Motivation and Context

The previous scheduler could only express Daily / Weekly / "Monthly", and "Monthly" was actually every 10 days, with no way to pick a real calendar month or a 10-day cadence. The schema and next-run logic now follow the established Cacti Automation Networks pattern, giving operators the schedule types requested in #133 while remaining compatible with Cacti 1.2.29+.

How Has This Been Tested?

Ran the plugin's Pest suite from a Cacti checkout with the plugin as a real plugins/routerconfigs directory:

  • Full suite: pest --configuration phpunit.xml → 168 passed (1084 assertions). Added tests/Unit/RouterconfigsSchedulerTest.php (19 cases covering Manual, Daily, the 10-day cadence, Weekly with/without weekdays, true Monthly, Monthly-on-Day, year roll-over, last-day-of-month, the legacy fallback, Hourly, the unknown-type guard, and core delegation) and tests/Unit/RouterconfigsScheduleMigrationTest.php (migration mapping + idempotency).
  • Patch coverage gate: php tests/bin/patch-coverage.php coverage/clover.xml origin/develop 100 → 100.00% (136/136 lines).
  • i18n gate: php tests/bin/check-i18n-pot.php origin/develop → OK (locales/po/cacti.pot regenerated; per-language .po/.mo left to Weblate).
  • PHP syntax: php -l clean on every changed file.
  • Manifest: php tests/bin/validate-manifest.php → matches (includes/ already covered).

includes/constants.php was moved out of the instrumented <source> set and into the patch-coverage allowlist: it contains only define() declarations, and is loaded at Pest collection time (before coverage starts) via a top-level require in a Security test, so its lines can never register as covered.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

Copilot AI balanced review requested due to automatic review settings November 11, 2026 22:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Corrupted PHP source prevents plugin loading and test collection, while unresolved scheduling and Telnet authentication defects can trigger unwanted backups or failed logins.

4 open issues
What changed in this PR

Extends RouterConfigs with the remaining scheduling, Telnet login, and bulk-edit features from #133, building on the earlier TFTP and Cacti-device import work.

Changes:

  • Adds schedule types and recurrence settings for 10-day and calendar-month backups, with legacy schedule migration.
  • Adds password-only Telnet login handling when no username is configured.
  • Adds bulk device editing with a “No Change” option for each setting.
File Description
tests/​Unit/​RouterconfigsSchedulerTest.php Adds scheduler calculation and delegation test cases.
tests/​Unit/​RouterconfigsScheduleMigrationTest.php Adds legacy schedule migration test cases.
tests/​bin/​patch-coverage.php Excludes collection-time constant declarations from patch coverage.
setup.php Invokes schedule migration after schema upgrades.
router-devices.php Adds bulk editing and persists scheduler settings.
phpunit.xml Adds scheduler coverage and removes constants coverage.
locales/​po/​cacti.pot Updates translation strings for scheduling and bulk editing.
INFO Bumps the plugin version to 1.10.
includes/​scheduler.php Adds plugin-local scheduling calculations and optional core delegation.
includes/​functions.php Initializes imported schedules and integrates next-backup calculation.
includes/​database.php Adds scheduler columns and legacy migration logic.
includes/​constants.php Defines the bulk-edit action identifier.
includes/​arrays.php Adds schedule controls, recurrence choices, and bulk-edit action.
classes/​PHPTelnet.php Adds a login path that skips the username prompt.
CHANGELOG.md Documents the three feature additions.
.gitignore Ignores PHPUnit cache files.

Review effort: Balanced

Comment thread includes/scheduler.php Resolved
Comment thread classes/PHPTelnet.php Resolved
Comment thread includes/scheduler.php Resolved
Comment thread includes/scheduler.php Resolved
@TheWitness
TheWitness force-pushed the feature/issue-133-schedule-telnet-bulk branch from da01aea to 8c574b6 Compare November 11, 2026 23:01
@TheWitness
TheWitness force-pushed the feature/issue-133-schedule-telnet-bulk branch from 8c574b6 to 1b92698 Compare November 11, 2026 23:43
@TheWitness
TheWitness force-pushed the feature/issue-133-schedule-telnet-bulk branch from 1b92698 to ef91c61 Compare November 11, 2026 23:56

@somethingwithproof somethingwithproof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: Monthly without a day of month skips February, then drifts. When day_of_month is empty, plugin_routerconfigs_schedule_normalize() falls back to date('j', $after), the day of the current run. A run on Jan 31 asks for "February 31 2026", which strtotime() turns into 2026-03-03. February gets no backup, and the device runs on the 3rd from then on. Any late retry moves the day again. Every migrated or newly set Monthly device starts with day_of_month empty, so all of them hit this. Store the day when the device is saved or migrated, or clamp to the last day of the month (day_of_month = 32).

Suggestion: bulk edit doesn't validate the values it writes. plugin_routerconfigs_bulk_edit_set() casts sched_type, account and devicetype to int without checking them against the offered options. An unknown sched_type makes plugin_routerconfigs_schedule_next_start() return 0, so the device is due on every daily scheduled run. Check sched_type against plugin_routerconfigs_sched_types(), and account and device type against existing rows. save_devices() should get the same sched_type check.

Suggestion: Weekly with specific weekdays ignores Rerun Every. With Monday selected and Rerun Every 2, the device runs every week. The field's description promises "every X weeks". Either honor recur_every in the weekday branch or hide the field once weekdays are picked.

Suggestion: changing the schedule doesn't recompute nextbackup. Neither the edit form nor bulk edit updates it, so a device moved from Daily to Monthly runs once more on the old Daily schedule. Recomputing it from plugin_routerconfigs_schedule_next_start() on save would fix both paths.

…le on change

Monthly without a day of month used the day of the current run, so a Jan 31
run asked for "February 31", which strtotime() turned into March 3, and the
device drifted from then on. A Monthly schedule saved, or bulk-set, without
a day now stores the day it was set up on (Last, 32, after the 28th so short
months still run), and the calculator falls back to the 1st rather than the
run day. Days a month does not have (February 30, April 31) are skipped, as
RFC 5545 section 3.3.10 requires, instead of rolling into the next month.

The replica is now used on every Cacti version. Core's
api_scheduler_calculate_next_start() rolls nonexistent dates over, so
delegating to it on 1.3+ would give the same device different backup dates
on 1.2 and 1.3.

Bulk edit validated nothing: sched_type, account and devicetype were cast
to int, so an unknown sched_type resolved to 0 and the device ran on every
scheduled poll. plugin_routerconfigs_bulk_edit_validate() now accepts only
the offered Schedule Types, Rerun Every 1-31, a known Connection Type, and
existing accounts and device types; anything else raises an error and
nothing is changed. save_devices() checks Schedule Type and Rerun Every the
same way.

Changing the schedule now recomputes nextbackup on the device form and in
bulk edit, so a device moved from Daily to Monthly no longer runs once
more on the old schedule. Manual stores 0, which the download query
excludes. A device that has never been scheduled keeps NULL so its first
backup still runs next. Bulk edit reads the selected rows once and groups
devices with identical results into one UPDATE ... WHERE id IN (...).

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Now that nonexistent dates are skipped, a Monthly schedule on February 29
alone has no date in 2026 or 2027. The calculator tried only this year and
next, so it returned 0 and the device became due on every scheduled run.
It now searches 8 years, the longest gap between leap years (2096 to 2104),
and still returns false for a day that never exists, such as February 30.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

@somethingwithproof somethingwithproof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked against the previous review. All four findings are resolved:

  • Monthly with no day of month now stores a fixed day on save (32 once past the 28th), and the normalize fallback is the 1st, so a Jan 31 run no longer skips February or drifts.
  • save_devices() and bulk edit validate sched_type, recur_every, account, device type and connection type against what the form offers, and apply nothing if any value is bad.
  • Weekly + Rerun Every can't be reached in this PR, since there is no Days of Week field yet.
  • nextbackup is recomputed when a schedule field changes, on both the edit and bulk paths.

Nit: includes/scheduler.php:279 adds offset * 86400 to a local-midnight base. When the offset crosses a DST change, that one run lands an hour early or late (a Weekly 02:00 backup runs at 03:00 once). The next recompute corrects it. Building the candidate with strtotime('+N days', $base) would avoid it.

No blockers. CI is green.

@somethingwithproof

Copy link
Copy Markdown
Member

Pushed fixes for the review points:

  • 8677063: Monthly stays on a fixed day. Saving as Monthly fills the set-up day, using Last after the 28th, so Jan 31 no longer drifts to Mar 3. Days that don't exist in a month are skipped, as RFC 5545 does. Bulk edits are validated against an allow-list (schedule type, rerun, connection type, and account and device type checked against their tables) and applied as one SELECT plus one grouped UPDATE ... WHERE id IN per distinct result, instead of one UPDATE per device. nextbackup is recomputed whenever a schedule field changes, on save and in bulk.
  • 8aa842a: the next Monthly date is looked up to 8 years ahead, so a Feb 29-only schedule still resolves (2096 to 2104 skips 2100). A schedule that can never occur resolves to 0.

Tests: new RouterconfigsBulkEditTest, plus scheduler rows for the drift, skipped days, Feb 29 and never-occurs. The drift and skipped-day tests fail on the previous head. CI on 8aa842a: Pest 196 passed on PHP 8.2-8.6, patch coverage 100% (219/219), POT check and SonarCloud pass.

Profiling (PHP 8.5, M3 Max, median of 11): next_start takes 0.2-15 µs for ordinary schedules and about 0.5 ms in the worst case (Monthly on Day with all 12x5x7 combinations, p95 0.54 ms). That is about 2.7 s for 5,000 such devices, so it is not a hotspot. Bulk edit at 5,000 devices issues 1 SELECT and 1-29 UPDATEs, against 5,000 before.

To revisit:

  • next_start always uses the plugin's own copy instead of delegating to core, because core's Monthly handling has the same drift. It could delegate once core is fixed.
  • Devices whose nextbackup is new or NULL stay NULL until the scheduler first runs.

xmacan
xmacan previously approved these changes Nov 12, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
somethingwithproof dismissed stale reviews from xmacan and themself via d971476 November 13, 2026 08:07
@sonarqubecloud

Copy link
Copy Markdown

@TheWitness

Copy link
Copy Markdown
Member Author

Automated validity review of @somethingwithproof's commits (8677063, 8aa842a, d971476)

Reviewed and validated: full Pest suite passes (208 tests, 100% patch coverage), all integration legs (PHP 8.2–8.6) and SonarCloud are green, and the new strtotime('+N days/weeks') scheduling math is DST-robust (verified under America/New_York). The bulk-edit hardening (validate against offered options + existing account/devicetype rows, atomic grouped UPDATEs) and the Monthly fixed-day-of-month / checkdate() nonexistent-date handling are all correct improvements. No functional blockers found.

One design-decision note worth a maintainer confirmation (not a code blocker): commit 8677063 removes the core api_scheduler_calculate_next_start() delegation and now always uses the plugin-local replica, even on Cacti 1.3+. That reverses the approach agreed earlier for this series (plugin-local replica that delegates to core api_scheduler_* when present on 1.3+). The stated rationale is sound — core rolls nonexistent dates (Feb 30, Apr 31) into the next month, so delegating would give the same device different backup dates on 1.2 vs 1.3, whereas the replica skips them per RFC 5545 §3.3.10. Flagging only so the divergence from the original plan is a conscious, documented choice.

@TheWitness
TheWitness merged commit a1b519c into develop Nov 13, 2026
7 checks passed
@TheWitness
TheWitness deleted the feature/issue-133-schedule-telnet-bulk branch November 13, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants