Use getTimeRotatingLogger for WMCore REST #11955

d-ylee · 2024-04-01T21:34:18Z

Fixes/Is a part of #11922

Status

In development | not-tested

Description

Should set up a Python TimedRotatingFileHandler to replace the usage of rotatelogs

Is it backward compatible (if not, which system it affects?)

MAYBE
Should only be used when |rotatelogs ... is not used during deployment

Related PRs

dmwm/CMSKubernetes#1452

External dependencies / deployment changes

Potentially remove the use of rotatelogs.

cmsdmwmbot · 2024-04-01T21:54:32Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 tests no longer failing
Python3 Pylint check: failed
- 17 warnings and errors that must be fixed
- 4 warnings
- 110 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 40 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15002/artifact/artifacts/PullRequestReport.html

vkuznet

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.

cmsdmwmbot · 2024-04-09T03:02:57Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 changes in unstable tests
Python3 Pylint check: failed
- 18 warnings and errors that must be fixed
- 4 warnings
- 115 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 52 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15003/artifact/artifacts/PullRequestReport.html

d-ylee · 2024-04-09T03:03:53Z

@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 rotatelogs when calling wmc-httpd:

With rotatelogs:
https://github.com/dmwm/CMSKubernetes/blob/326de33e3769e2f66e088c91093d3179a8d1cc12/docker/pypi/reqmgr2ms-unmerged/run.sh#L145

Without rotatelogs:
https://github.com/dmwm/CMSKubernetes/blob/a6dfdee30f4a2cabc308663f2f3a9b215e08e991/docker/pypi/reqmgr2ms-unmerged/run.sh#L145

Now, I changed MyTimedRotatingFileHandler to add the YearMonthDate string when initializing the logger. I'm not sure if we want to inject the date via Python, or if we want to 100% replicate how we use rotatelogs by passing what we had originally in rotatelogs and replace %Y%m%d string in the MyTimedRotatingFileHandler.__init__ function with a todayStr.

Example
In run.sh:

wmc-httpd -r -d $STATEDIR -l "$LOGDIR/$srv-%Y%m%d-`hostname -s`.log 86400" $CFGFILE

In WMLogging.MyTimedRotatingFileHandler.__init__

 def __init__(self, logName, interval, backupCount):
        # Add todayStr to log name, like with rotatelogs
        todayStr = date.today().strftime("%Y%m%d")
        filename = logName.replace('%Y%m%d', todayStr)
        super(MyTimedRotatingFileHandler, self).__init__(filename, when=interval,
                                                         backupCount=backupCount)

d-ylee · 2024-04-09T03:15:03Z

To add a note:
This function doesn't do what the docstring says:

WMCore/src/python/WMCore/WMLogging.py

Lines 68 to 85 in dd78352

    
               def doRollover(self): 
        
                   """ 
        
                   _doRollover_ 
        
                   Rotate the log file and add the date between the log name 
        
                   and its extension, e.g.: 
        
                   reqmgr2.log becomes reqmgr2-20170815.log 
        
                   """ 
        
                   self.stream.close() 
        
                   # replace yesterday's date by today 
        
                   yesterdayStr = (date.today() - timedelta(1)).strftime("%Y%m%d") 
        
                   todayStr = date.today().strftime("%Y%m%d") 
        
                   self.baseFilename = self.baseFilename.replace(yesterdayStr, todayStr) 
        
                   if self.encoding: 
        
                       self.stream = codecs.open(self.baseFilename, 'w', self.encoding) 
        
                   else: 
        
                       self.stream = open(self.baseFilename, 'w') 
        
                   self.rolloverAt = self.rolloverAt + self.interval

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.

vkuznet

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.

cmsdmwmbot · 2024-04-09T21:06:06Z

Jenkins results:

