Make ResourceControlUpdater to continuously update PNNs in the database #11599
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
| @@ -70,6 +71,10 @@ def __init__(self, config): | |||
| # set resource control | |||
| self.resourceControl = ResourceControl(config=self.config) | |||
|
|
|||
| # get CRIC wrapper instance | |||
| self.cricPattern = ".*" # all sites | |||
There was a problem hiding this comment.
there is no need to create self data with static value since it never changed. Unless it shown otherwise I suggest to move value .* directly into updatePNNMap function and put it into function call where the comment will be more appropriate.
There was a problem hiding this comment.
I see pros and cons in each implementation. With this, we have a clear definition at the initialization of the object about which parameters are used within this module/thread. Creating a constant inside the function is fine as well. I will update this code soon.
| """ | ||
| # first create a dictionary to make key look-up easier | ||
| resp = {} | ||
| for thisItem in DBFormatter.format(self, result): |
There was a problem hiding this comment.
since below you takes elements of thisItem it is better to check that its length has at least 7 elements (the largest index I see in code below). If not it is better to provide appropriate logging message and skip the rest of the code in a loop.
There was a problem hiding this comment.
It could be done, but this is a SQL statement where we request 7 fields, hence it must have 7 elements in the data objects. I don't think we gain anything from adding this check here.
There was a problem hiding this comment.
Alan, my point is that it was unclear reading the PR code why we need 7 elements until I opened full file and saw the SQL statement which you mentioned. If you do not want to perform the check which will not hurt the codebase, then at least put appropriate comment either in docstring or better directly in for loop codebase that severn elements are part of SQL statement.
| siteName = siteInfo['site_name'] | ||
| if siteName not in cricMap: | ||
| logging.warning("Site name: %s cannot be found in CRIC.", siteName) | ||
| continue |
There was a problem hiding this comment.
I do not think we should simply continue here. We'd rather do something. the fact that the site has disappeared from CRIC must mean something. So I'd rather try to update the Agent's resource control and remove it from there as well. Even though it may require some more complicated code changes.
There was a problem hiding this comment.
The CRIC call uses the data-processing API, which requires an association of PNN and PSN.
Many opportunistic resources do not have a PNN, and those would fall into this warning message. Maybe I could update the message to something like "cannot be found in CRIC data-processing API"?
There was a problem hiding this comment.
Hi @amaltaro
Thanks for this PR, I've left few comments inline.
|
|
||
| results[0]["pnn"] = pnns | ||
| return results[0] | ||
| return results |
There was a problem hiding this comment.
Well, the DAO did change it's return format... that's obvious, but is this the only place where it is used?
There was a problem hiding this comment.
This API is only consumed by "bin/wmagent-resource-control", which has been updated accordingly.
| @@ -364,8 +364,9 @@ def addMCFakeFile(self): | |||
| siteInfo = self.getLocationInfo.execute(conn=self.getDBConn(), | |||
| transaction=self.existingTransaction()) | |||
| for site in siteInfo: | |||
| if site['pnn'] in self.commonLocation: | |||
| locations.add(site['pnn']) | |||
| for pnn in site['pnn']: | |||
There was a problem hiding this comment.
Couldn't that be achieved with set operations, instead of additional for cycle?
| @@ -91,6 +96,13 @@ def algorithm(self, parameters): | |||
| logging.info("This component is not enabled in the configuration. Doing nothing.") | |||
| return | |||
|
|
|||
| logging.info("Checking and updating PSN x PNN map...") | |||
There was a problem hiding this comment.
I think this x here in this log message is confusing. Maybe we can use PSN to PNN map instead.
|
@todor-ivanov @vkuznet I think I addressed all of your concerns in the last 2 commits (to be properly squashed before merging it). Please have another look. |
|
Jenkins results:
|
|
Jenkins results:
|
|
And the last commit fixes an actual bug that I had in the code. It has now been tested in vocms0193 and it seems to be working fine. |
| @@ -364,8 +364,7 @@ def addMCFakeFile(self): | |||
| siteInfo = self.getLocationInfo.execute(conn=self.getDBConn(), | |||
| transaction=self.existingTransaction()) | |||
| for site in siteInfo: | |||
| if site['pnn'] in self.commonLocation: | |||
| locations.add(site['pnn']) | |||
| locations.update(set(site['pnn']) & set(self.commonLocation)) | |||
There was a problem hiding this comment.
Way better... Thanks @amaltaro
append new item Correct list of pnns ResourceControlUpdater can now update PNN/PSN association Deal with list of PNNs in WMBSHelper Mock CRIIC in ResourceControlUpdater Apply Todor/Valentin suggestions on ResourceControlUpdater change set operation in WMBSHelper Apply Todor/Valentin suggestions on GetSiteInfo Fix ResourceControl API to listSiteInfo
fix WMBS/Locations unit test pylint fixes; warn replaced by warning
|
Jenkins results:
|
Fixes #11596
Status
ready
Description
This PR provides the following:
GetSiteInfoDAO which was broken when no site name was providedIs it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None