Enforce dataset_lifetime column to be integer and not null #11115

amaltaro · 2022-04-25T21:54:34Z

Fixes #11108
Alternative to #11111

Status

ready

Description

This change will ensure that:

the database won't accept None as input for the dataset_lifetime column
that we properly convert None to 0 whenever needed, before creating a record in the database.

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

NO (database schema change)

Related PRs

If this proposal is acceptable and correct, we might want to merge this in master only; backport and merge #11111 to the 2.0.2_wmagent branch and create a new patch release.

External dependencies / deployment changes

None

cmsdmwmbot · 2022-04-25T22:07:44Z

Jenkins results:

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

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

jhonatanamado · 2022-04-26T02:12:18Z

Hi @amaltaro, Alan please consider the following changes in this PR in order to fix an issue in the deployment of a T0 agent during the creation of the oracle tables as can been seen below

DEBUG:root:Problem creating database table 

CREATE TABLE dbsbuffer_dataset_subscription (
                 id                 INTEGER      NOT NULL,
                 dataset_id         INTEGER      NOT NULL,
                 site               VARCHAR(100) NOT NULL,
                 custodial          INTEGER      DEFAULT 0,
                 auto_approve       INTEGER      DEFAULT 0,
                 move               INTEGER      DEFAULT 0,
                 priority           VARCHAR(10)  DEFAULT 'Low',
                 subscribed         INTEGER      DEFAULT 0,
                 phedex_group       VARCHAR(100),
                 delete_blocks      INTEGER,
                 dataset_lifetime   INTEGER      NOT NULL DEFAULT 0,
                 PRIMARY KEY (id),
                 CONSTRAINT uq_dbs_dat_sub UNIQUE (dataset_id, site, custodial, auto_approve, move, priority)
               )

(cx_Oracle.DatabaseError) ORA-00907: missing right parenthesis
[SQL: CREATE TABLE dbsbuffer_dataset_subscription (
                 id                 INTEGER      NOT NULL,
                 dataset_id         INTEGER      NOT NULL,
                 site               VARCHAR(100) NOT NULL,
                 custodial          INTEGER      DEFAULT 0,
                 auto_approve       INTEGER      DEFAULT 0,
                 move               INTEGER      DEFAULT 0,
                 priority           VARCHAR(10)  DEFAULT 'Low',
                 subscribed         INTEGER      DEFAULT 0,
                 phedex_group       VARCHAR(100),
                 delete_blocks      INTEGER,
                 dataset_lifetime   INTEGER      NOT NULL DEFAULT 0,
                 PRIMARY KEY (id),
                 CONSTRAINT uq_dbs_dat_sub UNIQUE (dataset_id, site, custodial, auto_approve, move, priority)
               )]

I deployed a replay with WMCore 2.0.2.patch2 + PR11115 + Commit63d99e and the replay finished without issues. I checked the container rules created by T0 for subscriptions to disk and the rules have the proper lifetime.

vkuznet · 2022-04-26T11:39:47Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

