Set output datasets to VALID in DBS3 before announcing a standard workflow #10394
Conversation
|
Jenkins results:
|
|
@haozturk thanks for providing this new feature. I have one general comment/request though, could you please move the getter/setter methods to our DBS3 wrapper module: and use our wrapper instead of making direct calls with the |
|
Jenkins results:
|
|
@amaltaro should I move the setter function to https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/Services/DBS/DBSWriterObjects.py as it looks like that DBS3Reader module is intended to include only readonly functions? |
|
That DBSWriterObjects.py module has just a set of functions, not really a class implemented. Given that DBS3Reader.py already has a couple of write functions, it would be fine to add it there as well. However, if we really want to to the right thing, then I'd suggest creating a DBS3Writer.py function that inherits from DBS3Reader, something like this: |
|
Okay, I create a |
|
Jenkins results:
|
|
@amaltaro I've moved the DBS API calls to their dedicated classes with 2 new unit tests. Looks like there are problems with two unit tests: I pass all unit tests on my testing environment, can you check what's wrong with them, please? Regarding the changes within MSOutput, as I mentioned earlier, I could not test them due to lack of unit test setup for MSOutput. Those changes requires testing in some way. |
|
test this please |
|
Jenkins results:
|
|
@haozturk thanks for providing these changes Hasan. I'm going to review this PR soon, but just wanted to point you to the unit test failure logs: as you can see, the jenkins bot certificate has no privileges to write to DBS, which makes sense. So we should probably try to mock that write call as well. |
|
Ah, thanks for pointing out that Alan, that makes sense. Let me revise that unit test. |
There was a problem hiding this comment.
@haozturk Hasan, I made a non-exhaustive review of this PR, and I would suggest you to look at those such that we can have another clear review once those changes are in place. I hope that's fine with you.
Just a note/concern: this PR will break the MSOutputTemplate schema, so we need to be careful and make these changes backward compatible with the documents already in MongoDB.
@todor-ivanov might have other comments/requests/concerns as well.
| msg += "It needs to be of type: MSOutputTemplate" | ||
| raise UnsupportedError(msg) | ||
|
|
||
| dbs3Writer = DBS3Writer(url=self.msConfig["dbsReadUrl"], |
There was a problem hiding this comment.
I would suggest to initialize it in the __init__ method instead.
| dbsUpdateStatus = "done" | ||
| for dMap in workflow['OutputMap']: | ||
|
|
||
| if self.msConfig['enableDbsStatusChange']: |
There was a problem hiding this comment.
I guess I would remove this option (and the dry-run mode). When we have it implemented, I believe there is no reason to disable this feature in MSOutput, right?
There was a problem hiding this comment.
I put it to make the switch from Unified to WMCore easy. So, once the PR is good to merge, we can merge it and put it to production with enableDbsStatusChange False and simply it will not take any action but to print some logs. Once we're ready to switch from Unified to WMCore, we can simply change this parameter to True without touching the source code. Does it make sense?
There was a problem hiding this comment.
I think we could enable this DBS status change in MSOutput and keep both this service and Unified updating the DBS status for a day or two (I'm almost sure it's harmless and nothing fails). Then we have time to stop Unified.
At this stage, dry-run mode is just an over-complication of the code IMO.
There was a problem hiding this comment.
That makes sense to me. This option is removed in the latest commit.
| @@ -102,6 +102,7 @@ def docSchema(self): | |||
| 'Copies': 1, | |||
| ...}], | |||
| "TransferStatus": "pending"|"done, | |||
| "DBSUpdateStatus": "pending"|"done, | |||
There was a problem hiding this comment.
I guess I would make it a boolean, thus something like:
"DBSStatusUpdated": False|True,
but then it would require more changes from your side. So I leave it up to you.
There was a problem hiding this comment.
Good suggestion, this is applied in the last commit.
| :return: DBS status of the given dataset | ||
| """ | ||
|
|
||
| allowedDbsStatuses = ["VALID", "INVALID", "PRODUCTION"] |
There was a problem hiding this comment.
a dataset can have other statuses. If I'm not wrong, DELETED and DEPRECATED as well. However, I don't think we need to have a list of valid statuses, we just use whatever is provided by DBS.
There was a problem hiding this comment.
I was also unsure whether it's necessary to put this extra check and honestly I did not have a strong reason to do so. So, it's okay to remove it to reduce the level of complexity.
There was a problem hiding this comment.
As agreed, this extra check is also removed.
|
|
||
| try: | ||
| self.dbs.updateDatasetType(dataset=dataset, | ||
| dataset_access_type=status) |
There was a problem hiding this comment.
indentation needs fixing. I would suggest to remove the blank line at 44 as well.
There was a problem hiding this comment.
BTW, we should fetch the output of this call and log the error in case it happens; or return it to the caller module.
By parsing the output of that call, we can also get rid of of the getDBSStatus implementation in the lines below.
There was a problem hiding this comment.
I also thought that but the function returns None if the call is successful and throws an exception o/w. I would prefer it to return True or False respectively. Since it does not do that, I handled it with getDBSStatus calls to make it more intuitive (according to me :) ) . We can also handle it:
- Return True if
updateDatasetTypereturns None - Return False o/w
Which one do you prefer?
Btw, we already log the error in the except clause, in case the API fails
There was a problem hiding this comment.
Hmm, too bad it returns None in case the call is successful..
Return True if updateDatasetType returns None
Return False o/w
This is my preference. Given that - if there is an exception - the error would likely be on the server side, I would suggest to change the log level from exception to error, such that we do not pollute the service logs with unnecessary information.
There was a problem hiding this comment.
I applied your suggestions Alan, but setDBSStatus still returns True if the API call is successful, False otherwise. This looks quite intuitive to me. Do you agree?
|
|
||
| if self.msConfig['enableDbsStatusChange']: | ||
|
|
||
| res = dbs3Writer.setDBSStatus(dataset=dMap["Dataset"], |
There was a problem hiding this comment.
Note that my comments on the DBS3Writer module will impact this logic.
Another comment, I failed to see where this self.msConfig['dbsStatus'] gets set in this module. Perhaps I would suggest to define an object variable like:
self.dbsOutputStatus = "VALID"
and simply refer to it here.
There was a problem hiding this comment.
I also made it configurable: https://github.com/dmwm/deployment/pull/1044/files#diff-3a773e3fbfb5ff69f5fd806e7a4ab54a7dfaaf1f080e7e96ac1df9e73589c2ffR115 It's also possible to set it within the module. Which one do you prefer?
There was a problem hiding this comment.
Oh, I missed the deployment changes. Thank for pointing it out.
I think it's fine to keep it in the service configuration file. Nonetheless, I would set a default value to "VALID" in this module, in case the configuration does not have it.
There was a problem hiding this comment.
Since deployment has the value, I did not set a default value in this module to avoid duplicates. If you think, it's more suitable to keep the configuration in MSOutput module instead of deployment repository, that's also fine, I can change it if you'd like.
|
Jenkins results:
|
|
Jenkins results:
|
There was a problem hiding this comment.
Hasan, these changes are looking good. However, I have made some comments along the code. I have a suggestion for this DBS status related schema, which I believe to be a simplified version of what is currently implemented.
I wonder why you changed the DBSMockData.json as well? Did we have the wrong parentage information in there? That information is fed directly from DBS, so I would be surprised if we had such issue. In addition to that, that json file has been updated this week, so you might want to rebase your PR to get the latest changes.
Thank you very much for providing these changes, Hasan. And apologies for the delay.
| 'OutputMap']), | ||
| Functor(self.docDump, pipeLine='PipelineNonRelVal'), | ||
| Functor(self.docCleaner)]) | ||
|
|
||
| wfCounterTotal = 0 | ||
| mQueryDict = {'TransferStatus': 'pending'} | ||
| mQueryDict = { "$or": [ { "TransferStatus": "pending" }, { "DBSUpdateStatus": False } ] } |
There was a problem hiding this comment.
TransferStatus is meant to consider the overall status of a given workflow in MSOutput. In other words, TransferStatus will only be marked as done once ALL the necessary actions have already been taken and the workflow is ready to move to the next status (and move away of the MSOutput watch).
So I'd suggest to keep the original query, and make sure that its status remains pending as long as there are still actions to be taken on it.
| status=self.msConfig['dbsStatus']["valid"]) | ||
|
|
||
| if res: | ||
| dMap["DBSStatus"] = self.msConfig['dbsStatus']["valid"] |
There was a problem hiding this comment.
If I understood it right, you are defining a new property for the datasets dictionary to be stored in MongoDB, right?
In that case, I would suggest to drop it from the schema, the DBS status will be one and only one, for every single dataset. So, I'd keep only the top level key DBSUpdateStatus, which would contain the overall DBS status of the output datasets. What do you think?
There was a problem hiding this comment.
BTW, do you know what happens if you set a VALID dataset to VALID once again? I believe DBS won't complain, right?
In addition to that, should we take into consideration datasets that have not been produced in the workflow, thus not being available in the DBS database?
| for dMap in workflow['OutputMap']: | ||
|
|
||
| res = self.dbs3Writer.setDBSStatus(dataset=dMap["Dataset"], | ||
| status=self.msConfig['dbsStatus']["valid"]) |
There was a problem hiding this comment.
And here I would simply replace self.msConfig['dbsStatus']["valid"] by VALID.
| :return: DBS status of the given dataset | ||
| """ | ||
|
|
||
| response = None |
There was a problem hiding this comment.
| response = None | |
| dbsStatus = None |
| if response: | ||
| dbsStatus = response[0]['dataset_access_type'] | ||
| self.logger.info("%s is %s", dataset, dbsStatus) | ||
| return dbsStatus |
There was a problem hiding this comment.
and here we could replace this if/else block by something like:
if response:
dbsStatus = response[0]['dataset_access_type']
self.logger.info("%s is %s", dataset, dbsStatus)
return dbsStatus
| @@ -0,0 +1,53 @@ | |||
| #!/usr/bin/env python | |||
| """ | |||
| _DBSReader_ | |||
There was a problem hiding this comment.
Wrong copy/pasting? :-D You might just remove it as well.
|
Jenkins results:
|
|
Can one of the admins verify this patch? |
Fixes #9837
Status
DBS API calls are tested locally in a WMAgent env. (1.4.3.patch2), however the rest of the changes is not tested
Description
This PR includes the migration of one feature from Unified to WMCore: Setting output datasets to VALID in DBS3 before announcing a standard workflow.
This feature is implemented as a part of the MSOutput pipeline. In this PR
setDBSStatusis the first step of the pipeline (before making actual output data placement) and also note that this functionality works for both RelVal and non-RelVal workflows.Check dmwm/deployment#1044 for the configuration of this feature. Note that
enableDbsStatusChangeis False, right now. Because, we need to coordinate for disabling the feature in Unified and enabling in WMCore. Soon, I'll come up a PR to disable it in Unified.Is it backward compatible (if not, which system it affects?)
yes
Related PRs
Not for now - soon a related PR in https://github.com/CMSCompOps/WmAgentScripts will come.
External dependencies / deployment changes
DBS
@amaltaro @todor-ivanov As I mentioned, the feature is not tested properly, it would be great if you can help me testing it.