gitlab ci: Rework how mirrors are configured by scottwittenburg · Pull Request #39939 · spack/spack

scottwittenburg · 2023-09-12T01:41:24Z

Improve how mirrors are used in gitlab ci, where we have until now thought of them as only a string. This PR aims to catch gitlab ci up to where the rest of spack is, in terms of mirror configuration.

By configuring ci mirrors ahead of time using the proposed mirror templates, and by taking advantage of the expressiveness that spack now has for mirrors, this PR will allow us to easily switch the protocol/url we use for fetching binary dependencies.

This PR also:

Adds three mirror templates which, assuming some variable replacement, should account for all the ways we use mirrors in ci.
Deprecates complicated (and likely somehow wrong) mirror management logic from the ci module.
Deprecates temporary storage and artifacts buildcache functionality from gitlab pipeline schema.
Deprecates --buildcache-destination and --copy-to options from the spack ci generate command.

Still TODO:

create a spackbot PR to set the PIPELINE_MIRROR_TEMPLATE appropriately for "rebuild everything" pipelines (done here)
add backwards compatibility for things the community is actually using
set up cloudfront for s3://spack-binaries-prs (and fix urls in mirror templates)
to avoid generating pipelines from stale indices, tell cloudfront not to cache binary indices (done in buildcache: Tell servers not to cache index or hash #40339)
solve problem where already cached things ignore re-writing origin objects with CacheContent: no-cache headers
- For develop mirror,I think we can write a script to issue cache invalidations for the existing index files, followed immediately by updating the index using the CacheContent: no-cache header. Since caching only happens on client requests, we should not have to worry about the PR mirrors as long as we merge buildcache: Tell servers not to cache index or hash #40339 before this.

scottwittenburg · 2023-09-12T01:45:34Z

fyi @kwryankrattiger @zackgalbreath I finally made this PR from the branch I've been using to test fetching from cloudfront in pipelines.

I'm still not sure how we bust the cache managed by cloudfront when we:

prune specs from mirrors (didn't come up in my testing)
update buildcache index in between pipelines (didn't come up in my testing where every pipeline was a "rebuild everything")

kwryankrattiger · 2023-09-12T02:14:21Z

For the first point, I agree we need to investigate more. I can imagine some edge cases where it would be problematic and pipelines would fail where a spec hash left existence two weeks earlier, and was reintroduced via a reversion or something on a PR that was depending on the old cached binary. I can't think of a case where it would break develop pipelines though, but maybe you have some ideas.

For the second one I am less concerned about the growing mirror case. In the worst case it will schedule redundant jobs and then do nothing when it finds the binary in the cache. It isn't ideal, but it shouldn't break pipelines which I think is the most important. And if we are getting a huge speed bump in fetch times with CF, the cost of extra scheduled jobs may be outweighed. That may be something worth measuring and/or annotating in the database so we can compare how many jobs like that are getting scheduled and how long they are taking to run. It may be that CF causes a significant overhead.

In the case where things are removed from the index, I think that follows the first point.

scottwittenburg · 2023-09-12T15:51:18Z

If it's possible to avoid caching at all for certain patterns (.+index.json$ or similar), I wonder if that would solve both points?

share/spack/gitlab/cloud_pipelines/configs/multi-src-mirrors.yaml.in

scottwittenburg · 2023-10-06T22:06:04Z

Fixed one issue caught by testing in staging.

scottwittenburg · 2023-10-10T21:05:09Z

Used an old PR on spack-test/spack to test the proposed changes to spackbot (to make sure we can still "rebuild everything"). It's here.

The proof that it worked is, e.g., here, where you can see that instead of the usual three mirrors you'd expect for a PR pipeline, there's just the one, and it corresponds to the PR-specific version.

scottwittenburg · 2023-10-13T00:35:42Z

I did some more testing of the various flavors of pipeline in my side repo, which uncovered a couple of issues I have now fixed.

PR pipeline rebuilding only what changed (readline in build_systems)
PR pipeline rebuilding everything (triggered from here)
a develop pipeline rebuilding only what changed
a "develop snapshot" pipeline (copy-only)
a release branch pipeline (rebuild everything)
a release tag pipeline (copy-only)

All those were run with some extra WIP commits to:

build many fewer specs in each stack
change all the mirror urls to point away from production mirrors
comment out the id_tokens bits so we could fall back on aws credentials from ci variables

share/spack/gitlab/cloud_pipelines/.gitlab-ci.yml

kwryankrattiger

I still need to verify the stacks have removed the mirror section, but having the old mirror listed also shouldn't hurt anything.

lib/spack/spack/ci.py

kwryankrattiger · 2023-10-16T21:17:30Z

lib/spack/spack/ci.py

+    # TODO: Remove this block in Spack 0.23
    mirrors_to_check = None
-    if remote_mirror_override:
+    if deprecated_mirror_config and remote_mirror_override:


I am not sure if it makes sense to disable this based on the deprecated_mirror_config flag since it is still a CLI option.

I don't want to modify the mirror state unless the user didn't use the new approach of adding a mirror named buildcache-destination and instead used that command line option. Did I get that logic wrong?

The option --buildcache-destination can still be specified without error, even in the new method. I think either this should be an error earlier saying we don't support both, or this specific override method should be allowed even in the new mode.

I didn't comment on the other mirror gymnastics we were doing because those were sort of magic from the outside world perspective.

Ok, I'm leaning toward an error saying we don't support both at the same time, unless you object.

That is also my inclination, having both active at the same time feels odd.

lib/spack/spack/ci.py

share/spack/gitlab/cloud_pipelines/.gitlab-ci.yml

Improve how mirrors are used in gitlab ci, where we have until now thought of them as only a string. By configuring ci mirrors ahead of time using the proposed mirror templates, and by taking advantage of the expressiveness that spack now has for mirrors, this PR will allow us to easily switch the protocol/url we use for fetching binary dependencies. This change also deprecates some gitlab functionality and marks it for removal in Spack 0.23: - arguments to "spack ci generate": * --buildcache-destination * --copy-to - gitlab configuration options: * enable-artifacts-buildcache * temporary-storage-url-prefix

kwryankrattiger

This looks good, thanks @scottwittenburg !

Improve how mirrors are used in gitlab ci, where we have until now thought of them as only a string. By configuring ci mirrors ahead of time using the proposed mirror templates, and by taking advantage of the expressiveness that spack now has for mirrors, this PR will allow us to easily switch the protocol/url we use for fetching binary dependencies. This change also deprecates some gitlab functionality and marks it for removal in Spack 0.23: - arguments to "spack ci generate": * --buildcache-destination * --copy-to - gitlab configuration options: * enable-artifacts-buildcache * temporary-storage-url-prefix

scottwittenburg added the don't-merge-yet label Sep 12, 2023

spackbot-app bot added binary-packages commands core PR affects Spack core functionality gitlab Issues related to gitlab integration tests General test capability(ies) labels Sep 12, 2023

kwryankrattiger mentioned this pull request Sep 12, 2023

gitlab ci: Put Cray binaries in public mirrors #39938

Merged

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from badb264 to 096a25b Compare September 12, 2023 16:35

spackbot-app bot added the shell-support label Sep 12, 2023

tldahlgren assigned kwryankrattiger Sep 12, 2023

scottwittenburg mentioned this pull request Sep 18, 2023

Draft: Add ppc64le pipelines for ML builds #39174

Closed

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from 096a25b to 67bce3d Compare October 2, 2023 23:01

spackbot-app bot added the documentation Improvements or additions to documentation label Oct 2, 2023

scottwittenburg force-pushed the ci-refactor-mirror-usage branch 6 times, most recently from 9319c39 to 48a990f Compare October 4, 2023 20:57

scottwittenburg mentioned this pull request Oct 4, 2023

Update how "@spackbot rebuild everything" is done spack/spackbot#92

Merged

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from 48a990f to 62e3688 Compare October 4, 2023 22:02

kwryankrattiger reviewed Oct 5, 2023

View reviewed changes

share/spack/gitlab/cloud_pipelines/configs/multi-src-mirrors.yaml.in Outdated Show resolved Hide resolved

scottwittenburg force-pushed the ci-refactor-mirror-usage branch 3 times, most recently from 91c7aa9 to 7ca2028 Compare October 6, 2023 22:04

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from 7ca2028 to e0e669a Compare October 6, 2023 22:37

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from e0e669a to abb3f89 Compare October 13, 2023 00:19

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from abb3f89 to 855ebeb Compare October 13, 2023 18:39

kwryankrattiger reviewed Oct 16, 2023

View reviewed changes

share/spack/gitlab/cloud_pipelines/.gitlab-ci.yml Outdated Show resolved Hide resolved

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from 855ebeb to 6437443 Compare October 16, 2023 18:02

kwryankrattiger reviewed Oct 16, 2023

View reviewed changes

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from 6437443 to 5cd8765 Compare October 18, 2023 17:35

scottwittenburg force-pushed the ci-refactor-mirror-usage branch from 5cd8765 to b614035 Compare October 18, 2023 23:17

kwryankrattiger approved these changes Oct 19, 2023

View reviewed changes

kwryankrattiger merged commit 46c1a8e into spack:develop Oct 19, 2023

scottwittenburg deleted the ci-refactor-mirror-usage branch October 19, 2023 16:43

Conversation

scottwittenburg commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottwittenburg commented Sep 12, 2023

Uh oh!

kwryankrattiger commented Sep 12, 2023

Uh oh!

scottwittenburg commented Sep 12, 2023

Uh oh!

Uh oh!

scottwittenburg commented Oct 6, 2023

Uh oh!

scottwittenburg commented Oct 10, 2023

Uh oh!

scottwittenburg commented Oct 13, 2023

Uh oh!

Uh oh!

kwryankrattiger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwryankrattiger Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

scottwittenburg Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

kwryankrattiger Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

scottwittenburg Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

kwryankrattiger Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kwryankrattiger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scottwittenburg commented Sep 12, 2023 •

edited

Loading