overlord/changeslogger: add changes logger manager#16678
overlord/changeslogger: add changes logger manager#16678asasine wants to merge 1 commit intocanonical:masterfrom
Conversation
93fa18e to
549e631
Compare
|
@asasine wow, thank you so much! I will review this and ask my colleagues for opinions. As for integration testing we can explore that together. |
There was a problem hiding this comment.
Pull request overview
This pull request adds a new changes logger manager to the snapd overlord that logs all state changes to /var/log/snapd/changes.log in a human-readable format. The feature implements a long-standing feature request (9 years old) to provide audit logging of snapd operations.
Changes:
- Introduces a new
overlord/changesloggerpackage that implements the StateManager interface to monitor and log changes - Updates packaging files across multiple distributions to create the
/var/log/snapddirectory - Adds infrastructure in the dirs package to define the new log file path
- Updates snap CLI help text to inform users about the new log file
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| overlord/changeslogger/changeslogger.go | Core implementation of the changes logger manager with Ensure() and StartUp() methods |
| overlord/changeslogger/changeslogger_test.go | Unit tests verifying log creation, status change tracking, and directory creation |
| overlord/changeslogger/export_test.go | Test helper to expose internal fields for testing |
| overlord/overlord.go | Integration of the changes logger manager into the overlord |
| dirs/dirs.go | Defines SnapdLogDir and SnapdChangesLog path variables |
| cmd/snap/cmd_changes.go | Updates help text to reference the new log file |
| packaging/ubuntu-16.04/snapd.dirs | Adds var/log/snapd directory creation |
| packaging/debian-sid/snapd.dirs | Adds var/log/snapd directory creation |
| packaging/snapd.mk | Adds var/log/snapd directory creation in makefile |
| packaging/opensuse/snapd.spec | Adds var/log/snapd directory in RPM spec |
| packaging/fedora/snapd.spec | Adds var/log/snapd directory in RPM spec, plus whitespace cleanup |
| mu sync.Mutex | ||
| seenChanges map[string]ChangeInfo | ||
| changeLogPath string | ||
| retryCount map[string]int |
There was a problem hiding this comment.
The retryCount field is defined but never incremented. Task retry counts are tracked internally by the state machine, not by changes themselves. Since this field is unused, it should be removed along with its references at lines 153-156 in the logChange method.
| if stat, err := os.Stat(m.changeLogPath); err == nil { | ||
| logger.Debugf("Changes log file already exists at %q (size: %d)", m.changeLogPath, stat.Size()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Log files can grow unbounded without rotation. The PR does not include logrotate configuration or any mechanism to prevent the changes.log file from growing indefinitely. Consider adding a logrotate configuration file in the data/ directory or documenting the need for one.
| // Note: this manager does not implement log rotation for the changes log. | |
| // The file at dirs.SnapdChangesLog is expected to be managed by an external | |
| // log rotation mechanism (for example, a logrotate configuration in the | |
| // data/ directory) to prevent unbounded growth over time. |
There was a problem hiding this comment.
This is a much welcome improvement to the long-standing problem of changes being recycled and an absence of a retention mechanism.
I can see the following high-level questions:
-
The format of the file. We could make it YAML-readable by making a small tweak (suggested inline)
-
Rotation of the log. At present, AFAIK, nothing would rotate the log. We might want to ship a configuration file to ensure the file does get rotated, on classic systems, over time.
-
Ability to enable or disable. For some systems it might be desirable to not enable this feature. I think the directory itself can be created but change retention should be governed by a core configuration toggle, similar to persistent journal or ssh. This would allow making a device that ships with this feature disabled to avoid writing to disk more.
-
Resilience to rm -rf /var/log/snapd. I think we ought to handle mkdir in ensure, not just on manager startup.
Separately to this we would need to write a few integration tests but I'm sure we can manage that in the team as the feature is fairly isolated.
| type Manager struct { | ||
| state *state.State | ||
| mu sync.Mutex | ||
| seenChanges map[string]ChangeInfo |
There was a problem hiding this comment.
| seenChanges map[string]ChangeInfo | |
| seenChanges map[string]ChangeInfo // indexed by change ID |
| - interfaces/u2f-devices: add Nitrokey 3 | ||
| - Update the ubuntu-image channel to candidate | ||
| - Allow hostnames up to 253 characters, with dot-delimited elements | ||
| - Allow hostnames up to 253 characters, with dot-delimited elements |
There was a problem hiding this comment.
This is unrelated but we can handle that separately.
| } | ||
|
|
||
| // Write blank line separator between entries | ||
| if _, err := f.WriteString("\n"); err != nil { |
There was a problem hiding this comment.
Should we use the yaml --- record separator? We could then treat the whole file as a YAML and automatically be machine readable.
There was a problem hiding this comment.
Seems reasonable to me. My goal was to keep the file as plain-texty as possible so it wouldn't require special tools to parse and would therefore be trivially scriptable with native tools that come bundled with most distributions.
Separating by --- seems reasonable to me but do you think it's fine to keep the extension as .log and the rest of the file simple (e.g., no nested mappings, sequences, or other YAML-isms)?
https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1695934 has been open for nine years so I thought I'd take a chance at implementing it myself. The changes are sent to
/var/log/snapd/changes.logand are formatted as below, separated by a blank line between each change:I've added unit tests but was not entirely sure how to test this end-to-end.