Python3 Unit tests: failed
- 1 new failures
- 1 tests no longer failing
- 1 tests added
- 1 changes in unstable tests
Python3 Pylint check: failed
- 16 warnings and errors that must be fixed
- 4 warnings
- 119 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 57 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15007/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2024-04-10T15:04:11Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 tests added
- 2 changes in unstable tests
Python3 Pylint check: failed
- 16 warnings and errors that must be fixed
- 4 warnings
- 120 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 58 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15008/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2024-04-10T16:57:47Z

Jenkins results:

Python3 Unit tests: failed
- 7 new failures
- 1 tests added
- 1 changes in unstable tests
Python3 Pylint check: failed
- 16 warnings and errors that must be fixed
- 4 warnings
- 112 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15009/artifact/artifacts/PullRequestReport.html

d-ylee · 2024-04-10T18:08:28Z

test this please

cmsdmwmbot · 2024-04-10T18:22:55Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 tests added
Python3 Pylint check: failed
- 16 warnings and errors that must be fixed
- 4 warnings
- 112 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 31 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15010/artifact/artifacts/PullRequestReport.html

amaltaro

@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

amaltaro · 2024-04-15T18:03:16Z

src/python/WMCore/REST/Main.py

@@ -619,5 +625,8 @@ def main():
            else:


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?

Added date to filename for MyTimedRotatingFileHandler if it doesn't have it Added usage for wmc-httpd LOG-FILE arg

d-ylee · 2024-04-17T13:42:30Z

@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

    
           if isinstance(self.logfile, list): 
        
               subproc = Popen(self.logfile, stdin=PIPE, stdout=devnull, stderr=devnull, 
        
                               bufsize=0, close_fds=True, shell=False) 
        
               logger = subproc.stdin

I think this was used to setup streaming to rotatelogs.

The other thing is that the line you referenced:

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"]

...gets overwritten if we provide -l:

WMCore/src/python/WMCore/REST/Main.py

Lines 622 to 629 in 6228aa0

    
           if opts.logfile: 
        
               if opts.logfile.startswith("|"): 
        
                   server.logfile = re.split(r"\s+", opts.logfile[1:]) 
        
               else: 
        
                   server.logfile = opts.logfile 
        
                   # setup rotating log 
        
                   getTimeRotatingLogger(None, server.logfile)

So the change will have the RESTDaemon default to logging to a file.

cmsdmwmbot · 2024-04-17T13:50:32Z

Jenkins results:

Python3 Unit tests: failed
- 1 new failures
- 1 tests added
- 1 changes in unstable tests
Python3 Pylint check: failed
- 18 warnings and errors that must be fixed
- 4 warnings
- 111 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 33 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15019/artifact/artifacts/PullRequestReport.html

amaltaro · 2024-04-17T14:04:22Z

@d-ylee yes, I think you are correct about the Popen and I am inclined to say that we should completely remove that, as we no longer expect (accept?) a list type parameter for the logfile.

Given that this line:

self.logfile = ["rotatelogs", "%s/%s-%%Y%%m%%d.log" % (self.statedir, self.appname), "86400"]

was being used as default and get overwritten by the -l option. How about we keep this default value, but with the following difference:

self.logfile = "%s/%s-%%Y%%m%%d.log".format(self.statedir, self.appname, todayStr)

or perhaps simply:

self.logfile = "%s/%s.log".format(self.statedir, self.appname)

?

For now, I'd suggest to keep it in a different commit, just in case we regret this change :)

d-ylee · 2024-07-16T18:20:41Z

test this please

cmsdmwmbot · 2024-07-16T18:32:46Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 tests no longer failing
- 1 tests added
- 1 changes in unstable tests
Python3 Pylint check: failed
- 18 warnings and errors that must be fixed
- 4 warnings
- 111 comments to review
Pylint py3k check: succeeded
Pycodestyle check: succeeded
- 33 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/15106/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2024-09-30T20:59:23Z

Can one of the admins verify this patch?

d-ylee requested review from vkuznet and amaltaro April 1, 2024 21:34

vkuznet approved these changes Apr 8, 2024

View reviewed changes

d-ylee marked this pull request as draft April 8, 2024 13:58

d-ylee marked this pull request as ready for review April 9, 2024 03:10

