ci(kwok): add flux-git lane with in-cluster gitea#1290
Conversation
haarchri
force-pushed
the
feature/kwok-flux-git-lane
branch
from
8864e41 to
5188632
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Thanks @haarchri — this is a lot of careful work landing in one PR, and the bundling makes sense (#1288 and #1289 only surfaced because the new lane existed). ADR-010 is comprehensive, the distinct exit codes 70/71/72 follow the existing pattern, and the DRA name rollback is documented in 5 places that cross-reference each other so it's hard to drift.
One actionable: add the upstream kubernetes-sigs/dra-driver-nvidia-gpu issue link in values.yaml so "revert when the chart fix ships" has a closeable trigger. Two nits inline. Nothing blocking.
The workload rename (nvidia-dra-driver-gpu-* → dra-driver-nvidia-gpu-*) is a behavior change anyone selecting on the old names will feel — your rollout note already covers it, just flagging for visibility.
| namespace: flux-system | ||
| status: | ||
| ((conditions[?type=='Ready']|[0].status == 'True') || ((conditions[?type=='Released']|[0].status == 'True') && (history|[0].status == 'deployed') && (conditions[?type=='Stalled']|[0].status != 'True'))): true | ||
| ((conditions[?type=='Ready']|[0].status != 'True') && ((conditions[?type=='Released']|[0].status != 'True') || (history|[0].status != 'deployed') || (conditions[?type=='Stalled']|[0].status == 'True'))): true |
There was a problem hiding this comment.
The existence + error-polarity pair is exactly the right fix, and the WHY comment ("1 Ready + 12 dependency-blocked HRs passed instantly") is the kind of context that saves the next person hours. Quickly verified the De Morgan'd predicate against the positive form — matches. Nice catch surfacing this from the flux-git lane.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
recipes/components/nvidia-dra-driver-gpu/values.yaml (1)
59-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the upstream issue link to unblock restoration.
The comment references an upstream bug but provides no issue number or link. Without a tracking reference, the "restore the override once a fixed chart ships" guidance on lines 60-61 is unactionable — nobody will know when the fix is available.
📎 Suggested fix
- kubernetes-sigs/dra-driver-nvidia-gpu; restore the override (and the + kubernetes-sigs/dra-driver-nvidia-gpu#<issue-number>; restore the override (and the nvidia-dra-driver-gpu-* workload names below) once a fixed chart🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@recipes/components/nvidia-dra-driver-gpu/values.yaml` at line 59, The comment that says "bundle of this component. Upstream bug filed against" and the follow-up guidance to "restore the override once a fixed chart ships" lacks an upstream issue reference; update that comment in values.yaml to include the upstream issue URL or issue/PR number (or a link to the vendor's issue tracker) so anyone can track the fix, and if the exact issue is unknown add a TODO with a clear placeholder like "UPSTREAM-ISSUE: <url-or-number>" so the restore action is actionable and discoverable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@recipes/components/nvidia-dra-driver-gpu/values.yaml`:
- Line 59: The comment that says "bundle of this component. Upstream bug filed
against" and the follow-up guidance to "restore the override once a fixed chart
ships" lacks an upstream issue reference; update that comment in values.yaml to
include the upstream issue URL or issue/PR number (or a link to the vendor's
issue tracker) so anyone can track the fix, and if the exact issue is unknown
add a TODO with a clear placeholder like "UPSTREAM-ISSUE: <url-or-number>" so
the restore action is actionable and discoverable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6295b579-ea28-4986-a7fb-7fa55ee74861
📒 Files selected for processing (21)
.github/actions/kwok-test/action.yml.github/workflows/kwok-recipes.yaml.settings.yamlMakefiledocs/contributor/inference-perf-fluctuation.mddocs/contributor/tests.mddocs/contributor/validator.mddocs/design/008-kwok-deployer-matrix.mddocs/design/010-kwok-git-source-lanes.mdkwok/README.mdkwok/kind-config.yamlkwok/scripts/install-infra.shkwok/scripts/run-all-recipes.shkwok/scripts/validate-scheduling.shrecipes/checks/nvidia-dra-driver-gpu/health-check.yamlrecipes/components/nvidia-dra-driver-gpu/values.yamltests/chainsaw/ai-conformance/common/assert-dra-driver.yamltests/chainsaw/kwok/flux-git-sync/chainsaw-test.yamltests/chainsaw/kwok/flux-sync/chainsaw-test.yamlvalidators/conformance/dra_support_check.govalidators/deployment/expected_resources_test.go
|
Looks like the root-caused the OKE flux failures (all 6 OKE flux-oci / flux-git cells) has one-line fix. Symptom (from helm-controller logs on This is exactly the upstream duplicate-label bug this PR is fixing — but # See values.yaml for why both nameOverride and fullnameOverride are pinned.
nameOverride: nvidia-dra-driver-gpu # ← needs to go
fullnameOverride: nvidia-dra-driver-gpuOKE recipes merge Why only OKE-flux fails:
Cascade: Fix: delete the |
|
@mchmarny seeing upstream folks working on the issue regarding the label kubernetes-sigs/dra-driver-nvidia-gpu#1187 - we want to wait for a v0.4.1 or RC release and pin this ? |
Yes, let's give it a day, than we pin to the RC. |
haarchri
force-pushed
the
feature/kwok-flux-git-lane
branch
from
807e52b to
356032f
Compare
Signed-off-by: Christopher Haar <[email protected]>
haarchri
force-pushed
the
feature/kwok-flux-git-lane
branch
from
356032f to
ec3a15f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/kwok-test/action.yml (1)
110-111:⚠️ Potential issue | 🔴 CriticalGate Flux installation on Flux deployers (matches
flux_versiondocumented contract)
flux_versionis documented as required only forflux-ociandflux-git, but/.github/actions/kwok-test/action.ymlalways passesinstall_flux: 'true'(withflux_versiondefaulting to''):install_flux: 'true' flux_version: '${{ inputs.flux_version }}'
/.github/actions/setup-build-tools/action.ymlinstalls Flux wheninstall_flux=trueand fails ifFLUX_VERSIONis empty, so non-Flux deployers (including the defaultdeployer: helm) will break. Gateinstall_fluxbased oninputs.deployer(or require/populateflux_versionfor all deployers).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/kwok-test/action.yml around lines 110 - 111, The action always sets install_flux: 'true' and passes flux_version even when deployer is not a Flux deployer; change the logic in .github/actions/kwok-test/action.yml so install_flux is only true when inputs.deployer indicates a Flux-based deployer (e.g., 'flux-oci' or 'flux-git') or alternatively require/populate inputs.flux_version for all deployers; locate the install_flux and flux_version keys in the job inputs (references: install_flux, flux_version, inputs.deployer) and gate setting install_flux (or validate flux_version) accordingly so setup-build-tools no longer receives an empty FLUX_VERSION for non-Flux deployers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/010-kwok-git-source-lanes.md`:
- Around line 141-145: The architecture diagram entry for the aicr-registry uses
the incorrect image tag "registry:3" which conflicts with the ADR and KWOK
install docs; update the diagram text inside the block that shows
"aicr-registry" so the image label reads "registry:2" (matching other references
like registry:2 and the KWOK deployment image) to keep the documentation
consistent.
- Around line 97-103: In docs/design/010-kwok-git-source-lanes.md the ADR
reference links use "../docs/..." which, relative to docs/design, resolves to
docs/docs/...; update those links to use sibling-relative paths (e.g. replace
"../docs/..." with "../..."/appropriate "../<file>" so they point to the
intended files) ensuring each ADR link correctly targets the file from the
docs/design directory.
In `@kwok/scripts/validate-scheduling.sh`:
- Around line 968-981: The GIT_PUSH_URL currently embeds KWOK_GITEA_USER and
KWOK_GITEA_PASSWORD raw, which breaks URLs when credentials contain reserved
chars; update the script to URL-encode the overrides before constructing
GIT_PUSH_URL (or alternatively set credentials via git config/environment
instead of embedding). Implement or call a helper (e.g., url_encode or similar)
to encode ${KWOK_GITEA_USER} and ${KWOK_GITEA_PASSWORD} and use the encoded
variables when forming
GIT_PUSH_URL="http://${ENC_USER}:${ENC_PASS}`@localhost`:${gitea_host_port}/${gitea_user}/${recipe}.git"
(keep GIT_IN_CLUSTER_URL unchanged). Ensure any helper is sourced/defined in the
same script so encoding always runs when KWOK_GITEA_USER/KWOK_GITEA_PASSWORD are
overridden.
In `@tests/chainsaw/kwok/flux-git-sync/chainsaw-test.yaml`:
- Around line 144-167: Update the misleading numeric step references in the sync
note so they use step identifiers instead of step numbers: replace the comment
that says "Steps 2-4 (Kustomization, the HelmRelease terminal-pass predicate,
ArtifactGenerator) MUST stay byte-identical" with wording that names the steps
(e.g., refer to assert-kustomization-ready, the HelmRelease terminal-pass
predicate step name, and the ArtifactGenerator step name) so the note remains
correct even when assert-no-gitrepository-failed is inserted; apply the same
change to the other occurrences that mirror this comment (the blocks covering
the later ranges).
---
Outside diff comments:
In @.github/actions/kwok-test/action.yml:
- Around line 110-111: The action always sets install_flux: 'true' and passes
flux_version even when deployer is not a Flux deployer; change the logic in
.github/actions/kwok-test/action.yml so install_flux is only true when
inputs.deployer indicates a Flux-based deployer (e.g., 'flux-oci' or 'flux-git')
or alternatively require/populate inputs.flux_version for all deployers; locate
the install_flux and flux_version keys in the job inputs (references:
install_flux, flux_version, inputs.deployer) and gate setting install_flux (or
validate flux_version) accordingly so setup-build-tools no longer receives an
empty FLUX_VERSION for non-Flux deployers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 55aed5ed-dfa3-4e1a-bfe3-f54674ffc2f0
📒 Files selected for processing (14)
.github/actions/kwok-test/action.yml.github/workflows/kwok-recipes.yaml.settings.yamlMakefiledocs/contributor/tests.mddocs/design/008-kwok-deployer-matrix.mddocs/design/010-kwok-git-source-lanes.mdkwok/README.mdkwok/kind-config.yamlkwok/scripts/install-infra.shkwok/scripts/run-all-recipes.shkwok/scripts/validate-scheduling.shtests/chainsaw/kwok/flux-git-sync/chainsaw-test.yamltests/chainsaw/kwok/flux-sync/chainsaw-test.yaml
| Host 3300, not Gitea's default 3000, for the same collision-avoidance | ||
| reason as the registry's 5500-vs-5000 (port 3000 is commonly held by | ||
| Grafana / local dev servers). | ||
|
|
||
| **Branch `main` is a hard requirement.** The bundler defaults | ||
| `GitRepository.ref.branch` to `main` (`resolveTargetRevision()`), and | ||
| there is no CLI flag to change it for filesystem output, so the test |
There was a problem hiding this comment.
Fix the relative links in the ADR references.
These paths are resolved from docs/design/, so ../docs/... points at docs/docs/... and breaks the links. Use sibling-relative paths instead.
Suggested fix
-Lane details, sync gates, exit codes, and tuning variables are
-documented in
-[docs/contributor/tests.md](../docs/contributor/tests.md) ("KWOK
-Matrix Testing"); design rationale in
-[ADR-008](../docs/design/008-kwok-deployer-matrix.md) (OCI lanes) and
-[ADR-010](../docs/design/010-kwok-git-source-lanes.md) (Git-source
-lanes).
+Lane details, sync gates, exit codes, and tuning variables are
+documented in
+[docs/contributor/tests.md](../contributor/tests.md) ("KWOK Matrix
+Testing"); design rationale in
+[ADR-008](008-kwok-deployer-matrix.md) (OCI lanes) and
+[ADR-010](010-kwok-git-source-lanes.md) (Git-source lanes).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/010-kwok-git-source-lanes.md` around lines 97 - 103, In
docs/design/010-kwok-git-source-lanes.md the ADR reference links use
"../docs/..." which, relative to docs/design, resolves to docs/docs/...; update
those links to use sibling-relative paths (e.g. replace "../docs/..." with
"../..."/appropriate "../<file>" so they point to the intended files) ensuring
each ADR link correctly targets the file from the docs/design directory.
| ┌─────────────────── Kind cluster (KWOK-enabled) ───────────────────┐ | ||
| │ │ | ||
| │ ┌──────────────────────────┐ ┌───────────────────────────┐ │ | ||
| │ │ aicr-registry │ │ flux-system │ │ | ||
| │ │ registry:3 Service:5000 │ │ source-controller │ │ |
There was a problem hiding this comment.
Correct the registry image label in the architecture diagram.
registry:3 conflicts with the registry:2 image used elsewhere in the ADR and in the KWOK install docs. The diagram should match the actual Deployment image.
Suggested fix
-│ │ registry:3 Service:5000 │ │ source-controller │ │
+│ │ registry:2 Service:5000 │ │ source-controller │ │🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/010-kwok-git-source-lanes.md` around lines 141 - 145, The
architecture diagram entry for the aicr-registry uses the incorrect image tag
"registry:3" which conflicts with the ADR and KWOK install docs; update the
diagram text inside the block that shows "aicr-registry" so the image label
reads "registry:2" (matching other references like registry:2 and the KWOK
deployment image) to keep the documentation consistent.
| local gitea_host_port="${KWOK_GITEA_HOST_PORT:-3300}" | ||
| local gitea_user="${KWOK_GITEA_USER:-aicr}" | ||
| local gitea_password="${KWOK_GITEA_PASSWORD:-aicr-kwok-ci}" | ||
| # Dual-view URLs (same pattern as OCI_REF / OCI_IN_CLUSTER_REF): | ||
| # the runner pushes via the Kind host-port mapping; Flux's | ||
| # source-controller clones via Service DNS. --repo gets the | ||
| # IN-CLUSTER URL because it is baked into the bundle's own | ||
| # GitRepository source CR, which source-controller resolves | ||
| # from inside the cluster. The repo must be anonymously | ||
| # readable: the bundler's gitrepo template has no secretRef | ||
| # (install-infra.sh configures Gitea so pushed repos are | ||
| # public via DEFAULT_PUSH_CREATE_PRIVATE=false). | ||
| GIT_IN_CLUSTER_URL="http://gitea.aicr-registry.svc.cluster.local:3000/${gitea_user}/${recipe}.git" | ||
| GIT_PUSH_URL="http://${gitea_user}:${gitea_password}@localhost:${gitea_host_port}/${gitea_user}/${recipe}.git" |
There was a problem hiding this comment.
URL-encode overridden Gitea credentials before building the push URL.
KWOK_GITEA_USER and KWOK_GITEA_PASSWORD are documented as overridable, but this interpolates them raw into http://user:pass@.... Any @, :, /, or # in either value produces an invalid remote URL, so the flux-git push path fails even though Gitea itself is healthy. Encode the credentials first, or pass them through Git config/environment instead of embedding raw values in the URL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@kwok/scripts/validate-scheduling.sh` around lines 968 - 981, The GIT_PUSH_URL
currently embeds KWOK_GITEA_USER and KWOK_GITEA_PASSWORD raw, which breaks URLs
when credentials contain reserved chars; update the script to URL-encode the
overrides before constructing GIT_PUSH_URL (or alternatively set credentials via
git config/environment instead of embedding). Implement or call a helper (e.g.,
url_encode or similar) to encode ${KWOK_GITEA_USER} and ${KWOK_GITEA_PASSWORD}
and use the encoded variables when forming
GIT_PUSH_URL="http://${ENC_USER}:${ENC_PASS}`@localhost`:${gitea_host_port}/${gitea_user}/${recipe}.git"
(keep GIT_IN_CLUSTER_URL unchanged). Ensure any helper is sourced/defined in the
same script so encoding always runs when KWOK_GITEA_USER/KWOK_GITEA_PASSWORD are
overridden.
| - name: assert-kustomization-ready | ||
| description: | | ||
| Kustomization must reach Ready=True. Proves kustomize-controller | ||
| could PARSE the bundle's root kustomization.yaml and APPLY all | ||
| manifests (no template-render bugs, no missing files, no schema | ||
| violations). | ||
| try: | ||
| - assert: | ||
| resource: | ||
| apiVersion: kustomize.toolkit.fluxcd.io/v1 | ||
| kind: Kustomization | ||
| metadata: | ||
| name: ($kustomizationName) | ||
| namespace: flux-system | ||
| status: | ||
| (conditions[?type=='Ready']|[0].status == 'True'): true | ||
| catch: | ||
| - script: | ||
| content: | | ||
| echo "--- Kustomizations ---" | ||
| kubectl get kustomization -A 2>&1 || true | ||
| kubectl get kustomization -n flux-system -o yaml 2>&1 || true | ||
| echo "--- kustomize-controller (tail=200) ---" | ||
| kubectl logs -n flux-system deploy/kustomize-controller --tail=200 2>&1 || true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
SYNC NOTE step numbering is off by one due to the extra GitRepository step.
The comment at lines 21-23 says "Steps 2-4 (Kustomization, the HelmRelease terminal-pass predicate, ArtifactGenerator) MUST stay byte-identical" but with the additional assert-no-gitrepository-failed step, those are actually steps 3-5 in this file's step array. Consider updating the comment to reference step names instead of numbers for clarity and maintainability.
The functional content of the Kustomization, HelmRelease, and ArtifactGenerator steps is correctly byte-identical to the sibling flux-sync/chainsaw-test.yaml.
Also applies to: 169-228, 230-274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/chainsaw/kwok/flux-git-sync/chainsaw-test.yaml` around lines 144 - 167,
Update the misleading numeric step references in the sync note so they use step
identifiers instead of step numbers: replace the comment that says "Steps 2-4
(Kustomization, the HelmRelease terminal-pass predicate, ArtifactGenerator) MUST
stay byte-identical" with wording that names the steps (e.g., refer to
assert-kustomization-ready, the HelmRelease terminal-pass predicate step name,
and the ArtifactGenerator step name) so the note remains correct even when
assert-no-gitrepository-failed is inserted; apply the same change to the other
occurrences that mirror this comment (the blocks covering the later ranges).
PR 1341 (pin nvidia-dra-driver-gpu to 0.4.1-rc.1) has been merged. Should be unblocked now |
2 participants
Summary
Adds the
flux-gitKWOK deployer lane: an in-cluster Gitea receives the filesystem bundle viagit push, and Flux reconciles it from aGitRepositorysource — closing the Git-source round-trip gap in the deployer matrix. Along the way this fixes the chainsaw sync-gate exists-semantics bug (#1288).Motivation / Context
The KWOK matrix only validated OCI round-trips (
argocd-oci,argocd-helm-oci,flux-oci— ADR-008 / PR #956). The flux deployer's Git output shape (sources/gitrepo-*.yamlGitRepository CRs,branch: maindefault, GitRepositorysourceRefon local-chart HelmReleases) had zero end-to-end coverage, so regressions in it would ship undetected. The first live run of the new lane immediately surfaced two such masked bugs — validating the premise. Design rationale in ADR-010 (docs/design/010-kwok-git-source-lanes.md).Fixes: #1288
Related: #963 (flux half;
argocd-gitfollows on the same Gitea infra), #843, #1285, kubernetes-sigs/dra-driver-nvidia-gpu#1184Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)kwok/,tests/chainsaw/kwok), CI workflows (.github/), conformance validators (validators/), component values (recipes/components,recipes/checks)Implementation Notes
Testing
Risk Assessment
Rollout notes:
kind delete cluster --name aicr-kwok-test) to pick up the 3300→30300 port mapping;install-infra.shexit 71 is the telltale.Checklist
make testwith-race)make lint)git commit -S) — GPG signing info