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

protected runner jobs at uo: trust e4s-uo signing key #34828

Merged

Conversation

eugeneswalker
Copy link
Contributor

@eugeneswalker eugeneswalker commented Jan 5, 2023

Excluding these changes from the aarch64 stacks as we do not plan to provide aarch64 runners for the protected pool.

@kotfic @scottwittenburg

@spackbot-app spackbot-app bot added core PR affects Spack core functionality gitlab Issues related to gitlab integration labels Jan 5, 2023
@scottwittenburg
Copy link
Contributor

The last time I was thinking about this, I thought we were only having UO signing to support the power builds, so #32710 had a bit to support it, but only for that stack.

Based on this PR and the conversation I saw in slack today though, it seems like the plan is to have UO do builds for all stacks?

@eugeneswalker
Copy link
Contributor Author

eugeneswalker commented Jan 6, 2023

... it seems like the plan is to have UO do builds for all stacks?

Well, the thing is, we need at least a couple of our X86_64 machines in the protected runner pool in order to start running GPU tests using our GPUs. We would like to leverage the existing spack test integration in CI to try testing some of the packages like kokkos +cuda on the GPU-enabled runners we have.

Maybe we could flesh this out more in one of the CI meetings if there are some aspects we need to consider prior to merge? Bottom line I think we need to have at least a couple of our X86_64 GPU instances in the protected runner pool, as well as our power runners to support (a) spack test of GPU-requiring packages as part of CI and (b) E4S power pipeline. What do you think?

Copy link
Contributor

@scottwittenburg scottwittenburg left a comment

Choose a reason for hiding this comment

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

I think we can move ahead with letting UO pick up any protected jobs (instead of restricting it to just power builds). But before we merge this, we'll need to coordinate a PR on spack/spack-infrastructture to provision the UO public key to aws protected runners, and make sure we add that secret wherever it belongs.

@eugeneswalker eugeneswalker force-pushed the protected-runners-at-uo branch 2 times, most recently from fc38366 to 8e54c67 Compare January 6, 2023 16:34
@eugeneswalker
Copy link
Contributor Author

@scottwittenburg I updated this PR to address your comments. Can you review? Is this OK?

scottwittenburg
scottwittenburg previously approved these changes Jan 6, 2023
Copy link
Contributor

@scottwittenburg scottwittenburg left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I think this could be merged now, although we can't make the change to actually let UO runners pick up protected jobs until we get the UO public key mounted onto our runners.

@eugeneswalker
Copy link
Contributor Author

Looks good, thank you! I think this could be merged now, although we can't make the change to actually let UO runners pick up protected jobs until we get the UO public key mounted onto our runners.

Sounds great. Thanks for the quick review.

I'm thinking it might be best to keep this one open until the other work is done, in case we realize we could tweak this to be better. I'm not saying that will happen, but by leaving this open we keep our options open. And since merging now won't have an effect until the other work is done, there is no cost to leaving it open. What do you think?

@scottwittenburg
Copy link
Contributor

I'm thinking it might be best to keep this one open until the other work is done

Yeah, that makes sense.

@kotfic
Copy link
Contributor

kotfic commented Jan 9, 2023

K8S infrastructure has been updated to include the e4s.gpg public key which means dependencies built on UO infrastructure should now verify in AWS. I believe once this is rebased on develop it should be good to go.

@eugeneswalker
Copy link
Contributor Author

Just re-based this on develop. Hoping we get a green CI pipeline here and we can proceed with merge.

@eugeneswalker
Copy link
Contributor Author

Looks like this is all green and ready to go. @scottwittenburg What do you think?

@scottwittenburg
Copy link
Contributor

Just waiting for some protected build jobs to run so I can validate the e4s key mounted on the image, then I'll approve again and merge. Thanks for the reminder!

@scottwittenburg
Copy link
Contributor

I tested trusting the key that gets mounted in the protected runners now (thanks @kotfic), this is what I see:

gpg: key 954DB486ABE5E7AA: public key "E4S - University of Oregon - Spack 01" imported
gpg: Total number processed: 1
gpg:               imported: 1
gpg: inserting ownertrust of 6

So the key is properly formatted and I'll merge this now.

Copy link
Contributor

@scottwittenburg scottwittenburg left a comment

Choose a reason for hiding this comment

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

Thanks @eugeneswalker and @kotfic!

nhanford pushed a commit to nhanford/spack that referenced this pull request Feb 15, 2023
amd-toolchain-support pushed a commit to amd-toolchain-support/spack that referenced this pull request Feb 16, 2023
techxdave pushed a commit to Tech-XCorp/spack that referenced this pull request Feb 17, 2023
jmcarcell pushed a commit to key4hep/spack that referenced this pull request Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core PR affects Spack core functionality don't-merge-yet gitlab Issues related to gitlab integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants