Handle invalid entries in <data-access> of site-local-config.xml #40961

nhduongvn · 2023-03-06T14:21:44Z

PR description:

In the previous implementation, when the first entry in <data-access> of site-local-config.xml contains invalid information such as protocol name an exception is thrown out from InputFileCatalog::init() and no further data catalogs are constructed from other entries in <data-access>. This pull request fixes this. All entries are tried and if no one is valid, the execution stops. Moreover, there are some improvements in exception messages for more readable (double quote added ...).

cmsbuild · 2023-03-06T14:28:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40961/34457

This PR adds an extra 16KB to repository

cmsbuild · 2023-03-06T14:28:36Z

A new Pull Request was created by @nhduongvn for master.

It involves the following packages:

FWCore/Catalog (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@makortel, @missirol, @wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

makortel · 2023-03-06T17:22:34Z

FWCore/Catalog/src/InputFileCatalog.cc

+            tmps[0],
+            tmps[1],
+            tmps[2],
+            tmps[3],
+            tmps[4]);  //current-site,current-subSite,desired-data-access-site,desired-data-access-volume,desired-data-access-protocol


How about reformatting along

Suggested change

tmps[0],

tmps[1],

tmps[2],

tmps[3],

tmps[4]); //current-site,current-subSite,desired-data-access-site,desired-data-access-volume,desired-data-access-protocol

tmps[0], //current-site

tmps[1], // current-subSite

tmps[2], // desired-data-access-site

tmps[3], // desired-data-access-volume

tmps[4]); // desired-data-access-protocol

? (using named constants instead of a number + comment would be even better)

makortel · 2023-03-06T17:22:57Z