-                          'phedex_group': phedex_group,
-                          'delete_blocks': delete_blocks,
-                          'dataset_lifetime': subscriptionInfo['DatasetLifetime']})
+            bind = {'id': datasetID,


I think it would be more protective if you explicitly cast data-types for each binding value, e.g.

bind = {'id': int(datasetID), 'site': str(site), ...}

Doing this way you'll ensure that proper data-type is passed to bind dictionary which you later pass to ORACLE DB

I think this is a good suggestion indeed.

I'm updating this code with a few more safe checks for None. But these suggestions might actually cause more problems than resolve them. For instance, casting None to Integer will crash, casting a byte string to string will make things even worse.

In addition to that, dataset_id is already protected by the database schema and this code is in use for many years. Different than dataset_lifetime, which is a new feature.

The way how you implemented those additional checks do look sound to me. Thanks @amaltaro

todor-ivanov

Thanks for fixing in it the proper way @amaltaro. As of the current implementation I basically did second the previous two comments. But I read the PR status as not-tested so I believe you would have those fixed during testing anyway.

todor-ivanov · 2022-04-26T17:24:31Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

-                          'phedex_group': phedex_group,
-                          'delete_blocks': delete_blocks,
-                          'dataset_lifetime': subscriptionInfo['DatasetLifetime']})
+            bind = {'id': datasetID,


I think this is a good suggestion indeed.

todor-ivanov · 2022-04-26T17:26:00Z

src/python/WMComponent/DBS3Buffer/Oracle/Create.py

@@ -49,7 +49,7 @@ def __init__(self, logger = None, dbi = None, params = None):
                 subscribed         INTEGER      DEFAULT 0,
                 phedex_group       VARCHAR(100),
                 delete_blocks      INTEGER,
-                 dataset_lifetime   INTEGER     DEFAULT 0,
+                 dataset_lifetime   INTEGER      NOT NULL DEFAULT 0,


@amaltaro I think @jhonatanamado's comment needs to be addressed here too.

amaltaro · 2022-04-26T22:57:09Z

@jhonatanamado thank you very much for testing this in a replay! I fixed the order of those constraints in my second commit. It also has a fix further data type checks, as requested by Valentin and Todor. Please have another look at it.

cmsdmwmbot · 2022-04-26T23:06:59Z

Jenkins results:

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

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

amaltaro · 2022-04-27T01:39:51Z

I confirm that Oracle-based Jenkins tests are happier now, while the previous order of constraints was causing 92 new tests to fail.

todor-ivanov

Thanks for the follwo up @amaltaro !
The code looks good.

todor-ivanov · 2022-04-27T07:09:42Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

-                          'phedex_group': phedex_group,
-                          'delete_blocks': delete_blocks,
-                          'dataset_lifetime': subscriptionInfo['DatasetLifetime']})
+            bind = {'id': datasetID,


The way how you implemented those additional checks do look sound to me. Thanks @amaltaro

vkuznet

If you want to fix it right, please add validation to all passed values, I provided concrete examples to each individual columns.

vkuznet · 2022-04-27T12:14:32Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

-                          'delete_blocks': delete_blocks,
-                          'dataset_lifetime': subscriptionInfo['DatasetLifetime']})
+            bind = {'id': datasetID,
+                    'site': site,


can you protect site to satisfy to correct regexp. The schema only require it to be not null, so someone can pass "ABC" and it will be fine, but this is not what our sites are. Please put protection to validate this input value.

This is already validated upstream, at the request spec level, e.g.:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/StdBase.py#L1135

vkuznet · 2022-04-27T12:16:14Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

-                          'dataset_lifetime': subscriptionInfo['DatasetLifetime']})
+            bind = {'id': datasetID,
+                    'site': site,
+                    'custodial': 0 if custodialFlag is None else custodialFlag,


custodialFlag here can be passed as -1 , it is still and integer but I doubt it is correct value, please validate it appropriately to fall to a specific range, e.g. be only 1 or 0.

It looks like the custodialFlag is actually either True or False, as defined in this very same code. Unsure how was/is getting casted to integer though...

vkuznet · 2022-04-27T12:16:54Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

+                    'site': site,
+                    'custodial': 0 if custodialFlag is None else custodialFlag,
+                    'auto_approve': 1 if site in subscriptionInfo['AutoApproveSites'] else 0,
+                    'move': 0 if isMove is None else isMove,


isMove requires validation too, someone can pass -1 and code will not spot it.

I'm actually removing this check on isMove because it's already validated a few lines above.

vkuznet · 2022-04-27T12:17:49Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

+                    'custodial': 0 if custodialFlag is None else custodialFlag,
+                    'auto_approve': 1 if site in subscriptionInfo['AutoApproveSites'] else 0,
+                    'move': 0 if isMove is None else isMove,
+                    'priority': subscriptionInfo['Priority'],


can you make proper default here, e.g. subscriptionInfo.get('Priority', "some-default-value"). And, the value should be validated too to satisfy allowed values.

Request priority default comes from the upper spec file, here (and data type is also validated in there):
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/StdBase.py#L1025

Having default values defined in multiple places is just source for a future error.

vkuznet · 2022-04-27T12:18:01Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

+                    'auto_approve': 1 if site in subscriptionInfo['AutoApproveSites'] else 0,
+                    'move': 0 if isMove is None else isMove,
+                    'priority': subscriptionInfo['Priority'],
+                    'phedex_group': phedex_group,


I don't know if you really need it anymore.

You are right, it's no longer relevant. Actually, a good chunk of this is no longer needed and we need to re-purpose it according to the Rucio convention. To be addressed here: #9639

vkuznet · 2022-04-27T12:18:16Z

src/python/WMComponent/DBS3Buffer/MySQL/NewSubscription.py

+                    'move': 0 if isMove is None else isMove,
+                    'priority': subscriptionInfo['Priority'],
+                    'phedex_group': phedex_group,
+                    'delete_blocks': delete_blocks}


requires validation too.

