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.

Sign up for GitHub

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

Jump to bottom

[8.0] feat: SingularityCE: looking for the platform-aware image in CVMFS lo… #7589

Draft
wants to merge 3 commits into
base: rel-v8r0
Choose a base branch
from
Draft

[8.0] feat: SingularityCE: looking for the platform-aware image in CVMFS lo… #7589

wants to merge 3 commits into from
+171 −19

Conversation

fstagni
Copy link
Contributor

@fstagni fstagni commented Apr 22, 2024 •
edited
Loading

…cation

15th Jul Converted to draft, waiting if https://mattermost.web.cern.ch/cernvm/pl/djgirmur8tn33rkpu3px8dm6ao is implemented (multi-arch images)

BEGINRELEASENOTES

*Resources
CHANGE: SingularityCE can find at runtime which image to use, based on the local platform

ENDRELEASENOTES

@DIRACGridBot DIRACGridBot added the alsoTargeting:integration Cherry pick this PR to integration after merge label Apr 22, 2024
@fstagni fstagni force-pushed the 80_SingularityCE_platform branch 6 times, most recently from 2fc2b4f to ff5bdd0 Compare April 24, 2024 16:10
chrisburr
chrisburr reviewed Apr 25, 2024
View reviewed changes
src/DIRAC/Core/scripts/dirac_apptainer_exec.py
@@ -0,0 +1,84 @@
""" Starts a DIRAC command inside an apptainer container.
Copy link
Member

Choose a reason for hiding this comment

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

This is a great idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed this the other day but have yet to do a single test with it.

Anyway, a use case for this is clearly running, from the pilot, the dirac-platfom (dirac-architecture) script.

@fstagni fstagni force-pushed the 80_SingularityCE_platform branch 2 times, most recently from c2fc3c2 to 29b5e71 Compare May 1, 2024 12:48
@fstagni fstagni force-pushed the 80_SingularityCE_platform branch 2 times, most recently from 3017a3b to 0607a20 Compare May 16, 2024 13:08
@fstagni fstagni force-pushed the 80_SingularityCE_platform branch from 0607a20 to 7743c8a Compare May 23, 2024 11:56
chrisburr
chrisburr requested changes Jun 4, 2024
View reviewed changes
Copy link
Member

@chrisburr chrisburr left a comment

Choose a reason for hiding this comment

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

This still contains the LHCb specific stuff?

@fstagni fstagni force-pushed the 80_SingularityCE_platform branch 2 times, most recently from 79acb75 to 9ddc28f Compare June 5, 2024 09:06
@chrisburr
Copy link
Member

To summarise some of the thingsI just said in person:

  • For strings represeting the architecture I really think we should follow: https://github.com/opencontainers/image-spec/blob/main/image-index.md#platform-variants
  • In ${CVMFS_locations}/container/${technology}/${operating_system}/${platform}/
    • /container/ really restricts what you can use. For example we can't use /cvmfs/unpacked.cern.ch or /cvmfs/lhcb.cern.ch
    • technology is meaningless in this context
    • How could you apply versioning to this scheme so people can quickly roll back to a previous image?

I really think the layout should just be:

${MultiPlatformContainerRoot}/${architecture}${variant}

Then installations can do whatever they want with MultiPlatformContainerRoot.

For the ${architecture}${variant} part there is a little bit more room for debabte, ideally we should get unpacked.cern.ch to commit to exactly what they're going to do and do the same thing. We can also probably be lazy about variant for now (but we definitely need to have a plan for how it can be seemlessly supported).

It's then nice and simple, there are just two options for SingularityCE:

  • ContainerRoot: Legacy, for installations with homogeneous resources
  • MultiPlatformContainerRoot: Recommend, used by default to get whatever we decide as the canonical image

Then the question is what the default image for vanilla DIRAC should be...

@fstagni fstagni force-pushed the 80_SingularityCE_platform branch from 9ddc28f to 6475951 Compare June 5, 2024 10:46
@fstagni
Copy link
Contributor Author

fstagni commented Jun 5, 2024

I pushed (just before your comment) something close to what you are suggesting.

chrisburr
chrisburr reviewed Jun 5, 2024
View reviewed changes
src/DIRAC/Core/scripts/dirac_apptainer_exec.py

def getEnv():
"""Gets the environment for use within the container.
We blank almost everything to prevent contamination from the host system.
Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to reconcile the docstring with the implementation...it doesn't clear anything at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just a copy-paste. I will fix it once I can test this script.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we should clean the environment and apply the allow list that is in SingularityCE.

chrisburr
chrisburr reviewed Jun 5, 2024
View reviewed changes
src/DIRAC/Core/Utilities/Os.py Outdated
return None

CVMFS_locations = DIRAC.gConfig.getValue(
"LocalSite/CVMFS_locations", Operations().getValue("Pilot/CVMFS_locations", [])
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of CVMFS_locations here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These locations can be VO dependant.

Copy link
Member

Choose a reason for hiding this comment

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

How is that helpful in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A VO might want to /cvmfs/unpacked.cern.ch and another cvmfs/dirac.egi.eu, and so on...
In any case there is always a default.

Copy link
Member

Choose a reason for hiding this comment

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

But continer_root will basically never make sense on another CVMFS location...

Even if it did, it would be really weird for a VO to set up something different at the same path to try and change the behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Ping @fstagni

Copy link
Contributor Author

Choose a reason for hiding this comment

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

container_root IMHO should make sense on any CVMFS location.

@fstagni fstagni force-pushed the 80_SingularityCE_platform branch from c3c0635 to 6b517e7 Compare June 5, 2024 13:14
@fstagni fstagni force-pushed the 80_SingularityCE_platform branch from 6b517e7 to b82fb28 Compare July 15, 2024 14:25
@fstagni fstagni force-pushed the 80_SingularityCE_platform branch from b82fb28 to 9fc4830 Compare July 15, 2024 15:45
@fstagni fstagni marked this pull request as draft July 15, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisburr chrisburr chrisburr requested changes

@andresailer andresailer andresailer left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
alsoTargeting:integration Cherry pick this PR to integration after merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fstagni @chrisburr @andresailer @DIRACGridBot