Run unit and validation test against Py based server

[yuyiguo]

@vkuznet
I merged PR #22 . Can you start to run DBS unit and validation tests against the python and Go servers using branch DBSClient4go? I knew you already ran unit tests before, if you haven't run validation tests, here is the command to run it:

python setup_test.py test_system --host=https://cmsweb-testbed.cern.ch --validation

[vkuznet]

@yuyiguo I can't use directly setup_test.py since it does not work with arbitrary URL. The logic inside of it assumes that DBS has instances and provided host name is concatenated with specific (hard-coded) instance to make DBS_READER_URL, and other URLs. Therefore, if we deploy dbs2go (or any other) DBS server on non-standard URL, e.g. https://cmsweb-testbed.cern.ch/dbs2go the current logic of setup_test.py will not work since there is no instance in deployed URL.

Do you want me to provide a PR to fix this problem in this repo? I already made dmwm/DBS#646 about it, but it was not merged and not propagated to this repository. My suggestion is to adjust setup_test.py to provide both full DBS_READER_URL and DBS_WRITER_URL (and DBS_MIGRATE_URL). Each URL should have full path to specific DBS server, e.g.

# dbs2go test server
--reader=htttps://cmsweb-testbed.cern.ch/dbs2go-reader --writer=https://cmsweb-testbed.cern.ch/dbs2go-writer

# standard DBS python server on testbed
--reader=htttps://cmsweb-testbed.cern.ch/dbs/int/global/DBSReader --writer=https://cmsweb-testbed.cern.ch/dbs/int/global/DBSWriter

Such options will allow to test ANY combinations of DBS servers which can be deploy on any cluster in any end-point.

[vkuznet]

@yuyiguo , moreover I found that since you did not apply dmwm/DBS#657 PR the DBSValidation_t.py also will not run out of the box. It inherits the same set of problems with invalid data-types, and incorrect values for DBS attributes. Therefore, in order to proceed we should fix DBSValidation_t.py too in a similar fashion I fixed DBSClientWriter_t.py (in dmwm/DBS#657). Do you want separate PRs for that within this repo?

[vkuznet]

Here PR for setup_test.py: #26

[vkuznet]

Here PR for DBSValidation_t.py: #27

[vkuznet]

Here PR for DBSClientWriter_t.py: #28

[yuyiguo]

@vkuznet
DBS server has formatted url. We should use the same format for both python and go based servers while we migrate to go server. I hope you agree with this.

For the testing, we can have different url formatting just for the convenience.

[vkuznet]

@yuyiguo I doubt it is feasible. On cmsweb-testbed the DBS Python URLs are fixed to /dbs/int/global/DBSReader and /dbs/int/global/DBSWriter, and I can't deploy to these URLs dbs2go if I want to keep them for Python server. Therefore I should use a different URL for dbs2go in order to make tests against both servers. I can deploy dbs2go on another k8s cluster with these end-points, but in this case it would be different hostname. Therefore, I propose the changes to use arbitrary URL to allow us to perform tests against both servers. If you know any other way around this please tell me.

[vkuznet]

In other words, I do agree that when we'll migrate Go server we will preserve the DBS URLs, but if we want to compare results from Python server to Go server and ensure that DBSClient works with both servers we either need to use different URLs, or deploy to the same URLs/end-points different servers. Operationally, I prefer to deploy both servers on the same cluster and use different URLs which will allow testing/validation done quickly, e.g. we can get results from one server using one set of URLs and results from another server using different set of URLs (but both servers will talk to the same DB). This is why I propose changes to support arbitrary URLs.

[yuyiguo]

@vkuznet
For testing it is ok to have different url formats as mentioned earlier in this issue, but for production after migration, we should have the same url for the go server as the python one. So the DBS clients don't have to change their code for the new url. I hope we will not need go back to python server for comparison after the migration. All these comparisons should be done before the migration. In case, we need to start the python server for whatever the reason after migrating to go, we should use a different name for the python server, but keeping the current url schema to the go server.

Does this make sense?

[vkuznet]

@yuyiguo I do agree that official DBS URLs should stay the same during and after migration, but I disagree that we can't support arbitrary URLs. The former are designed to keep production/testbed access without interruption, but for development I can deploy DBS server anywhere including my laptop and I should be able to use DBS Client to test it. It does not make any sense to me that for latter case I always should manually change DBS client code since by default it only access production/testbed URLs. Let separate the production/testbed access from dev cycle. For instance, dbs2go fully support SQLite DB, such that we cam add CI/CD test to test DBSClient with dbs2go deployed at local machine. For that we only need to support local URL. If we stick to production/testbed URL the testing become locked to those specific URLs.

[yuyiguo]

@vkuznet

You misunderstood my points. All I said that we should keep the urls to follow the same schema as it is now after migration.
I want to make sure we are in the same page on this.

As I said in the beginning, for testing it is ok with different url schemas.

[yuyiguo]

@vkuznet

PRs were merged. Please testing for both python and go server.

Let me know if you need anything else for the testing.

[vkuznet]

@yuyiguo I start testing and identified additional missing part, please review and merge this PR: #29

[yuyiguo]

Done

[vkuznet]

@yuyiguo I continue running validation tests and found couple of issues both in Client and my server. I provided fix for DBSClient in this PR: #31 It basically fixes aggregation function call and add debug option which I used to identify the issue.

[vkuznet]

@yuyiguo I provided an additional PR #32 about missing aggregator for listFileParentsByLumi DBS API. The PR contains proper unit test too.

[vkuznet]