Already covered a few lines above and in accordance with the database schema. I could be wrong, but AFAIK it's more performant to have a column with None/null than with an integer 0. So I'm in favor of actually not changing this 1 or None.

amaltaro · 2022-04-27T18:00:28Z

@vkuznet in addition to the follow up comments I made along the code, I also pushed another commit with further changes. I will squash it once review is over. Please have another look at your convenience, but keep in mind that much of this actually comes from upstream - request level - and most of that is already validated and data type is checked as well.

I'd like to avoid adding multiple layers of data validation whenever possible.

cmsdmwmbot · 2022-04-27T18:08:47Z

Jenkins results:

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

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

vkuznet

Since you mentioned several times that some values are validated upstream please put appropriate comment to the code about these checks. It would be useful to know why some values you check and others do not.

amaltaro · 2022-04-27T21:51:22Z

Since you mentioned several times that some values are validated upstream please put appropriate comment to the code about these checks. It would be useful to know why some values you check and others do not.

I understand your request, but bear in mind that EVERY single workflow parameter is actually validated by WMSpec/StdSpecs specs. If we do so, then we should do it for 100s of files that are using/setting workflow related parameters.

Instead, just give it some time and you will get to know what those parameters are, but in short everything is defined in this super class:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/StdSpecs/StdBase.py#L968

amaltaro · 2022-04-27T21:54:10Z

BTW, these latest changes have also been tested with oracle jenkins.

ensure move and custodial are not None; fix order of db constraints address some of Valentins concerns add docstring and extra comment for the param/validation

amaltaro · 2022-05-02T18:33:43Z

@vkuznet Valentin, I just added a docstring string and further comments on the parameters/validation. However, as stated above, every workflow-related information that goes to the relational database is validated beforehand and IMO adding such comments to 1 DAO out of hundreds won't be of much help. Please have another look at your convenience.

cmsdmwmbot · 2022-05-02T19:05:31Z

Jenkins results:

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

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

amaltaro · 2022-05-02T19:06:58Z

Thank you Valentin and Todor. Given that it doesn't change anything in the central services, I'm merging it now.

amaltaro requested review from todor-ivanov and vkuznet April 25, 2022 21:56

vkuznet requested changes Apr 26, 2022

View reviewed changes

todor-ivanov reviewed Apr 26, 2022

View reviewed changes

amaltaro requested review from vkuznet and todor-ivanov April 26, 2022 22:57

todor-ivanov approved these changes Apr 27, 2022

View reviewed changes

vkuznet requested changes Apr 27, 2022

View reviewed changes

amaltaro added the PR: squashing needed label Apr 27, 2022

amaltaro requested a review from vkuznet April 27, 2022 17:58

vkuznet reviewed Apr 27, 2022

View reviewed changes

Enforce dataset_lifetime column to be integer and not null

356a00c

ensure move and custodial are not None; fix order of db constraints address some of Valentins concerns add docstring and extra comment for the param/validation

amaltaro force-pushed the fix-11108 branch from b942bed to 356a00c Compare May 2, 2022 18:31

amaltaro removed the PR: squashing needed label May 2, 2022

amaltaro requested a review from vkuznet May 2, 2022 18:33

vkuznet approved these changes May 2, 2022

View reviewed changes

amaltaro merged commit daea165 into dmwm:master May 2, 2022

amaltaro added the PR: DB schema change label May 2, 2022

This was referenced Jun 10, 2022

Not all binds created for DBS3Buffer NewSubscription DAO #11178

Closed

Ensure dataset_lifetime bind is defined for DBS3Buffer/NewSubscription #11179

Merged

Enforce dataset_lifetime column to be integer and not null #11115

Enforce dataset_lifetime column to be integer and not null #11115

Conversation

amaltaro commented Apr 25, 2022 • edited Loading

Status

Description

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

Related PRs

External dependencies / deployment changes

cmsdmwmbot commented Apr 25, 2022

jhonatanamado commented Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todor-ivanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaltaro commented Apr 26, 2022

cmsdmwmbot commented Apr 26, 2022

amaltaro commented Apr 27, 2022

todor-ivanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkuznet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaltaro commented Apr 27, 2022

cmsdmwmbot commented Apr 27, 2022

vkuznet left a comment

Choose a reason for hiding this comment

amaltaro commented Apr 27, 2022

amaltaro commented Apr 27, 2022

amaltaro commented May 2, 2022

cmsdmwmbot commented May 2, 2022

amaltaro commented May 2, 2022

amaltaro commented Apr 25, 2022 •

edited

Loading

jhonatanamado commented Apr 26, 2022 •

edited

Loading