RDKEMW-10363: Update logrotate Conf from Middleware Layer on RDK devices#332
RDKEMW-10363: Update logrotate Conf from Middleware Layer on RDK devices#332madhubabutt wants to merge 1 commit intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes outdated middleware-layer logrotate configuration by deleting the logrotate-update-service-files.patch and associated installation logic. The patch previously added hardcoded configuration files (logrotate_hdd.conf, logrotate_nohdd.conf) and systemd service files that are no longer needed with the current framework-based approach using the logrotate_config bbclass.
Changes:
- Removed reference to
logrotate-update-service-files.patchfrom the bbappend SRC_URI - Deleted the
do_install:append:client()function that installed client-specific configuration - Completely removed the
logrotate-update-service-files.patchfile
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| recipes-extended/logrotate/logrotate_3.21.0.bbappend | Removes patch file reference from SRC_URI, aligning with patch deletion |
| recipes-extended/logrotate/logrotate_3.21.0.bb | Removes client-specific install function that deployed outdated configuration |
| recipes-extended/logrotate/logrotate/logrotate-update-service-files.patch | Complete deletion of obsolete patch containing hardcoded middleware configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4a9f624 to
25ae0b2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c3b9360 to
b649f01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mkdir -p ${D}${sysconfdir}/logrotate.d | ||
| mkdir -p ${D}${localstatedir}/lib | ||
| install -p -m 644 ${S}/examples/logrotate.conf ${D}${sysconfdir}/logrotate.conf | ||
| install -p -m 644 ${S}/examples/btmp ${D}${sysconfdir}/logrotate.d/btmp | ||
| install -p -m 644 ${S}/examples/wtmp ${D}${sysconfdir}/logrotate.d/wtmp | ||
| touch ${D}${localstatedir}/lib/logrotate.status |
There was a problem hiding this comment.
The CONFFILES variable still references btmp and wtmp configuration files that are no longer being installed. Since the installation of these files was removed (lines 73-74 in the diff), they should also be removed from the CONFFILES list to prevent build or runtime warnings about missing configuration files.
3b79f68 to
ec6ef03
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| install -p -m 644 ${S}/examples/logrotate.conf ${D}${sysconfdir}/logrotate.conf | ||
| install -p -m 644 ${S}/examples/btmp ${D}${sysconfdir}/logrotate.d/btmp | ||
| install -p -m 644 ${S}/examples/wtmp ${D}${sysconfdir}/logrotate.d/wtmp | ||
| touch ${D}${localstatedir}/lib/logrotate.status |
There was a problem hiding this comment.
The CONFFILES variable in this recipe references btmp and wtmp configuration files that are no longer being installed. Lines 26-27 declare btmp and wtmp as configuration files, but the installation commands for these files were removed from the do_install function. This inconsistency will cause packaging issues as the system will expect these files to exist but they won't be installed. Consider either:
- Removing the btmp and wtmp references from CONFFILES, or
- Keeping the installation of these files if they are actually needed
| /opt/logs/*.txt.0{ | ||
| size 2097152 | ||
| rotate 1 | ||
| create |
There was a problem hiding this comment.
The new logrotate_nohdd.conf configuration file uses 'create' directive on line 6, which differs from the previous configuration in the deleted patch file that used 'copytruncate'. This is a significant behavioral change:
- 'copytruncate': Copies the log file and truncates the original file in place, keeping the same inode and file descriptor. Applications can continue writing to the same file descriptor without needing to reopen.
- 'create': Renames the original log file and creates a new empty file, changing the inode. Applications need to reopen the file or receive a signal to write to the new file.
The postrotate script on line 10 uses 'syslog-ng-ctl reopen' which should handle the file descriptor reopen. However, this change may affect other applications writing to these log files. Verify that all applications writing to /opt/logs/.txt and /opt/logs/.log files properly handle log rotation with the 'create' directive, or consider whether 'copytruncate' should be retained for backward compatibility.
26235b3 to
717605a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| create | ||
| missingok | ||
| ignoreduplicates | ||
| postrotate | ||
| syslog-ng-ctl reopen --control=/tmp/syslog-ng/syslog-ng.ctl | ||
| endscript |
There was a problem hiding this comment.
The new logrotate_nohdd.conf file uses different directives compared to the version in the deleted patch. Specifically, it replaces 'copytruncate' with 'create' and adds a 'postrotate' script to reopen syslog-ng. This is a significant behavioral change: 'copytruncate' copies and then truncates the file in place, while 'create' moves the file and creates a new one. The postrotate script added is necessary with 'create' mode to signal syslog-ng to reopen log files, but this change should be verified to ensure it matches the intended behavior for RDK devices.
| FILES:${PN}-conf += "${sysconfdir}/logrotatemax.conf" | ||
| FILES:${PN}-conf += "${sysconfdir}/logrotate.conf" | ||
|
|
||
| FILES:${PN}:remove = "${sysconfdir}/logrotatemax.conf" |
There was a problem hiding this comment.
The logrotatemax.conf file is assigned to the new logrotate-conf package but only gets installed on client machines (via do_install:append:client). This means the logrotate-conf package will be incomplete on non-client builds, potentially containing only logrotate.conf but not logrotatemax.conf. Consider whether the package splitting logic should account for machine-specific configurations or if the FILES assignment should be conditional.
| FILES:${PN}-conf += "${sysconfdir}/logrotatemax.conf" | |
| FILES:${PN}-conf += "${sysconfdir}/logrotate.conf" | |
| FILES:${PN}:remove = "${sysconfdir}/logrotatemax.conf" | |
| FILES:${PN}-conf:client += "${sysconfdir}/logrotatemax.conf" | |
| FILES:${PN}-conf += "${sysconfdir}/logrotate.conf" | |
| FILES:${PN}:client:remove = "${sysconfdir}/logrotatemax.conf" |
There was a problem hiding this comment.
With the new package splitting that moves logrotate.conf and logrotatemax.conf to a separate logrotate-conf package, the CONFFILES declaration should be updated to reflect this split. Currently, CONFFILES:${PN} includes logrotate.conf (line 25 of the main recipe), but after this change, logrotate.conf belongs to logrotate-conf package. Consider adding CONFFILES:${PN}-conf to properly mark these configuration files for the new package, and updating CONFFILES:${PN} to remove logrotate.conf.
| CONFFILES:${PN}-conf += "${sysconfdir}/logrotate.conf ${sysconfdir}/logrotatemax.conf" | |
| CONFFILES:${PN}:remove = "${sysconfdir}/logrotate.conf ${sysconfdir}/logrotatemax.conf" |
| /opt/logs/*.txt | ||
| /opt/logs/*.log | ||
| /opt/logs/*.txt.0{ |
There was a problem hiding this comment.
The logrotate configuration includes overlapping file patterns: '/opt/logs/.txt', '/opt/logs/.log', and '/opt/logs/.txt.0'. The third pattern '/opt/logs/.txt.0' appears to target already-rotated files, which would match files like 'example.txt.0'. This could cause logrotate to attempt rotating already rotated files, leading to unexpected behavior. Verify if this is intentional or if it should be adjusted to only target active log files.
| /opt/logs/*.txt | |
| /opt/logs/*.log | |
| /opt/logs/*.txt.0{ | |
| /opt/logs/*.txt /opt/logs/*.log { |
717605a to
f7b3217
Compare
No description provided.