@yuyiguo I found another missing aggregation for parentDSTrio DBS API. Please take associative PR #33 which includes relevant unit test.

[vkuznet]

@yuyiguo finally I converged again that Reader and Writer tests are fine (except the dbs_version regexp match as dbs2go uses different versions schema), and I tried validation tests. But I immediately found that in order to do it few things should be fixed in this tests. Before providing concrete PRs I would like to understand the logic of validation test.

So here is my findings:

• we should decide how to fix or remove dbs_version tests (https://github.com/dmwm/DBSClient/blob/main/tests/dbsclient_t/unittests/DBSClientReader_t.py#L1482-L1487) So far it only matches versions of DBS python server and as I mentioned already in dbs2go I used different version schema. Do you want me to provide PR for it? Otherwise, I'll either not paying attention to this failing test or suggest to remove it. I found that it exists in DBSValidation_t too, see test15 here https://github.com/dmwm/DBSClient/blob/main/tests/dbsclient_t/validation/DBSValidation_t.py#L575
• In DBSValidation_t you use four different URLs:
  • DBS_WRITER_URL, DBS_MIGRATE_URL, source_url which points to production DBS instance, and hard-coded cmswebtestbed_api which uses cmsweb-testbed int DBS global instance. As such, I need to properly fix the logic if we'll supply custom DBS_READER_URL, DBS_WRITER_URL. Please describe in English what DBSValitaion_t test is doing. My understanding that it migrate data from source_url to DBS_WRITER_URL, but then look-up data in testbed URL. As such I propose to replace
  • cmswebtestbed_api by DBS_READER_URL which will be used to data look-up
  • use env variable for source_url instead of hard-coded DBS prod
  • the data look-up should be read from DBS_READER_URL
  • while data injection should go to DBS_WRITER_URL
• in Python server you use the same set of APIs both in DBSReader and DBSWriter. In dbs2go I separate them such that I can have only read APIs in reader instance and write APIs in writer instance. This make more secure DBS instance, e.g. clients can't use writer DBS instance for data-lookup and therefore can't abuse it with these calls. But my code is flexible enough that I can restore Python server behavior and use the same set of APIs in both reader and writer instances. Please let me know what is your opinion on that and I can make appropriate changes to DBSClient, i.e. all listXXX calls should use reader DBS instance, while all injectXXX calls should use only DBS writer.

After we fix above issues I still need some time to work on DBS Migration server which is not yet finished, and then I can proceed with DBSValidation_t tests.

[yuyiguo]

@vkuznet
Let me reply you item by item:
we should decide how to fix or remove dbs_version tests (https://github.com/dmwm/DBSClient/blob/main/tests/dbsclient_t/unittests/DBSClientReader_t.py#L1482-L1487) So far it only matches versions of DBS python server and as I mentioned already in dbs2go I used different version schema. Do you want me to provide PR for it? Otherwise, I'll either not paying attention to this failing test or suggest to remove it. I found that it exists in DBSValidation_t too, see test15 here https://github.com/dmwm/DBSClient/blob/main/tests/dbsclient_t/validation/DBSValidation_t.py#L575

I think we should keep this test until DBS migrated to to go server. But I don't think you need to make this test for working for go. You may want to have a new version test after the migration.

In DBSValidation_t you use four different URLs

The purpose of DBSValidation test is to verify if DBS server can write, read and migration data correctly. In order to do it, we need to write to server, read it back, then compare the data to be written and read back. We do the same for migration too. DBS validation test run against any DBS python server. My development process is to code locally, test on my VM, then test on cmsweb-testbed.

in Python server you use the same set of APIs both in DBSReader and DBSWriter. In dbs2go I separate them such that I can have only read APIs in reader instance and write APIs in writer instance. This make more secure DBS instance, e.g. clients can't use writer DBS instance for data-lookup and therefore can't abuse it with these calls.

I don't think your approach make DBS server any more secure, but make client life difficult. DBS read server can only read, but the write server can do both read and write. DBS read and write server has different roles/groups. The writers are limited a group of project and individuals.

[vkuznet]

@yuyiguo , thanks for explanation. Then, I'll proceed as following:

• I'll leave dbs_version test as is and will ignore it in my tests
• for DBSValiation_t.py
  • I'll put back reader APIs into DBS writer server it is not a big deal
  • please explain logic of test16 since it gets blocks from DBS global and checks if it can finds them in cmsweb-testbed int instance. This test relies on hard-coded cmsweb-testbed URL (cmsweb-testbed.cern.ch/dbs/int/global/DBSReader) and I need to know how to generalize it if DBS reader is not at that specific UR (dbs2go server uses so far different URL). This test uses random datasets (and not migrated ones) and therefore can fail if content of DBS int instance is differ from DBS global one. Therefore, I would expect that code should get some datasets from a migration list and compare that their content is identical both in global DBS and DBS instance defined by DBS_MIGRATE_URL (which may not be the same as DBS int instance on cmsweb-testbed).

[vkuznet]

@yuyiguo please merge this PR #34 which adds debug option to all DbsApi calls in unit/validation tests.

[yuyiguo]

@vkuznet please check the comments I put ony PR#33. I will merge #33 and #34 together.

[yuyiguo]

@vkuznet
My memory on test 16 is kind of faded. If I recalled correctly, this test was put in by Shengchan. He asked a special API for finding the file parents by using Lumi info because the data processing could not get the parents while processing the data. The purpose of the test was to make sure that the parentage from the API/listFileParentsByLumi match what the real parentage. Maybe he thought only the data from prod considered "real".

[yuyiguo]

Done.