Allow setting connect string from SiteLocalConfig #31544

wddgit · 2020-09-22T19:44:27Z

PR description:

Add support for setting the connection string in PoolDBESSource based on the site local config XML file and the global tag. Here is an example of of how to modify the XML file:

<site-local-config>
<site name="DUMMY">
   ...
   <calib-data>
      ...
      <local-connect>
         <connectString prefix="anything1" suffix="anything2"/>
      </local-connect>
...

If a non-empty globaltag is in the PoolDBESSource configuration and <local-connect> is in the XML file as shown above, then the connection string will be overwritten by the result of concatenating the prefix plus the globaltag plus the suffix. The prefix and suffix are from the XML file and can be set to anything or nothing.

An exception will be thrown if <local-connect> and <frontier-connect> are both set in the same XML file.

It is also possible to override the prefix and suffix from the configuration of the SiteLocalConfigService. It is also possible to force overwriting of the connection string in the same way.

This is primarily intended for use at sites where the worker nodes do not have network connectivity and the data normally retrieved from a database must be put into a local file.

PR validation:

Extended SiteLocalConfig unit tests to cover these new variables.

This should not change any behavior or results until a site local config XML file modified or the override configuration parameters are used. Existing tests of PoolDBESSource verify that.

We have not yet tested this on remote worker nodes that do not have network connectivity. The intent is to do that testing after the PR is merged and if necessary make further changes.

cmsbuild · 2020-09-22T19:44:53Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-22T19:49:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31544/18553

This PR adds an extra 32KB to repository

cmsbuild · 2020-09-22T19:50:12Z

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

CondCore/ESSources
FWCore/Catalog
FWCore/Services

@smuzaffar, @Dr15Jones, @makortel, @christopheralanwest, @tocheng, @cmsbuild, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks.
@makortel, @mmusich, @tocheng this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

wddgit · 2020-09-22T19:50:59Z

please test

FYI @hernand

cmsbuild · 2020-09-22T19:51:19Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-22-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9506/console Started: 2020/09/22 21:53

cmsbuild · 2020-09-22T21:14:58Z

+1
Tested at: 529704b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-53dcb8/9506/summary.html
CMSSW: CMSSW_11_2_X_2020-09-22-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-22T21:15:01Z

Comparison job queued.

makortel · 2020-09-22T22:11:07Z

CondCore/ESSources/plugins/CondDBESSource.cc

+      if (siteLocalConfig.isAvailable()) {
+        std::string const& localConnectPrefix = siteLocalConfig->localConnectPrefix();
+        std::string const& localConnectSuffix = siteLocalConfig->localConnectSuffix();
+        if (!localConnectPrefix.empty() && !localConnectSuffix.empty()) {


Should the suffix be allowed to be empty?

How about SiteLocalConfig would have a function telling if the local-config was specified or not, and then using the prefix and suffix without further checks here?

This seems reasonable. I will change the code so that if is present in the XML, then the connection string will be changed to prefix + globaltag + suffix, even if the prefix and suffix are empty or not specified in the XML. I will push a commit with this change soon.

makortel · 2020-09-22T22:15:27Z

FWCore/Services/src/SiteLocalConfigService.cc

+                m_localConnectPrefix = safe(connectString->Attribute("prefix"));
+                m_localConnectSuffix = safe(connectString->Attribute("suffix"));
+              }
+            }


I would make the <frontier-connect> and <local-connect> elements mutually exclusive.

I will make the change such that the SiteLocalConfigService constructor will throw an exception if both <frontier-connect> and <local-connect> are present. This seems reasonable to me.

This means someone creating a new XML file for one of these sites will have to both delete the <frontier-connect> section and add the <local-connect>. They will not be able to be lazy and just add <local-connect> and the other one then gets ignored. It is more work to edit the XML, but should be easier for someone to understand what is happening when reading the XML file.

cmsbuild · 2020-09-22T22:47:10Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-53dcb8/9506/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2540471
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2540442
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

davidlange6 · 2020-10-01T12:21:05Z

On Oct 1, 2020, at 2:09 PM, Marco Musich ***@***.***> wrote: @davidlange6 Nah - we do this routinely in all productions #27393 If I am not mistaken I said exactly that , quoting myself: A part from the AlCa/DB team actively chasing overrides of the production location for retrieving conditions (issue #27393), so once that's done normally all the conditions needed to run a production workflow should come from same GlobalTag ESSource, there are few places in which the GlobalTag overide is done on purpose by means of so-called "symoblic Global Tags" I assume someone is following up on that issue. Once that's solved the other point will still be there.

