Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

O2O test setup modified to work across boost version changes #35440

Merged
merged 4 commits into from Sep 28, 2021

Conversation

ggovi
Copy link
Contributor

@ggovi ggovi commented Sep 28, 2021

PR description:

The workflows of integration tests for most of the O2Os are currently executed on special sqlite exports of the concerned production Tags, where only the second-last iov is copied. This choice, implemented with the command "conddb copy --o2oTest", allows to limit the execution time of the O2O workflow to a reasonable time. However, the present implementation fails when the second-last iov points to a payload produced with a boost release incompatible (=more recent) with the underlying release boost version. Typically, this happening in CMSSW branches that are left behind with respect to the last release used in production for the O2Os.
This problem is the cause of issue #33989

To overcome the problem, the o2oTest option of the function conddb copy has been modified, to select the highest iov in the source tag, pointing to a payload with boost version compatible with the underlying CMSSW release.

The new implementation fixes the above issue.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35440/25582

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • CondCore/Utilities (db)

@ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please review it and eventually sign? Thanks.
@mmusich 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

@ggovi
Copy link
Contributor Author

ggovi commented Sep 28, 2021

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35440/25583

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

Pull request #35440 was updated. @ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please check and sign again.

@ggovi
Copy link
Contributor Author

ggovi commented Sep 28, 2021

please test

Comment on lines 752 to 755
if timeType=='Run':
return time[0]
elif timeType=='Run':
return time[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these supposed to be both timeType == 'Run' ?

Copy link
Contributor Author

@ggovi ggovi Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really! It is not relevant for the above issue (all O2Os populates either run or timestamp tags), but definitely logically wrong.

@tvami
Copy link
Contributor

tvami commented Sep 28, 2021

test parameters:

  • addpkg = CondTools

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35440/25599

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

Pull request #35440 was updated. @ggovi, @malbouis, @cmsbuild, @tvami, @francescobrivio can you please check and sign again.

@ggovi
Copy link
Contributor Author

ggovi commented Sep 28, 2021

please test

@ggovi
Copy link
Contributor Author

ggovi commented Sep 28, 2021

test parameters:

addpkg = CondTools

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-53e808/19193/summary.html
COMMIT: dee5cb8
CMSSW: CMSSW_12_1_X_2021-09-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35440/19193/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 40
  • DQMHistoTests: Total histograms compared: 3211080
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3211058
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
  • Checked 169 log files, 37 edm output root files, 40 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Sep 28, 2021

+db

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d9b5bfd into cms-sw:master Sep 28, 2021
@ggovi
Copy link
Contributor Author

ggovi commented Sep 28, 2021

@tvami a bit too fast :-)
@perrotta
there is still one test failing in the 10_6_X build ( for some reason did not come up in my local testing ). I have ready the fix, but how do we deal with it? In principle I could add it directly to the 10_6_X PR (#35442) but I would need to make a new PR (disjoint from 35443) for the master? Please let me know how to proceed...

@perrotta
Copy link
Contributor

@tvami a bit too fast :-) @perrotta there is still one test failing in the 10_6_X build ( for some reason did not come up in my local testing ). I have ready the fix, but how do we deal with it? In principle I could add it directly to the 10_6_X PR (#35442) but I would need to make a new PR (disjoint from 35443) for the master? Please let me know how to proceed...

The backport #35442 has far many more changes than the few lines that were coded here. Does the fix relate to those extra lines, or the ones in common?
In any case, if any fix is needed also for the master, please implement if in an additional PR.

@ggovi
Copy link
Contributor Author

ggovi commented Sep 29, 2021

@perrotta Ah, right: the extra changes already appearing in 10_6_X are coming from the code-checks + code-format, that was already executed here in the master! The new fix will be totally unrelated. So - do I understand correctly:

Please confirm!

@perrotta
Copy link
Contributor

@perrotta Ah, right: the extra changes already appearing in 10_6_X are coming from the code-checks + code-format, that was already executed here in the master! The new fix will be totally unrelated. So - do I understand correctly:

Please confirm!

Correct @ggovi , please proceed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants