Use getTimeRotatingLogger for WMCore REST

[d-ylee]

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

https://github.com/dmwm/CMSKubernetes/pull/1452

External dependencies / deployment changes

Potentially remove the use of rotatelogs.

[cmsdmwmbot]

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

[cmsdmwmbot]

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]

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

To add a note: This function doesn't do what the docstring says: https://github.com/dmwm/WMCore/blob/dd7835203b951857de3a544c0a5dc8ed50d121c7/src/python/WMCore/WMLogging.py#L68-L85

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.

[cmsdmwmbot]

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]

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]

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]

test this please

[cmsdmwmbot]

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

[d-ylee]

@amaltaro If we remove the lines in RESTDaemon as you suggested, should we also remove this: https://github.com/dmwm/WMCore/blob/6228aa0268f7cab8e36cb9d0698c1d1ddb4de4e3/src/python/WMCore/REST/Main.py#L410-L413

I think this was used to setup streaming to rotatelogs.

The other thing is that the line you referenced: https://github.com/dmwm/WMCore/blob/6228aa0268f7cab8e36cb9d0698c1d1ddb4de4e3/src/python/WMCore/REST/Main.py#L319

...gets overwritten if we provide -l: https://github.com/dmwm/WMCore/blob/6228aa0268f7cab8e36cb9d0698c1d1ddb4de4e3/src/python/WMCore/REST/Main.py#L622-L629

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

[cmsdmwmbot]

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]

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

test this please

[cmsdmwmbot]

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]

Can one of the admins verify this patch?