[11_0_X] Protect storage accounting UDP messages from NaN, and Use StatisticsSenderService for all framework files #36358

makortel · 2021-12-03T20:30:40Z

PR description:

This PR is a combined backport of #35362 and #35505, following requests in #29412 and #36349. Includes also #36403 as further cleanup.

PR validation:

Unit tests pass.

NaN's were being reported from the values computed using sqrt. This most likely was from the different variables not being updated atomically together.

Previously, each try to open the file using a different PFN would report an open attempt for the same LFN. This meant we could have multiple opens but only one close for a given LFN.

When sending information to the StatisticsSenderService, the file LFN or URL must be supplied.

Send statistics for primary, secondary, and embedded files. The aggregate file statistics are only reset on primary file close boundaries to keep the behavior the same as previous. Changed all calls to closeFile_() to be the new closeFile()

Now broadcasts how the file is used.

cmsbuild · 2021-12-03T20:31:05Z

A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_11_0_X.

It involves the following packages:

IOPool/Input (core)
IOPool/SecondaryInput (core)
IOPool/TFileAdaptor (core)
Utilities/StorageFactory (core)
Utilities/XrdAdaptor (core)

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

cms-bot commands are listed here

makortel · 2021-12-03T20:32:30Z

@cmsbuild, please test

makortel · 2021-12-03T20:32:34Z

backport

cmsbuild · 2021-12-03T23:16:25Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0c72ac/20977/summary.html
COMMIT: e96e622
CMSSW: CMSSW_11_0_X_2021-11-28-0000/slc7_amd64_gcc820
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36358/20977/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2793840
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2793498
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 30 edm output root files, 34 DQM output files
TriggerResults: no differences found

cmsbuild · 2021-12-04T02:38:07Z

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

perrotta · 2021-12-04T09:20:56Z

IOPool/Input/src/RootInputFileSequence.cc includes also the updates originally in #28911 (“New developments using multiple data catalogs provided in site-local-config.xml“) : they were merged in CMSSW_11_1_0_patch1

@makortel please confirm that importing the updates only from one file in #28911 (instead of the whole PR) doesn't cause possible issues somewhere

makortel · 2021-12-06T14:23:07Z

@perrotta Thanks for detailed check, but could you clarify? My intention was to not include any changes from #28911 (and if I wasn't careful enough and something sneaked in, I want to understand it well). The changes in IOPool/Input/src/RootInputFileSequence.cc should be

Create one std::unique_ptr<InputSource::FileOpenSentry> sentry instead of separate objects for primary and fallback open attempts.
Call edm::storage::StatisticsSenderService::openingFile()
Call edm::storage::StatisticsSenderService::closedFile() via new member function closeFile().

All these are from #35505. Note that the first of these changes caused indentation changes, and the diff becomes easier to read with "Hide whitespace".

perrotta · 2021-12-06T16:43:05Z

@perrotta Thanks for detailed check, but could you clarify? My intention was to not include any changes from #28911 (and if I wasn't careful enough and something sneaked in, I want to understand it well). The changes in IOPool/Input/src/RootInputFileSequence.cc should be

Create one std::unique_ptr<InputSource::FileOpenSentry> sentry instead of separate objects for primary and fallback open attempts.

Call edm::storage::StatisticsSenderService::openingFile()

Call edm::storage::StatisticsSenderService::closedFile() via new member function closeFile().

All these are from #35505. Note that the first of these changes caused indentation changes, and the diff becomes easier to read with "Hide whitespace".

Hi Matti.
For example, the following line is from #28911:
https://github.com/cms-sw/cmssw/pull/36358/files?w=1;#diff-4f6e0887868f2e273e21bf5c6449848b28466b34e0a88961d3db5a3801d27c21R231
and in general, if I am not wrong, all the usages of the bool usedFallback_ were removed with #28911 and the same happens in this PR (were by the way that usedFallback_ is only defined and updated, but never used in the code)

makortel · 2021-12-06T17:18:13Z