FWCore/Catalog/src/InputFileCatalog.cc

        try {
          fileLocators_trivalCatalog_.push_back(std::make_unique<FileLocator>(*it));
        } catch (cms::Exception const& e) {
-          continue;
+          edm::LogWarning("InputFileCatalog")
+              << "Catch an exception while constructing a file locator in InputFileCatalog::init: " << e.what();


Suggested change

<< "Catch an exception while constructing a file locator in InputFileCatalog::init: " << e.what();

<< "Caught an exception while constructing a file locator in InputFileCatalog::init: " << e.what();

makortel · 2023-03-06T17:24:12Z

FWCore/Catalog/src/InputFileCatalog.cc

+          edm::LogWarning("InputFileCatalog")
+              << "Catch an exception while constructing a file locator in InputFileCatalog::init: " << e.what();
+          if (it != tmp_dataCatalogs.end() - 1) {
+            std::cout << "\n Skip this catalog" << std::endl;


Should this be printed always, i.e. be

Suggested change

std::cout << "\n Skip this catalog" << std::endl;

edm::LogWarning("InputFileCatalog") << "\n Skip this catalog";

or not, i.e. something along

Suggested change

std::cout << "\n Skip this catalog" << std::endl;

LogDebug("InputFileCatalog") << "\n Skip this catalog";

?

LogWarning is better since there is nothing wrong with the code but problem comes from invalid configuration in

makortel · 2023-03-06T17:41:21Z

FWCore/Catalog/src/InputFileCatalog.cc

+            cms::Exception ex("FileCatalog");
+            ex << "Can not construct a file locator in InputFileCatalog::init";
+            ex.addContext("Calling edm::InputFileCatalog::init()");
+            throw ex;


How about organizing the loop along

for (auto const& cat : tmp_dataCatalogs) { try { fileLocators_trivalCatalog_.push_back(std::make_unique<FileLocator>(cat)); } catch (cms::Exception const& e) { edm::LogWarning...; } } if (fileLocators_trivalCatalog_.empty()) { cms::Exception ex("FileCatalog"); ex << "Unable to construct any file locators in InputFileCatalog::init"; ex.addContext("Calling edm::InputFileCatalog::init()"); throw ex; }

?

Actually I think the currently this PR would throw if and only if the last catalog has errors, whereas my understanding of the intention would be to throw only if all the catalogs had errors.

Yes your understanding is correct: if all catalogs had errors the code should stop and throw an exception. This is done by continue statement

cmssw/FWCore/Catalog/src/InputFileCatalog.cc

Line 93 in f777646

continue;

I like your suggestion which makes things clearer. If all catalogs have errors fileLocators container is empty and the code throws an exception

cmsbuild · 2023-03-14T01:50:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40961/34613

This PR adds an extra 12KB to repository

cmsbuild · 2023-03-14T01:50:27Z

Pull request #40961 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again.

makortel · 2023-03-15T07:39:32Z

@cmsbuild, please test

makortel · 2023-03-15T09:15:46Z

FWCore/Catalog/src/InputFileCatalog.cc

-
-      for (auto it = tmp_dataCatalogs.begin() + 1; it != tmp_dataCatalogs.end(); ++it) {
+      //Construct all file locators from data catalogs. If a data catalog is invalid (wrong protocol for example), it is skipped and no file locator is constructed (an exception is thrown out from FileLocator::init).
+      for (auto it = tmp_dataCatalogs.begin(); it != tmp_dataCatalogs.end(); ++it) {


Now this loop (and the similar loop below) could become range-for along

Suggested change

for (auto it = tmp_dataCatalogs.begin(); it != tmp_dataCatalogs.end(); ++it) {

for (auto const& catalog : tmp_dataCatalogs) {

makortel · 2023-03-15T09:16:23Z

Could you also squash the commits into one, so that eventual backporting would be easier?

cmsbuild · 2023-03-15T09:59:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-981e16/31285/summary.html
COMMIT: 2621415
CMSSW: CMSSW_13_1_X_2023-03-14-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40961/31285/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 4 lines to the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3550756
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3550731
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

cmsbuild · 2023-03-15T14:34:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40961/34660

This PR adds an extra 16KB to repository

cmsbuild · 2023-03-15T14:34:43Z

Pull request #40961 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again.

makortel · 2023-03-15T14:34:44Z

@cmsbuild, please test

Thanks!

cmsbuild · 2023-03-15T16:59:50Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-981e16/31294/summary.html
COMMIT: da906d0
CMSSW: CMSSW_13_1_X_2023-03-14-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40961/31294/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 16 lines from the logs
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3550756
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3550725
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2023-03-15T17:19:34Z

+core

cmsbuild · 2023-03-15T17:19:59Z

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

makortel · 2023-03-15T17:20:02Z

This PR should then be backported to 13_0_X and 12_6_X, right? (tagging also @stlammel)

stlammel · 2023-03-15T17:42:34Z

Yes, please, this should go into the backport too. - Stephan

nhduongvn · 2023-03-15T18:22:20Z

Should I go ahead to prepare the pull requests for backporting or wait?

perrotta · 2023-03-16T06:35:11Z

+1

makortel · 2023-03-17T07:32:03Z

Should I go ahead to prepare the pull requests for backporting or wait?

I'd say please prepare the backports to 13_0_X and 12_6_X (and for further beyond the fix should be combined with the backport of #37278 along the other fixes listed in #37278 (comment))

makortel · 2023-04-11T15:25:32Z

@nhduongvn Thanks for the backport to 12_5_X in #41309 . I think though that backporting this PR to 13_0_X and 12_6_X would be more impactful.

cmsbuild added this to the CMSSW_13_1_X milestone Mar 6, 2023

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending labels Mar 6, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 6, 2023

makortel reviewed Mar 6, 2023

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 14, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 14, 2023

cmsbuild added tests-started and removed tests-pending labels Mar 15, 2023

makortel reviewed Mar 15, 2023

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Mar 15, 2023

handle invalid entries in <data-access>

da906d0

nhduongvn force-pushed the handle-invalid-data-access-master branch from 2621415 to da906d0 Compare March 15, 2023 14:28

cmsbuild added tests-pending and removed tests-approved code-checks-approved labels Mar 15, 2023

cmsbuild added the code-checks-pending label Mar 15, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 15, 2023

cmsbuild added tests-started and removed tests-pending labels Mar 15, 2023

cmsbuild added tests-approved and removed tests-started labels Mar 15, 2023

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Mar 15, 2023

makortel mentioned this pull request Mar 15, 2023

Add new data catalogs from Rucio storage description (RucioCatalog) and use it by default instead of trivial data catalogs (TrivialCatalog) #37278

Merged

cmsbuild added orp-approved and removed orp-pending labels Mar 16, 2023

cmsbuild merged commit e3e8bd5 into cms-sw:master Mar 16, 2023
11 checks passed

cmsbuild mentioned this pull request Mar 17, 2023

fix ASAN Segmentation failure & heap-buffer-overflow in unit test Alignment/OfflineValidation/GCPall #41080

Merged

nhduongvn mentioned this pull request Apr 10, 2023

Backport codes for adding new data catalogs from Rucio storage description (RucioCatalog) and use it by default instead of trivial data catalogs (TrivialCatalog) #37278, and related bug fixes #41309

Merged

nhduongvn mentioned this pull request Apr 28, 2023

Backport codes for handling invalid entries in <data-access> #41463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle invalid entries in <data-access> of site-local-config.xml #40961

Handle invalid entries in <data-access> of site-local-config.xml #40961

nhduongvn commented Mar 6, 2023 •

edited

cmsbuild commented Mar 6, 2023

cmsbuild commented Mar 6, 2023

makortel Mar 6, 2023

nhduongvn Mar 7, 2023

makortel Mar 6, 2023

nhduongvn Mar 7, 2023

makortel Mar 6, 2023

nhduongvn Mar 7, 2023 •

edited

makortel Mar 6, 2023

nhduongvn Mar 7, 2023

cmsbuild commented Mar 14, 2023

cmsbuild commented Mar 14, 2023

makortel commented Mar 15, 2023

makortel Mar 15, 2023

makortel commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

makortel commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

makortel commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

makortel commented Mar 15, 2023

stlammel commented Mar 15, 2023

nhduongvn commented Mar 15, 2023

perrotta commented Mar 16, 2023

makortel commented Mar 17, 2023

makortel commented Apr 11, 2023

	<< "Catch an exception while constructing a file locator in InputFileCatalog::init: " << e.what();
	<< "Caught an exception while constructing a file locator in InputFileCatalog::init: " << e.what();

	std::cout << "\n Skip this catalog" << std::endl;
	edm::LogWarning("InputFileCatalog") << "\n Skip this catalog";

	std::cout << "\n Skip this catalog" << std::endl;
	LogDebug("InputFileCatalog") << "\n Skip this catalog";

	for (auto it = tmp_dataCatalogs.begin(); it != tmp_dataCatalogs.end(); ++it) {
	for (auto const& catalog : tmp_dataCatalogs) {

Handle invalid entries in <data-access> of site-local-config.xml #40961

Handle invalid entries in <data-access> of site-local-config.xml #40961

Conversation

nhduongvn commented Mar 6, 2023 • edited

PR description:

cmsbuild commented Mar 6, 2023

cmsbuild commented Mar 6, 2023

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

nhduongvn Mar 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Mar 14, 2023

cmsbuild commented Mar 14, 2023

makortel commented Mar 15, 2023

Choose a reason for hiding this comment

makortel commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

Comparison Summary

cmsbuild commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

makortel commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

Comparison Summary

makortel commented Mar 15, 2023

cmsbuild commented Mar 15, 2023

makortel commented Mar 15, 2023

stlammel commented Mar 15, 2023

nhduongvn commented Mar 15, 2023

perrotta commented Mar 16, 2023

makortel commented Mar 17, 2023

makortel commented Apr 11, 2023

nhduongvn commented Mar 6, 2023 •

edited

nhduongvn Mar 7, 2023 •

edited