Cmslinks issues #803
Cmslinks issues #803
Conversation
| @@ -235,10 +251,18 @@ def update(self, overwrite=False, disable=True, dry=False, srcselect=r'\S+', dst | |||
|
|
|||
| OPTIONS = PARSER.parse_args() | |||
|
|
|||
| # Configure logger | |||
| # Add new logging level, so usefull info is seperated from info | |||
| USEFULL_INFO_NUM = 25 | |||
There was a problem hiding this comment.
Why do you need a custom logging level?
There was a problem hiding this comment.
In the script there are info and warning logs. By default logging level is set to info, however info logs mainly populate the logs instead of providing helpful information. In addition, there is information that there is need to be logged, for example when a new distance is set or when a distance is overwritten. So, a new log level, that sits between info and warning levels, was set for this king of information. This new level was set also as default sο that there is no unneeded filling of the logs.
There was a problem hiding this comment.
Feel free to remove/modify the level for existing logs. I believe the 5 default log levels are enough.
There was a problem hiding this comment.
Unimportant logs moved to debug level
| logging.warning(f'Problem getting PNN from gitlab for RSE {rse} SITE {site.get("site")}. Error: {str(e)}') | ||
|
|
||
| # If PNN could be retrieved skip to the next RSE (do not add entry to the list) | ||
| if pnn is None: |
There was a problem hiding this comment.
You can add the continue under exceptions above.
There was a problem hiding this comment.
The problem here is that the desired 'rse' field may be empty. Moving continue under the exception above will skip to the next site. However, in case that no sites have the 'rse' field set, the pnn value will remain 'None'. Appending this value to self.rselist will create problems in other parts of the code. So, preventing RSEs with pnn value of None to be appended in the list is mandatory.
| @@ -182,21 +193,26 @@ def update(self, overwrite=False, disable=True, dry=False, srcselect=r'\S+', dst | |||
| if dry: | |||
There was a problem hiding this comment.
You can refactor this and everywhere else to:
logging.log ("message")
if not dry_run:
//perform the actual operation
|
It is worth mentioning that since the script was not be running, applying script changes and running the cronejob with the updated code will apply several changes in the distances of most RSEs. |
|
Can you find out which distances will be effected and from what to what? Also, remember to |
Changes have been made. Request ready to be reviewed again. Commits are squashed. cmslinks_dryrun_logs.txt contains logs from a dry run. In the first lines you can see warnings regarding pnn issue and the rest is logs regarding distances.
|
There was a problem hiding this comment.
LGTM.
Try to make sense of the results of the dry-run.
I have assigned this to you: https://its.cern.ch/jira/browse/CMSTRANSF-515
|
As mentioned in the Jira issue, I would like to highlight following points.
From my understanding the DEFAULT_EXCLUDE_LINKS is up to date with the changes requested in the Jira issue. Let me know what you think. |
|
Hello @eachristgr |
As noticed during resolving issue CMSDM-173, cmslinks.py could not be executed successfully from the corresponding cronjob and its logs were not visible in the corresponding container's logs.
Details can be found in the corresponding issue #804.