Thanks @perrotta, I see your point now. Indeed the equivalent of usedFallback_=false was done in #28911. Via FileOpenSentry that boolean gets used only for preOpenFile, postOpenFile, preCloseFile, and postCloseFile signals. I went through all users of those signals (in 11_0_0)

FWCore/MessageService/src/MessageLogger.cc (clean)
FWCore/Services/plugins/CheckTransitions.cc (clean)
FWCore/Services/plugins/CondorStatusUpdater.cc (clean)
FWCore/Services/plugins/Timing.cc (clean)
FWCore/Services/plugins/Tracer.cc (all print)
HeterogeneousCore/CUDAServices/plugins/NVProfilerService.cc (clean)
IgTools/IgProf/plugins/IgProfService.cc (clean)
Utilities/StorageFactory/src/StatisticsSenderService.cc

Of those, Tracer makes use of the boolean by showing it in its printouts

cmssw/FWCore/Services/plugins/Tracer.cc

Lines 531 to 537 in a1d2ae9

    
           void Tracer::preOpenFile(std::string const& lfn, bool b) { 
        
             LogAbsolute out("Tracer"); 
        
             out << TimeStamper(printTimestamps_); 
        
             out << indention_ << indention_ << " starting: open input file: lfn = " << lfn; 
        
             if (dumpNonModuleContext_) 
        
               out << " usedFallBack = " << b; 
        
           }

and StatisticsSenderService propagates it in the UDP packet

cmssw/Utilities/StorageFactory/src/StatisticsSenderService.cc

Lines 236 to 238 in a1d2ae9

    
           if (usedFallback) { 
        
             os << "\"fallback\": true, "; 
        
           }

(although this PR changes the information delivery mechanism from the ActivityRegistry callbacks to a direct call of StatisticsSenderService::closedFile() from RootInputFileSequence::closeFile(), but the source of the information is the same).

I think this points to a bug in #28911 that the information if a fallback file is not used does not propagate anymore to the UDP packets.

makortel · 2021-12-06T18:08:11Z

I think this points to a bug in #28911 that the information if a fallback file is not used does not propagate anymore to the UDP packets.

I opened an issue #36375 and am thinking of fixing it, and including the fix as part of these backports. Thanks @perrotta for pointing it out!

makortel · 2021-12-09T21:04:16Z

IOPool/Input/src/RootInputFileSequence.cc

