-
Notifications
You must be signed in to change notification settings - Fork 109
Use getTimeRotatingLogger for WMCore REST #11955
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: master
Are you sure you want to change the base?
Use getTimeRotatingLogger for WMCore REST #11955
Conversation
|
Jenkins results:
|
vkuznet
left a comment
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.
Dennis, changes are fine but I don't know how getTimeRotatingLogger works and will rely on you that you tested it and confirmed that it works as expected.
|
Jenkins results:
|
|
@amaltaro I was testing this and it was rotating the logs, but not retaining them or adding the date. I found out that the date was originally provided in With Without Now, I changed Example In |
|
To add a note: WMCore/src/python/WMCore/WMLogging.py Lines 68 to 85 in dd78352
The new date string is replaced, not added. So if the string doesn't contain yesterdayStr, then the string isn't added and the baseName remains the same. |
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.
Dennis, the changes are fine but I would suggest to improve description and add to it exact syntax how to use this when we invoke python program., in other words, so far we use wmc-httpd -r -d $STATEDIR -l "|rotatelogs $LOGDIR/$srv-%Y%m%d-`hostname -s`.log 86400" $CFGFILE and it would be beneficial to have an example how we will call similar daemon (or python command) with new logger.
5062326 to
3358120
Compare
|
Jenkins results:
|
3358120 to
5a54b57
Compare
|
Jenkins results:
|
5a54b57 to
6228aa0
Compare
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
amaltaro
left a comment
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.
@d-ylee I left a few comments/questions along the code.
I also wanted to point out to this line:
WMCore/src/python/WMCore/REST/Main.py
Line 319 in 6228aa0
| self.logfile = ["rotatelogs", "%s/%s-%%Y%%m%%d.log" % (self.statedir, self.appname), "86400"] |
which IMO should be removed with this PR. In other words, I would say the log file name option has to be as simple as -l filename.log or -l filename_date.log and that is it. No need to accept pipe, rotatelogs and time in secs. Any thoughts?
src/python/WMCore/REST/Main.py
Outdated
| if opts.logfile: | ||
| if opts.logfile.startswith("|"): | ||
| server.logfile = re.split(r"\s+", opts.logfile[1:]) | ||
| else: |
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.
Perhaps the option above in the if statement should be removed and we define this option to be in the format of -l my_logfile.log only. In other words, |, rotatelogs and 86400 (integer with time in seconds) are no longer an acceptable option. What do you think?
958a24a to
a43ebca
Compare
|
@amaltaro If we remove the lines in RESTDaemon as you suggested, should we also remove this: WMCore/src/python/WMCore/REST/Main.py Lines 410 to 413 in 6228aa0
I think this was used to setup streaming to The other thing is that the line you referenced: WMCore/src/python/WMCore/REST/Main.py Line 319 in 6228aa0
...gets overwritten if we provide WMCore/src/python/WMCore/REST/Main.py Lines 622 to 629 in 6228aa0
So the change will have the RESTDaemon default to logging to a file. |
|
Jenkins results:
|
|
@d-ylee yes, I think you are correct about the Given that this line: was being used as default and get overwritten by the or perhaps simply: ? For now, I'd suggest to keep it in a different commit, just in case we regret this change :) |
|
test this please |
|
Jenkins results:
|
|
Can one of the admins verify this patch? |
a43ebca to
dbe1fc2
Compare
|
@amaltaro Is there anything we should add to this pull request? |
|
test this please |
|
Jenkins results:
|
|
Once we have this change in, we can likely remove the dependency on I don't know if anything is used from this package, but rotatelogs is: |
|
@amaltaro I created a PR here for removing dmwm-alma9-base doesn't install |
|
After doing the testing for CMSKubernetes#1604, there are some issues during the rollover period of the log file, with the file not actually being renamed. I am doing more further investigation. |
629a2c4 to
6092fbf
Compare
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
6d34d12 to
6d8a7f4
Compare
|
Jenkins results:
|
6d8a7f4 to
b35a3fc
Compare
|
Jenkins results:
|
|
@amaltaro While doing some investigation today, I noticed that MSUnmerged is creating 3 log files:
[1] and [2] both utilize the WMTimedRotatingFileHandler that is being worked on in this PR. [3] on the other hand seems to be something separate. Should the CherryPy RESTApi logs be in the same file as [2]? |
|
Jenkins results:
|
8554b05 to
03be719
Compare
|
Jenkins results:
|
87bb3c5 to
171da3f
Compare
|
Jenkins results:
|
Use getTimeRotatingLogger for WMCore REST Added date to filename for MyTimedRotatingFileHandler if it doesn't have it Added usage for wmc-httpd LOG-FILE arg Renamed MyTimedRotatingFileHandler to WMTimedRotatingFileHandler Simplified WMTimedRotatingFileHandler to use namer function Remove unused code and docstrings Fixed service log rotation Add QueueHandler for cherrypy, remove redirecting logging QueueHandler for cherrypy logs Added a queue listener thread that aggregates all logs that are handled with the WMTimedRotatingLogger Removed redirecting logging to a subprocess or a file using file descriptors
Updated WMLogging test to test a quick log rotation Fix some pylint errors in WMLogging_t.py Replace existing log rotation test
171da3f to
6647c4e
Compare
|
Jenkins results:
|
Fixes/Is a part of #11922
Status
In development | not-tested
Description
Should set up a Python TimedRotatingFileHandler to replace the usage of
rotatelogsIs it backward compatible (if not, which system it affects?)
MAYBE
Should only be used when
|rotatelogs ...is not used during deploymentRelated PRs
dmwm/CMSKubernetes#1452
dmwm/CMSKubernetes#1604
External dependencies / deployment changes
Potentially remove the use of
rotatelogs.