Indeed, if thats true, thats great - ideas on the timescale? I looks like all the originally reported ones are still there.

ggovi · 2020-10-06T07:47:41Z

My impression is that the proposal of this PR has not been fully understood here. There are two issues that are ( in my opinion ) well separated:

The 'custom' overwriting of the data source for ( potentially ) multiple condition records, with the toGet statements. In production workflow this is dangerous, it has been said that it should be not allowed. This is the issue mentioned by Chris. Last comment on my proposal has been issued by Fabio on August 2019. I think we should revisit the thing and start the implementation
The support of the processing of production workflow with NO db connection. Maybe this need has not been discussed enough in a public forum? IT looks to me that the reactions are questioning the use case itself. My opinion is that it can and it should be supported. And also, that the export of an entire GT into file(s), once the tools are well established, is an operation that should not require a 'validation'. The condition data set provided by a query in FronTier and the corresponding export in sqlite should be considered completely equivalent.

ggovi · 2020-10-06T07:50:16Z

@mmusich whatever cannot be fully traced and reproduced within a db query, for me falls into category one of the issues...

mmusich · 2020-10-06T08:02:28Z

@ggovi

whatever cannot be fully traced and reproduced within a db query, for me falls into category one of the issues...

I am not sure to follow you. The tags I am talking about are fully traced by the CMSSW configuration and reproduced by a DB queury, only they are not directly associated with any "physical" Global Tag.