+      }
+      if (!filePtr && (hasFallbackUrl)) {
+        try {
+          usedFallback_ = true;


Actually, since the usedFallback_ = true is set here, 11_0_X (and earlier) do not need the backport of #36379. The FileOpenSentry will still always signal that none of the files are fallbacks, but that information is not being used anywhere (except in Tracer Service, but those being "wrong" is not a big deal). The StatisticsSenderService anyway gets the value of this boolean via direct call (instead of the ActivityRegistry callbacks).

cmsbuild · 2021-12-09T21:26:07Z

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

makortel · 2021-12-09T21:26:22Z

Now including #36403 too.

makortel · 2021-12-09T21:26:48Z

unhold

makortel · 2021-12-09T21:28:16Z

@cmsbuild, please test

cmsbuild · 2021-12-10T00:38:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0c72ac/21145/summary.html
COMMIT: ed0609f
CMSSW: CMSSW_11_0_X_2021-12-05-0000/slc7_amd64_gcc820
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36358/21145/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2793840
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2793497
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 30 edm output root files, 34 DQM output files
TriggerResults: no differences found

makortel · 2021-12-10T02:06:53Z

+1

cmsbuild · 2021-12-10T02:07:12Z

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

qliphy · 2021-12-10T04:13:36Z

+1

Dr15Jones added 7 commits December 3, 2021 13:56

Protect storage accounting UDP messages from NaN

d96e900

NaN's were being reported from the values computed using sqrt. This most likely was from the different variables not being updated atomically together.

Report only once when trying to open file

751e6a0

Previously, each try to open the file using a different PFN would report an open attempt for the same LFN. This meant we could have multiple opens but only one close for a given LFN.

StatisticsSenderService now requires LFN or URL

f9ef3a6

When sending information to the StatisticsSenderService, the file LFN or URL must be supplied.

Changed SecondaryProducer to be a one::EDProducer

421b4a2

Added unit test for StatisticsSenderService

51a1bd4

Pass InputType to StatisticsSenderService

e96e622

Now broadcasts how the file is used.

cmsbuild added this to the CMSSW_11_0_X milestone Dec 3, 2021

cmsbuild added core-pending orp-pending pending-signatures tests-pending labels Dec 3, 2021

makortel mentioned this pull request Dec 3, 2021

Backport recent StatisticsSenderService changes to earlier releases cms-sw/framework-team#329

Closed

11 tasks

cmsbuild added tests-started and removed tests-pending labels Dec 3, 2021

cmsbuild added tests-approved core-approved fully-signed and removed tests-started core-pending pending-signatures labels Dec 3, 2021

cmsbuild added core-pending hold pending-signatures labels Dec 6, 2021

makortel mentioned this pull request Dec 6, 2021

Fallback information not propagated to StatisticsSenderService #36375

Closed

Silence file closuring printouts in StatisticsSenderService by default

ed0609f

makortel commented Dec 9, 2021

View reviewed changes

cmsbuild added tests-pending and removed tests-approved labels Dec 9, 2021

cmsbuild removed the hold label Dec 9, 2021

cmsbuild added tests-started and removed tests-pending labels Dec 9, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 10, 2021

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Dec 10, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 10, 2021

cmsbuild merged commit 085def3 into cms-sw:CMSSW_11_0_X Dec 10, 2021

makortel deleted the backportStatisticsSenderService_110x branch December 10, 2021 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[11_0_X] Protect storage accounting UDP messages from NaN, and Use StatisticsSenderService for all framework files #36358

[11_0_X] Protect storage accounting UDP messages from NaN, and Use StatisticsSenderService for all framework files #36358

makortel commented Dec 3, 2021 •

edited

cmsbuild commented Dec 3, 2021

makortel commented Dec 3, 2021

makortel commented Dec 3, 2021

cmsbuild commented Dec 3, 2021

cmsbuild commented Dec 4, 2021

perrotta commented Dec 4, 2021

makortel commented Dec 6, 2021

perrotta commented Dec 6, 2021

makortel commented Dec 6, 2021

makortel commented Dec 6, 2021

makortel Dec 9, 2021

cmsbuild commented Dec 9, 2021

makortel commented Dec 9, 2021

makortel commented Dec 9, 2021

makortel commented Dec 9, 2021

cmsbuild commented Dec 10, 2021

makortel commented Dec 10, 2021

cmsbuild commented Dec 10, 2021

qliphy commented Dec 10, 2021

[11_0_X] Protect storage accounting UDP messages from NaN, and Use StatisticsSenderService for all framework files #36358

[11_0_X] Protect storage accounting UDP messages from NaN, and Use StatisticsSenderService for all framework files #36358

Conversation

makortel commented Dec 3, 2021 • edited

PR description:

PR validation:

cmsbuild commented Dec 3, 2021

makortel commented Dec 3, 2021

makortel commented Dec 3, 2021

cmsbuild commented Dec 3, 2021

Comparison Summary

cmsbuild commented Dec 4, 2021

perrotta commented Dec 4, 2021

makortel commented Dec 6, 2021

perrotta commented Dec 6, 2021

makortel commented Dec 6, 2021

makortel commented Dec 6, 2021

makortel Dec 9, 2021

Choose a reason for hiding this comment

cmsbuild commented Dec 9, 2021

makortel commented Dec 9, 2021

makortel commented Dec 9, 2021

makortel commented Dec 9, 2021

cmsbuild commented Dec 10, 2021

Comparison Summary

makortel commented Dec 10, 2021

cmsbuild commented Dec 10, 2021

qliphy commented Dec 10, 2021

makortel commented Dec 3, 2021 •

edited