-
Notifications
You must be signed in to change notification settings - Fork 125
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
Decouple automated K8s deployments #1531
Decouple automated K8s deployments #1531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 43 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache / macos-14, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, Web Platform Deployment / macos-14, Web Platform Deployment / ubuntu-24.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @SchahinRohani)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the modularity approach! But what do you think of moving kubernetes/overlays/chromium
, kubernetes/overlays/lre-manual
, kubernetes/overlays/lre
into the deploy/*
directory and removing the kubernetes/overlays
directory. We could avoid that layer and reduce a little bit of complexity.
Reviewed 43 of 43 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed, and 2 discussions need to be resolved (waiting on @SchahinRohani)
.github/workflows/lre.yaml
line 141 at r1 (raw file):
- name: Trigger pipelines run: | nix develop --impure --command bash -c 'cat > dummy-repo.yaml << EOF
nit: Nice idea! We might find a better a way in future, but this ensures for now that we are having the right ordering for flux!
kubernetes/nativelink/kustomization.yaml
line 7 at r1 (raw file):
resources: - nativelink.yaml - ../resources/insecure-certs
nit: Out of scope for this PR, but there might a better way to inject the necessary certs.
This change allows deploying nativelink in various new configurations, including cache-only and multi-toolchain setups. The new setup is significantly more modular and lets us add and remove toolchains at runtime. Includes a reordering of the deployment logic to ensure that Alerts get deployed before the GitRepository that triggers them. For demo and CI this comes at the cost of an additional dummy GitRepository. This tradeoff seems well worth it as the previous ordering was entirely wrong and unreliable. Fixes the flakiness of the LRE workflow where it often gets stuck in the `Waiting for Tekton pipelines` loop.
73a3966
to
7d4e34e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained, and 36 of 43 files reviewed, and pending CI: Web Platform Deployment / macos-14, Web Platform Deployment / ubuntu-24.04, pre-commit-checks (waiting on @SchahinRohani)
.github/workflows/lre.yaml
line 141 at r1 (raw file):
Previously, SchahinRohani (Schahin) wrote…
nit: Nice idea! We might find a better a way in future, but this ensures for now that we are having the right ordering for flux!
Done.
kubernetes/nativelink/kustomization.yaml
line 7 at r1 (raw file):
Previously, SchahinRohani (Schahin) wrote…
nit: Out of scope for this PR, but there might a better way to inject the necessary certs.
Done. Yeah this should be using cert-manager. I'll add it at some point 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 36 of 43 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed (waiting on @SchahinRohani)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained, and all files reviewed
This change allows deploying nativelink in various new configurations, including cache-only and multi-toolchain setups.
The new setup is significantly more modular and lets us add and remove toolchains at runtime.
Includes a reordering of the deployment logic to ensure that Alerts get deployed before the GitRepository that triggers them. For demo and CI this comes at the cost of an additional dummy GitRepository. This tradeoff seems well worth it as the previous ordering was entirely wrong and unreliable.
Fixes the flakiness of the LRE workflow where it often gets stuck in the
Waiting for Tekton pipelines
loop.This change is