d-ylee requested a review from vkuznet April 9, 2024 03:58

vkuznet reviewed Apr 9, 2024

View reviewed changes

d-ylee force-pushed the 11922-build-a-msunmerged-docker-image-based-on-almalinux branch from 5062326 to 3358120 Compare April 9, 2024 20:53

d-ylee force-pushed the 11922-build-a-msunmerged-docker-image-based-on-almalinux branch from 3358120 to 5a54b57 Compare April 10, 2024 14:53

d-ylee force-pushed the 11922-build-a-msunmerged-docker-image-based-on-almalinux branch from 5a54b57 to 6228aa0 Compare April 10, 2024 16:40

amaltaro requested changes Apr 15, 2024

View reviewed changes

d-ylee added 3 commits April 17, 2024 08:39

Use getTimeRotatingLogger for WMCore REST

77eb0f5

Added date to filename for MyTimedRotatingFileHandler if it doesn't have it Added usage for wmc-httpd LOG-FILE arg

Added test for WMCore.WMLogging.MyTimedRotatingFileHandler

1c955e2

Removed rotatelogs setup in RESTDaemon

a43ebca

d-ylee force-pushed the 11922-build-a-msunmerged-docker-image-based-on-almalinux branch from 958a24a to a43ebca Compare April 17, 2024 13:39

amaltaro added the PR: Do not merge yet label Apr 29, 2024

amaltaro mentioned this pull request Apr 29, 2024

Build a MSUnmerged docker image based on Almalinux #11922

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getTimeRotatingLogger for WMCore REST #11955

Use getTimeRotatingLogger for WMCore REST #11955

d-ylee commented Apr 1, 2024

cmsdmwmbot commented Apr 1, 2024

vkuznet left a comment

cmsdmwmbot commented Apr 9, 2024

d-ylee commented Apr 9, 2024 •

edited

Loading

d-ylee commented Apr 9, 2024

vkuznet left a comment •

edited

Loading

cmsdmwmbot commented Apr 9, 2024

cmsdmwmbot commented Apr 10, 2024

cmsdmwmbot commented Apr 10, 2024

d-ylee commented Apr 10, 2024

cmsdmwmbot commented Apr 10, 2024

amaltaro left a comment

amaltaro Apr 15, 2024

d-ylee commented Apr 17, 2024

cmsdmwmbot commented Apr 17, 2024

amaltaro commented Apr 17, 2024

d-ylee commented Jul 16, 2024

cmsdmwmbot commented Jul 16, 2024

cmsdmwmbot commented Sep 30, 2024

Use getTimeRotatingLogger for WMCore REST #11955

Are you sure you want to change the base?

Use getTimeRotatingLogger for WMCore REST #11955

Conversation

d-ylee commented Apr 1, 2024

Status

Description

Is it backward compatible (if not, which system it affects?)

Related PRs

External dependencies / deployment changes

cmsdmwmbot commented Apr 1, 2024

vkuznet left a comment

Choose a reason for hiding this comment

cmsdmwmbot commented Apr 9, 2024

d-ylee commented Apr 9, 2024 • edited Loading

d-ylee commented Apr 9, 2024

vkuznet left a comment • edited Loading

Choose a reason for hiding this comment

cmsdmwmbot commented Apr 9, 2024

cmsdmwmbot commented Apr 10, 2024

cmsdmwmbot commented Apr 10, 2024

d-ylee commented Apr 10, 2024

cmsdmwmbot commented Apr 10, 2024

amaltaro left a comment

Choose a reason for hiding this comment

amaltaro Apr 15, 2024

Choose a reason for hiding this comment

d-ylee commented Apr 17, 2024

cmsdmwmbot commented Apr 17, 2024

amaltaro commented Apr 17, 2024

d-ylee commented Jul 16, 2024

cmsdmwmbot commented Jul 16, 2024

cmsdmwmbot commented Sep 30, 2024

d-ylee commented Apr 9, 2024 •

edited

Loading

vkuznet left a comment •

edited

Loading