Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extending the scope of the maxFiles parameter in logging configuration #30839

Open
wants to merge 8 commits into
base: integration
Choose a base branch
from
Open

Extending the scope of the maxFiles parameter in logging configuration #30839

wants to merge 8 commits into from

Conversation

ncpibm
Copy link
Contributor

@ncpibm ncpibm commented Feb 21, 2025 •
edited
Loading

fixes #30932
fixes #PH65488

The proposed change enhances the log cleanup mechanism in Liberty by ensuring that log files exceeding the configured maxFiles limit are deleted, regardless of whether they were created by the currently running server process or a previous instance. The existing behavior only removes old logs when the currently running process has created more than maxFiles logs, leaving older logs from previous runs untouched. As a result, when the server restarts, these old log files remain.

With the implemented logic, when the server starts, it scans the entire log output directory for files matching the log filename pattern, regardless of which process created them. It then sorts these files by their last modified timestamp, retaining only the most recent ones within the maxFiles limit and deleting the oldest ones. This ensures that the log directory does not grow indefinitely and strictly adheres to the configured retention policy. Additionally, the change includes debug-level tracing to log the deletion of old log files, improving visibility during troubleshooting.

Furthermore, in the cleanup logic, the this.backups variable is now updated before deleting old log files to ensure the newly rotated log file is properly tracked. Previously, this update occurred after the deletion logic, but repositioning it ensures the latest log file is accounted for before older ones are removed.

@ncpibm ncpibm self-assigned this Feb 21, 2025
@ncpibm ncpibm closed this Feb 21, 2025
@ncpibm ncpibm reopened this Feb 21, 2025
@ncpibm ncpibm closed this Feb 21, 2025
@ncpibm ncpibm reopened this Feb 21, 2025
@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@ncpibm
Copy link
Contributor Author

ncpibm commented Feb 21, 2025 •
edited by LibbyBot
Loading

#build (view Open Liberty Personal Build - ✅ completed successfully!)

Note: Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@LibbyBot
Copy link

https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?pipelineId=02e35eb6-207c-4710-bbe9-d0dd1c132f3a - Open Liberty Personal Build completed successfully!

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_SmIBEPCIEe-iabY-3Iflhw

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_Qos1oPCJEe-iabY-3Iflhw

Target locations of links might be accessible only to IBM employees.

@ncpibm ncpibm marked this pull request as ready for review February 23, 2025 11:37
@ncpibm
Copy link
Contributor Author

ncpibm commented Feb 23, 2025 •
edited by LibbyBot
Loading

#build (view Open Liberty Personal Build - ❌ completed with errors/failures.)

Note: Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_tegmMPHwEe-5mo24eL2W6g

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 2 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@ncpibm
Copy link
Contributor Author

ncpibm commented Feb 27, 2025 •
edited by LibbyBot
Loading

#build (view Open Liberty Personal Build - ❌ completed with errors/failures.)

Note: Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 2 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_EQM8oPUjEe-5mo24eL2W6g

Target locations of links might be accessible only to IBM employees.

pnicolucci
pnicolucci reviewed Mar 4, 2025
View reviewed changes
...erty.transport.http_fat/fat/src/io/openliberty/transport/http_fat/AccessLogRolloverTest.java Outdated

// Wait for log rollover to occur
LOG.info("Waiting for log rollover to complete...");
Thread.sleep(180000);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to avoid these sleeps? These are going to add up.

Copy link
Contributor Author

@ncpibm ncpibm Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is set to allow the logs to roll over. After the rollover, we will verify that our property works as expected. Another way to trigger the rollover would be to hit the URL multiple times until the logs reach the minimum file size of 1MB. However, based on my local testing, this approach would take a similar amount of time or even longer. . I will look into if there is any other way to rollover.

pnicolucci
pnicolucci reviewed Mar 4, 2025
View reviewed changes
Copy link
Member

@pnicolucci pnicolucci left a comment

Choose a reason for hiding this comment

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

I've given an initial review, I'd like to see the initial comments addressed and then I can take another look. Also please look into squashing your commits, I think many of them can be combined to keep the change history clean. Let me know if you have any questions.

I'd also like to see additional tests added. You're testing maxfiles = 2, which is the default, can we also test the default without specifying it? Also maybe setting maxfiles to 0 and a value other than the default of 2. What about setting an invalid value, although we're not changing this function we should try to expand the test cases in this area if at all possible.

If these tests exist someplace else let me know.

@ncpibm ncpibm force-pushed the 27819-Extending_scope_of_maxfiles branch from be88f9f to 2434249 Compare March 5, 2025 05:01
@ncpibm ncpibm force-pushed the 27819-Extending_scope_of_maxfiles branch from 363f302 to de74459 Compare March 6, 2025 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pnicolucci pnicolucci Awaiting requested review from pnicolucci

@isaacrivriv isaacrivriv Awaiting requested review from isaacrivriv

@mrsaldana mrsaldana Awaiting requested review from mrsaldana

At least 1 approving review is required to merge this pull request.

Assignees

@ncpibm ncpibm

Labels
CLA Signed in:Transport team:Sirius
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MaxFiles parameter in accesslogging is not working when the system restarts.
3 participants
@ncpibm @LibbyBot @pnicolucci