If this use-case is not supported anymore (to which I don't really object), then please agree with the AlCa part of the project to support multiple Global Tag Queues for upgrade purposes. What is in place now is necessary as they did not agree to the extra burden of maintaining several different additional queues.

ggovi · 2020-10-08T13:50:10Z

@mmusich The key point is the user-case. In what respect this is a special case and in what differs from the normal GT-centralized treatment? An hard-coded tag ( if this is the case ) is pretty much equivalent to a toGet statement, and therefore should be avoided. But maybe I'm missing something?

ggovi · 2020-10-08T14:24:49Z

@mmusich Ok I see that your use case is driven by the intention to keep to a reasonable volume the amount of GT queues that AlCa need to support. Now I remind the discussions...
That means that we need to solve this issue before to be fully capable to support the productions with Conditions from file.
I'll investigate the option of the GT composition, that looks like could help.

ggovi · 2020-10-12T08:19:42Z

@wddgit @makortel @christopheralanwest @hernand @mmusich
I propose to have a (short?) discussion on the subject. We should focus on 1. use cases and 2. operations. 1 is essential to clarify the need to solve the issues pointed out by Marco M. and 2. is important to define the formats and the technologies ( sqlite/ one or more files?
What about Wednesday this week, at 16:00?

makortel · 2020-10-12T13:44:03Z

What about Wednesday this week, at 16:00?

This is week is generally bad because of the ongoing O&C week. Would next week work, or does the PPD workshop interfere? (if it does, how about two weeks from now?)

ggovi · 2020-10-13T18:52:04Z

Ok. I guess next week will be equally hard to find a slot for everybody. Let's move it tentatively for the week of 26 October.

ggovi · 2020-10-23T06:55:28Z

@wddgit @makortel @christopheralanwest @hernand @mmusich
I hope it is not too late to propose the discussion for next Monday, on the AlCaDB meeting time slot ( 16:00Geneva time ).
Please let us know if this could be acceptable, it would be good to have even only few slides on the point 1 and 2 above ( use cases and operation ).

hernand · 2020-10-23T07:22:11Z

Fine with me on Monday 16h. I can prepare a couple of slides with the use case

makortel · 2020-10-23T19:12:27Z

Monday at 16h works for me as well. Thanks @hernand for preparing slides.

ggovi · 2020-10-24T06:43:23Z

@makortel @hernand @wddgit @christopheralanwest @mmusich
Thanks a lot, i've booked the AlCaDB meeting accordingly:

https://indico.cern.ch/event/969102/

wddgit · 2020-10-27T16:06:19Z

Was the decision at the meeting to go ahead with this? Are we still waiting for more discussion? Was there something I needed to change?

ggovi · 2020-10-30T08:56:22Z

For what I am concerned, we can go ahead. In the future, we might need some adaptation if the single file GT is replaced by a folder structure.

ggovi · 2020-10-30T08:56:27Z

+1

christopheralanwest · 2020-10-30T14:18:04Z

+1

cmsbuild · 2020-10-30T14:18:28Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2020-10-31T01:14:49Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Sep 22, 2020

cmsbuild added alca-pending code-checks-pending comparison-pending core-pending db-pending orp-pending pending-signatures tests-pending labels Sep 22, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 22, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 22, 2020

wddgit mentioned this pull request Sep 22, 2020

Implementing fallback for reading conditions from file #29558

Closed

cmsbuild added tests-approved and removed tests-started labels Sep 22, 2020

makortel reviewed Sep 22, 2020

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Sep 22, 2020

Allow setting connect string from SiteLocalConfig

7134489

wddgit force-pushed the localConnectInSiteLocalConfig branch from 529704b to 7134489 Compare September 24, 2020 18:44

cmsbuild removed the code-checks-approved label Sep 24, 2020

cmsbuild added db-approved and removed db-pending labels Oct 30, 2020

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Oct 30, 2020

cmsbuild added orp-approved and removed orp-pending labels Oct 31, 2020

cmsbuild merged commit c06e922 into cms-sw:master Oct 31, 2020

cmsbuild mentioned this pull request Nov 4, 2020

Switch to Geant4 thermal neutron physics #32009

Merged

wddgit deleted the localConnectInSiteLocalConfig branch May 3, 2021 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting connect string from SiteLocalConfig #31544

Allow setting connect string from SiteLocalConfig #31544

wddgit commented Sep 22, 2020 •

edited

cmsbuild commented Sep 22, 2020

cmsbuild commented Sep 22, 2020

cmsbuild commented Sep 22, 2020

wddgit commented Sep 22, 2020

cmsbuild commented Sep 22, 2020 •

edited

cmsbuild commented Sep 22, 2020

cmsbuild commented Sep 22, 2020

makortel Sep 22, 2020

wddgit Sep 23, 2020

makortel Sep 22, 2020

wddgit Sep 23, 2020 •

edited

cmsbuild commented Sep 22, 2020

davidlange6 commented Oct 1, 2020 via email

ggovi commented Oct 6, 2020

ggovi commented Oct 6, 2020 •

edited

mmusich commented Oct 6, 2020

ggovi commented Oct 8, 2020

ggovi commented Oct 8, 2020

ggovi commented Oct 12, 2020

makortel commented Oct 12, 2020

ggovi commented Oct 13, 2020

ggovi commented Oct 23, 2020

hernand commented Oct 23, 2020

makortel commented Oct 23, 2020

ggovi commented Oct 24, 2020

wddgit commented Oct 27, 2020

ggovi commented Oct 30, 2020

ggovi commented Oct 30, 2020

christopheralanwest commented Oct 30, 2020

cmsbuild commented Oct 30, 2020

qliphy commented Oct 31, 2020

Allow setting connect string from SiteLocalConfig #31544

Allow setting connect string from SiteLocalConfig #31544

Conversation

wddgit commented Sep 22, 2020 • edited

PR description:

PR validation:

cmsbuild commented Sep 22, 2020

cmsbuild commented Sep 22, 2020

cmsbuild commented Sep 22, 2020

wddgit commented Sep 22, 2020

cmsbuild commented Sep 22, 2020 • edited

cmsbuild commented Sep 22, 2020

cmsbuild commented Sep 22, 2020

makortel Sep 22, 2020

Choose a reason for hiding this comment

wddgit Sep 23, 2020

Choose a reason for hiding this comment

makortel Sep 22, 2020

Choose a reason for hiding this comment

wddgit Sep 23, 2020 • edited

Choose a reason for hiding this comment

cmsbuild commented Sep 22, 2020

davidlange6 commented Oct 1, 2020 via email

ggovi commented Oct 6, 2020

ggovi commented Oct 6, 2020 • edited

mmusich commented Oct 6, 2020

ggovi commented Oct 8, 2020

ggovi commented Oct 8, 2020

ggovi commented Oct 12, 2020

makortel commented Oct 12, 2020

ggovi commented Oct 13, 2020

ggovi commented Oct 23, 2020

hernand commented Oct 23, 2020

makortel commented Oct 23, 2020

ggovi commented Oct 24, 2020

wddgit commented Oct 27, 2020

ggovi commented Oct 30, 2020

ggovi commented Oct 30, 2020

christopheralanwest commented Oct 30, 2020

cmsbuild commented Oct 30, 2020

qliphy commented Oct 31, 2020

wddgit commented Sep 22, 2020 •

edited

cmsbuild commented Sep 22, 2020 •

edited

wddgit Sep 23, 2020 •

edited

ggovi commented Oct 6, 2020 •

edited