Repository navigation
Conversation
Operators created at runtime reference the default catalog sources (redhat-operators etc.), disabled in airgap. Observed: the MCH operator's multicluster-engine subscription pointed at redhat-operators and never resolved — MCH stuck Installing for hours on a missing MultiClusterEngine CRD. Step 3d repoints any such subscription to the cs-* mirrored catalogs. (cherry picked from commit 37090e3cfc7464fcce0a1a4fe3dfe5cc95190b85)
The vendored kyverno chart pulls its images from reg.kyverno.io (not ghcr.io, which the imageset mirrors) — without a tag-mirror entry every kyverno pod ImagePullBackOffs in airgap. Add the registry to the manual ITMS (placeholder form, substituted at apply time). Observed live: kyverno pods crash-looped on image pull until the ITMS covered reg.kyverno.io.
hashicorp-vault chart 0.1.11 deploys vault:2.1.2-ubi by default; the imageset still pinned 1.21.4-ubi, leaving the resolved operand image unmirrored (range-pin aging). Align the pinned operand with what the chart actually deploys.
…STRY For mirror targets serving content under an org (e.g. quay...:443/mirror), the manual-mirror substitution must use the org-prefixed root or every tag-based operand pull (kyverno, certified intel/hashicorp operands) fails with unauthorized/404. Observed live: the kyverno pods ImagePullBackOff until the ITMS mirrors carried the org prefix.
The values shipped source: certified-operators (the default catalog, disabled in airgap). Each root-app sync attempt applied the default source, the subscription failed to resolve (CatalogSourcesUnhealthy), and the sync retried — re-running the vault hook and burning the retry budget. Repoint both intel-device-plugins and intel-tdx-dcap at the mirrored catalog from the airgap overlay, so the connected default (certified-operators) is preserved and the flip is automatic in disconnected mode.
The connected default (remote Intel PCS URL) is unreachable in airgap —
every kata-cc pod's TEE-evidence verification failed with 401
(RcarAttestFailed). The previous mechanism was broken: the
values-baremetal-airgap.yaml overlay cannot flip inline application
parameters (its kbs.tdx block was inert dead config — removed here).
The clustergroup chart tpl-renders override values, so select the
file:// backend with an inline {{ if $.Values.global.airgap }} conditional
on the collateralService parameter: connected renders the Intel PCS URL
(unchanged behavior), airgap renders the offline file backend collected
via make collect-dcap-collateral.
CoCo guest VMs pull images directly from the pod-spec reference — no IDMS/ITMS redirect inside the CVM — so in airgap the workloads must reference the mirror registry directly (reachable, trusted via the initdata CA, authenticated via the KBS regcred / cluster pull secret). Previously the overrides were commented examples requiring three manual uncomment-and-edit operations mid-deploy. Move them into the airgap overlay: they are absent in connected mode (chart defaults, behavior unchanged) and activate automatically in disconnected mode, with the mirror root taken from global.mirrorRegistry (placeholder convention; set at deploy time, org-prefixed for Quay-style targets). The clustergroup chart tpl-renders the override values.
With multiSourceConfig enabled, ArgoCD pulls the VP Helm charts from helmRepoUrl directly and the repo-server performs no IDMS redirect — in airgap this must point at the mirror or every chart-type application fails at the chart-fetch stage. Also clarify the global.airgap comment: the flag is set in values-global.yaml on the disconnected branch and loads the airgap overlay (the overlay does not set the flag itself).
The MCH operator creates the multicluster-engine subscription at runtime from the mce-subscription-spec annotation on the MultiClusterHub, which the ACM chart templates from acm.mce_operator.source (default redhat-operators — disabled in airgap). Without the override the subscription is born on the dead default catalog and MCH stalls Installing until a manual repoint — observed in three consecutive disconnected runs (DEP3, DEP4, the PR validatedpatterns#165 validation). Set the source from the airgap overlay so the subscription is born on the mirrored catalog; connected mode keeps the chart default. Chart- level mechanism (the annotation contract), not a post-MCH patch step.
The patterns-operator restores ArgoCD's Application health check (dropped in ArgoCD 1.8, argoproj/argo-cd#3781) when gitops.applicationHealthCheckEnabled is set on patterns-operator-config — it defaults to false. Without it, child Applications have unknown health and the root app applies all sync waves ungated: every child app is created within seconds and later-wave apps (workloads, wave 30) race earlier-wave dependencies (trustee/KBS, wave 20) instead of waiting for them to be healthy. Observed live: all 15 child apps created in ~4s during the PR validatedpatterns#165 validation run, workloads wedged on the KBS race. Set the flag in step 10, before the Pattern CR deploys, so vp-gitops is created with the health check and the wave layout executes as designed.
butler54
changed the title
The MCH operator creates the multicluster-engine subscription at runtime from the mce-subscription-spec annotation on the MultiClusterHub, which the ACM chart templates from acm.mce_operator.source (default redhat-operators — disabled in airgap). Without the override the subscription is born on the dead default catalog and MCH stalls Installing until a manual repoint — observed in three consecutive disconnected runs (DEP3, DEP4, the PR validatedpatterns#165 validation). Set the source from the airgap overlay so the subscription is born on the mirrored catalog; connected mode keeps the chart default. Chart- level mechanism (the annotation contract), not a post-MCH patch step.
The patterns-operator restores ArgoCD's Application health check (dropped in ArgoCD 1.8, argoproj/argo-cd#3781) when gitops.applicationHealthCheckEnabled is set on patterns-operator-config — it defaults to false. Without it, child Applications have unknown health and the root app applies all sync waves ungated: every child app is created within seconds and later-wave apps (workloads, wave 30) race earlier-wave dependencies (trustee/KBS, wave 20) instead of waiting for them to be healthy. Observed live: all 15 child apps created in ~4s during the PR validatedpatterns#165 validation run, workloads wedged on the KBS race. Set the flag in step 10, before the Pattern CR deploys, so vp-gitops is created with the health check and the wave layout executes as designed.
The acm application had no sync-wave annotation (wave 0), but the ACM chart renders the MultiClusterHub whose CRD is registered by the ACM operator — whose Subscription is rendered by the root app at wave 10. Under sync-wave gating this deadlocks: wave 0 holds the acm app, whose sync fails ArgoCD discovery on the not-yet-served operator.open-cluster-management.io/v1 group; wave 10, which would install the operator and register the CRD, is gated behind acm going healthy. Ungated deployments never noticed — the everything-at-once apply was load-bearing for ACM. Wave 15 places acm after the wave-10 operator subscriptions (whose health means the CSV is installed and the CRD is served) and before the wave-20 trustee/workloads. Observed live in the no-bounce experiment: acm stuck at discovery-retry 7/20 with all later waves held.
bade750 to
33650b8
Compare
The full rationale lives in the commit messages and the PR body; the values and script comments shrink to one-to-three-line pointers in the repo's existing style.
| print(s['metadata']['namespace'] + ' ' + s['metadata']['name']) | ||
| " 2>/dev/null) || true | ||
| local sub ns name | ||
| for sub in $subs; do |
There was a problem hiding this comment.
Bug: for sub in $subs splits on every space, so no subscription ever gets patched. The Python prints namespace name on each line, but an unquoted $subs splits on spaces as well as newlines. Each iteration therefore gets a single token: ns="${sub%% *}" and name="${sub##* }" both end up as that same token. The script then runs oc patch subscriptions <ns> -n <ns>, followed by oc patch subscriptions <name> -n <name>. Both fail, and || true hides the failure, while the step still logs "repointing ..." and "complete". Read the output line by line instead: while read -r ns name; do ...; done <<< "$subs".
There was a problem hiding this comment.
Fixed in 4f2305e — the repoint was rewritten: subscriptions are enumerated and patched one by one (no word-splitting on the subs list, no || true masking), the target CatalogSource is verified to exist and be READY before patching, and the step reports repointed / skipped (ArgoCD-owned) / missing counts. Live-verified on clean disconnected installs — step 3d runs green on a fresh cluster, and the one subscription this used to chase (MCE) is now born on the mirrored catalog via the ACM chart override (5cb65e3).
| # Mirror root for the workload image overrides (org-prefixed for Quay). | ||
| mirrorRegistry: "MIRROR_REGISTRY_HOST:8443" | ||
| # Connected default: the certified-operators catalog, disabled in airgap. | ||
| clusterGroup: |
There was a problem hiding this comment.
Possible bug: the clusterGroup.subscriptions and clusterGroup.applications.*.overrides in this overlay may never reach the chart that renders them. The overlay is loaded through clusterGroup.sharedValueFiles. clustergroup 0.9.58 (templates/plumbing/applications.yaml) adds sharedValueFiles only to the valueFiles of child Applications. The root clustergroup Application, built by patterns-operator, uses values-global, values-, values-*, and extraValueFiles. That root app is the one that renders Subscription objects and runs tpl on overrides. If that is right, with global.airgap: true the intel subscriptions stay on certified-operators, acm.mce_operator.source is never set, and the CoCo workloads keep their connected images. Please confirm this with a real render of the root app (for example make preview-all). If it doesn't apply, load the overlay through global.extraValueFiles or the Pattern CR's extraValueFiles.
There was a problem hiding this comment.
Confirmed via render and fixed — 7a0ad34 selects the connected/airgap overlay through an inline templated sharedValueFiles entry, and the overlay header documents the reach: the patterns-operator resolves clusterGroup.sharedValueFiles itself when it builds the root Application, so the subscriptions and app overrides in the overlay are in scope. 599c2f8 adds a CI render gate that renders the full chain in both modes without a cluster — the airgap render shows the mirrored sources, file:// collateral and mirror-prefixed workload images. Live-verified: operator subscriptions born on cs-*-operator-index-v4-22 and workload pods pulling quay…/mirror/ubi9/… inside the CVMs.
| tdx: | ||
| collateralService: "file:///opt/confidential-containers/attestation-service/tdx/platform_collaterals.json" | ||
| # Mirror root for the workload image overrides (org-prefixed for Quay). | ||
| mirrorRegistry: "MIRROR_REGISTRY_HOST:8443" |
There was a problem hiding this comment.
Bug: global.mirrorRegistry is now active, but it is a literal placeholder that nothing substitutes. Before this PR the image overrides were commented out and told the user to "set MIRROR_REGISTRY_HOST to your mirror registry". They are now on by default, and that instruction was deleted from values-baremetal.yaml. Only itms-manual-mirrors.yaml gets the placeholder replaced (in airgap-post-install.sh). This overlay is consumed from git as is. As a result hello-openshift, kbs-access-curl and kbs-access-sealed pods render MIRROR_REGISTRY_HOST:8443/ubi9/..., and the CVM, which has no IDMS/ITMS, hits ImagePullBackOff. The comment also says "org-prefixed for Quay", yet the value carries no org path. Either document that this must be edited, or generate or substitute it from $MIRROR_REGISTRY.
There was a problem hiding this comment.
Fixed in 70e96c6 — the committed default is mirrorRegistry: "" (connected-inert), and the workload charts fail rendering in airgap mode when the value is empty or still a placeholder, so a literal placeholder can no longer render silently. The real value is set on the disconnected deploy branch as a config-only values-global.yaml delta. Live-verified: all five workload containers pull quay.apac-tech-lab.net:443/mirror/ubi9/httpd-24@sha256:68a91ff… (org-prefixed, digest-exact) inside the confidential VMs.
| # ─── Step 3d: Repoint default-catalog subscriptions to the mirror catalog ─ | ||
| # Operators created at runtime reference the DEFAULT catalog sources, which | ||
| # are disabled in airgap — without repointing they never resolve. | ||
| repoint_default_catalog_subscriptions() { |
There was a problem hiding this comment.
Timing and fix depth: this repoint step runs once, before the pattern is deployed. The comment says it targets "Operators created at runtime", but it runs in the bootstrap sequence before create_operator_config and before the Pattern CR exists. Subscriptions created later by ArgoCD, ACM, or operators themselves are never repointed. Subscriptions that ArgoCD owns with selfHeal on will also be reverted to the git source on the next sync. A one-shot out-of-band script therefore can't keep this right. AGENTS.md calls /scripts/ a "Last resort". Making this declarative would fix it at the source: either an idempotent imperative job on the 10-minute schedule, or the values overlay.
There was a problem hiding this comment.
Fixed in 4f2305e — the repoint is now idempotent and re-runnable (make repoint-subs / --repoint-subs) so it can be re-run after ArgoCD creates subscriptions, and the one runtime-created subscription that used to need chasing (MCE, created by the MCH operator) is handled at the source: it is born on the mirrored catalog via the ACM chart override (5cb65e3). Live-verified on a clean disconnected install: subscription.operators/multicluster-engine has source cs-redhat-operator-index-v4-22 with no manual repoint, MCH Running.
| gitops.sourceNamespace: "openshift-marketplace" | ||
| # App-of-apps sync-wave gating (Application health check) — see | ||
| # argoproj/argo-cd#3781; without it the root app applies all waves ungated. | ||
| gitops.applicationHealthCheckEnabled: "true" |
There was a problem hiding this comment.
Altitude: sync-wave gating is switched on only on the airgap bootstrap path. The PR is titled "mode-agnostic", and the ACM app moves to wave 15 "to break the CRD discovery deadlock" in values-baremetal.yaml, which is used in both modes. But gitops.applicationHealthCheckEnabled is set only in this airgap script's ConfigMap. The connected ./pattern.sh make install path never sets it. There, the root app applies every wave without waiting, so the wave-15 ordering has no effect and the deadlock can still happen. Set this for every install path, for example in the pattern-install / operator config used by make install.
There was a problem hiding this comment.
Valid point — root-caused rather than papered over: the connected path cannot set this key at all, because pattern-install (the chart behind every make install) never renders gitops.applicationHealthCheckEnabled into the patterns-operator-config ConfigMap, while the patterns-operator supports it and defaults it off.
Filed upstream with a full reproduction (the chart's rendered ConfigMap is byte-identical whether or not you set the value): validatedpatterns/pattern-install-chart#52, fixed by #53.
What this PR now does:
main.gitops.applicationHealthCheckEnabled: "true"invalues-global.yaml— declarative, mode-agnostic, effective on every install path once pattern-install ≥ 0.0.19 renders it (no race: the ConfigMap is applied with the chart's rendered content, before the operator's first reconcile).- The airgap bootstrap (step 10) keeps writing the key directly — that path installs the operator subscription before any chart render — and now asserts the key landed instead of hoping.
Do not merge this PR until validatedpatterns/pattern-install-chart#53 is merged and released — the wave-15 ordering introduced here only executes with the health check in place on both paths.
Both lifecycle validation runs (connected and disconnected) for this PR are being executed against the fixed chart via the PATTERN_INSTALL_CHART override; results will be posted here.
There was a problem hiding this comment.
Follow-up to my earlier reply, now landed and verified: 48831de sets gitops.applicationHealthCheckEnabled: "true" in values-global.yaml so every install path renders it (via pattern-install >= 0.0.19 — validatedpatterns/pattern-install-chart#53), while the disconnected bootstrap keeps writing the ConfigMap key directly as a bridge until that chart releases. Live-verified on both paths: the connected run (2026-11-03) and the disconnected run (2026-11-04) both executed the wave layout (0 -> 10 -> 15 -> 20 -> 30), where the pre-fix connected path applied all waves at once.
| # Wave 15: after the wave-10 ACM operator subscription (MCH CRD), | ||
| # before trustee at 20. | ||
| annotations: | ||
| argocd.argoproj.io/sync-wave: "15" |
There was a problem hiding this comment.
Bug: the wave-15 gating depends on Application health checks, which are enabled only in airgap. In connected installs (no gitops.applicationHealthCheckEnabled), Argo treats child Applications as healthy straight away. The acm app at wave 15 is applied together with the wave-10 subscription, so the MCH CRD discovery race this commit is meant to fix is still there.
There was a problem hiding this comment.
Valid point — root-caused rather than papered over: the connected path cannot set this key at all, because pattern-install (the chart behind every make install) never renders gitops.applicationHealthCheckEnabled into the patterns-operator-config ConfigMap, while the patterns-operator supports it and defaults it off.
Filed upstream with a full reproduction (the chart's rendered ConfigMap is byte-identical whether or not you set the value): validatedpatterns/pattern-install-chart#52, fixed by #53.
What this PR now does:
main.gitops.applicationHealthCheckEnabled: "true"invalues-global.yaml— declarative, mode-agnostic, effective on every install path once pattern-install ≥ 0.0.19 renders it (no race: the ConfigMap is applied with the chart's rendered content, before the operator's first reconcile).- The airgap bootstrap (step 10) keeps writing the key directly — that path installs the operator subscription before any chart render — and now asserts the key landed instead of hoping.
Do not merge this PR until validatedpatterns/pattern-install-chart#53 is merged and released — the wave-15 ordering introduced here only executes with the health check in place on both paths.
Both lifecycle validation runs (connected and disconnected) for this PR are being executed against the fixed chart via the PATTERN_INSTALL_CHART override; results will be posted here.
There was a problem hiding this comment.
Follow-up: 48831de enables the health check on every install path (see the thread on the bootstrap altitude), so the wave-15 acm placement now gates in connected installs too. Connected validation on 2026-11-03 shows the same ordering as airgap: wave 0 apps at 10:03-10:08, wave 10 at 10:20, acm (15) at 10:35 after the wave-10 operator subscriptions, trustee (20) at 10:35, workloads (30) at 11:09.
| # Vault — Red Hat certified UBI image from registry.connect.redhat.com | ||
| - name: registry.connect.redhat.com/hashicorp/vault:1.21.4-ubi | ||
| # (hashicorp-vault chart 0.1.11 deploys vault:2.1.2-ubi by default) | ||
| - name: registry.connect.redhat.com/hashicorp/vault:2.1.2-ubi |
There was a problem hiding this comment.
Bug: the imageset pins vault 2.1.2-ubi and drops 1.21.4-ubi, but the chart is a range (chartVersion: "~0.1.10"). If 0.1.10 is the chart version resolved (or mirrored), it deploys vault:1.21.4-ubi, which is no longer mirrored, and vault gets ImagePullBackOff. A future 0.1.12 that bumps the image breaks the same way. Pin chartVersion: 0.1.11 to match, or keep both tags in the imageset.
There was a problem hiding this comment.
Fixed in 6e3d851 — the imageset now pins the chart and its operand together: quay.io/validatedpatterns/hashicorp-vault:0.1.11 (the chart version the values pin exactly) and registry.connect.redhat.com/hashicorp/vault:2.1.2-ubi (what chart 0.1.11 deploys), with the pairing documented in the imageset comments. Live-verified: vault-0 Running and unsealed, 10 secrets loaded, ESO ClusterSecretStore Valid.
| step "3d" "Repoint default-catalog subscriptions to mirrored catalogs" | ||
|
|
||
| local map=" | ||
| redhat-operators:cs-redhat-operator-index-v4-22 |
There was a problem hiding this comment.
Hardcoded catalog version. The -v4-22 catalog names are hardcoded here (and in the overlay). The pattern supports global.clusterVersion 4.19 to 4.22, and create_operator_config in this same script falls back to cs-redhat-operator-index-v4-21. On a 4.21 mirror, subscriptions get repointed to a CatalogSource that doesn't exist and stay unresolvable. Derive the suffix from clusterVersion in values-global.yaml, or from the CatalogSources create_catalog_sources actually created.
There was a problem hiding this comment.
Fixed in 4f2305e — the catalog suffix is derived from global.clusterVersion (the step logs Catalog suffix -v4-22 (from global.clusterVersion 4.22)), and step 3e fails fast before the pattern deploys if any catalog referenced by the overlay is missing or not READY on the cluster.
| source: cs-certified-operator-index-v4-22 | ||
| # CoCo guest VMs pull images directly — no IDMS/ITMS inside the CVM. | ||
| applications: | ||
| acm: |
There was a problem hiding this comment.
Maintainability: the overlay's overrides: lists silently replace the base lists. Helm replaces lists when merging; it does not merge them. If anyone later adds an override to acm, hello-openshift, or the kbs-access-* apps in values-baremetal.yaml, it is silently dropped whenever global.airgap: true. The base comments ("Airgap: ... set by the airgap overlay") don't mention this. Consider tpl-conditional values in the base file, as done for kbs.tdx.collateralService. That keeps a single list.
There was a problem hiding this comment.
Addressed — the overlay was restructured to set only scalars (map values); the mode-dependent list entries became tpl-conditionals in the base files instead (e.g. acm.mce_operator.source in 5cb65e3). The overlay header now carries an explicit LIST WARNING documenting Helm's list-replacement semantics so future editors don't reintroduce the hazard.
| @@ -1,3 +1,5 @@ | |||
| # Org-prefixed mirror targets: MIRROR_REGISTRY must include the org path | |||
There was a problem hiding this comment.
Contradictory guidance on the mirror format. This new comment says MIRROR_REGISTRY must include the org path (for example quay.example:443/mirror). But airgap-post-install.sh documents and validates it as "registry host:port (e.g. MIRROR_REGISTRY_HOST:8443)", and the placeholder in this file and in the overlay is MIRROR_REGISTRY_HOST:8443 with no org. Users following the script's own help text get the 404s this comment warns about. Align the script help text, the error message, and the placeholders.
There was a problem hiding this comment.
Fixed in 66b45db — the ITMS header, the script's help/validation text and the imageset header now all state the org-prefixed mirror root consistently (host:port/org), matching what the script requires. Live-verified: tag-based operand pulls for reg.kyverno.io, registry.connect.redhat.com/intel and /hashicorp all succeeded against quay…:443/mirror.
yuvalk
left a comment
yuvalk
left a comment
There was a problem hiding this comment.
Review summary
I left 10 inline comments. These come from reading the diff and the clustergroup 0.9.58 templates; I haven't checked them against a live render or cluster.
Blocking
- Subscription repoint does nothing (
scripts/airgap-post-install.sh).for sub in $subssplits eachns nameline into two tokens, so everyoc patchtargets the wrong object.|| truehides the failures, and the step still logs success. - The airgap overlay may never reach the root app (
overrides/values-baremetal-airgap.yaml). As far as I can tell,sharedValueFilesis passed only to child Applications, not to the root clustergroup render that creates Subscriptions and appoverrides. If so, theintel-*subs stay oncertified-operators, the MCE source isn't set, and the workload images aren't switched to the mirror. Amake preview-allwithglobal.airgap=truewould confirm or rule this out. - Placeholder registry is committed.
global.mirrorRegistry: MIRROR_REGISTRY_HOST:8443is never substituted, so the workload images render with a hostname that doesn't resolve.
Should fix
- Repoint runs too early. It runs at bootstrap, before any ArgoCD-created Subscriptions exist. If it's re-run after deploy, ArgoCD self-heal reverts the subs it owns.
- ACM wave-15 gating only works in airgap.
gitops.applicationHealthCheckEnabledis set only in the airgap bootstrap ConfigMap, so connected installs still have the CRD race. The PR describes these fixes as mode-agnostic. - Catalog names hardcoded to
-v4-22. The script elsewhere falls back tov4-21, and repointed subs never check that the target CatalogSource exists. - Vault image vs chart range. The imageset pins
2.1.2-ubiand drops1.21.4-ubi, while the chart floats at~0.1.10.
Minor
- Overlay lists replace the base lists. Helm doesn't merge lists, so the overlay's
overrides:lists silently drop any future base-file entries when airgap is on. MIRROR_REGISTRYorg-path guidance is inconsistent. The ITMS header says the org path is required, but the script help and placeholders usehost:portonly.
Suggested order: fix #1, confirm #2 with a render, then #3. The rest can follow.
… path ArgoCD 1.8 dropped the Application health check (argoproj/argo-cd#3781); the patterns-operator can restore it but defaults it off, so the root app applied all sync waves at once and wave ordering (workloads after trustee) was never enforced. Set main.gitops.applicationHealthCheckEnabled declaratively in values-global.yaml — rendered into the patterns-operator-config ConfigMap by pattern-install >= 0.0.19 (validatedpatterns/pattern-install-chart#52, fixed by validatedpatterns#53). The disconnected bootstrap (airgap-post-install.sh step 10) keeps writing the key directly because the operator subscription predates any chart render there, and now asserts the key landed instead of hoping.
…point The repoint loop split each 'namespace name' line on whitespace, so every oc patch targeted the wrong object and || true hid the failures while the step still logged success. Read line by line instead, drop the error masking, and fail loudly if anything is still on a disabled default catalog at the end. Scope it to what it can actually fix: subscriptions created at runtime by operators (e.g. MCH's multicluster-engine). ArgoCD-owned subscriptions are skipped and reported — they are declarative via the airgap overlay, and ArgoCD self-heal would revert an out-of-band patch on them anyway. Derive the mirrored catalog suffix from global.clusterVersion (4.22 → v4-22) instead of hardcoding -v4-22, verify the target CatalogSource exists before patching, and do the same derivation for the step-10 gitops source fallback (previously a stale -v4-21 default). New step 3e fails fast before the pattern deploys when the airgap overlay references a catalog that is not READY on the cluster, or when global.mirrorRegistry is missing/placeholder or does not match MIRROR_REGISTRY. The step is exposed standalone as 'make airgap-repoint-subs' (safe to re-run, e.g. after MCH deploys).
CoCo guest VMs pull images directly — there is no IDMS/ITMS inside the CVM — so the workload charts now rewrite their image references against global.mirrorRegistry when global.airgap is true (oc-mirror source-path convention: registry hostname stripped, org-prefixed mirror root prefixed). Connected mode is unchanged. Rendering fails fast when the mirror is unset or a placeholder in airgap mode, instead of surfacing as ImagePullBackOff inside the CVMs later. global.mirrorRegistry moves to values-global.yaml as a deploy-time value on the disconnected branch (same convention as global.airgap) — the committed placeholder in the airgap overlay was never substituted and would have produced MIRROR_REGISTRY_HOST image references. This also removes the overlay's applications overrides entirely: Helm replaces lists when merging, so those lists would silently drop any entry later added to the same app in values-baremetal.yaml. The acm.mce_operator.source override moves into the base file as a single tpl-conditional entry (mirrored catalog in airgap, the chart default redhat-operators otherwise), leaving the overlay with scalars only.
The values range (~0.1.10) resolved whatever was newest at deploy time while the imageset pinned a single chart tag: chart 0.1.11 deploys vault:2.1.2-ubi but 0.1.10 deploys 1.21.4-ubi, so a range could desync the operand image from the mirror. Pin the exact version the imageset mirrors.
ca482e7 to
67e7516
Compare
butler54
added
the
do-not-merge
7fc24aa to
344076b
Compare
The ITMS header said the org path is required while the bootstrap script's help and error text said bare host:port; following the script's own guidance produced the 404s the header warned about. State the org-prefixed mirror root consistently in the script help/validation, the ITMS comment and the imageset header.
Renders the full root-app values chain (the same files the patterns-operator assembles: values-global + values-<group> + the resolved sharedValueFiles) with the clustergroup chart, and asserts the airgap overlay's effect on subscriptions and app overrides, connected mode rendering the upstream defaults, and the workload charts' image rewriting — including the fail-fast when the mirror is unset in airgap mode. Lets reviewers confirm the overlay reaches the root render without a cluster.
ArgoCD applies wave-0 resources by kind, so the imperative ServiceAccounts are attempted before the cluster-admin ClusterRoleBinding that authorises the controller to create them — the applies fail forbidden in the first seconds of the first sync (observed live: SAs forbidden at sync start, the binding applied 8s in by the same sync, permitted because the operator's instance role already grants rbac.authorization.k8s.io: */*). Ungated installs self-heal on the next automated sync, which is why this was never seen before; with sync-wave gating the first operation parks on wave-0 health (ESO Degraded ← vault unconfigured ← imperative cronjobs cannot start ← SA missing) and the retry never fires — a hard deadlock on clean installs. Render a distinct-named cluster-admin binding at sync-wave -1 via clusterGroup.extraObjects. The controller can create it from the start (full rbac.* from the operator role) and ClusterRoleBindings are instantly healthy, so wave 0 then applies the ServiceAccounts with cluster-admin in place. Bridge, not a fix: patterns-operator#887 tracks the root cause with two upstream options (operator-created binding, or moving the clustergroup chart's own binding to wave -1). TODO marker included; delete this once either lands.
344076b to
7efafc8
Compare
| # Skip ArgoCD-owned subscriptions: declarative via the overlay, | ||
| # and self-heal reverts out-of-band patches on them. | ||
| if oc get subscription.operators.coreos.com "$name" -n "$ns" \ | ||
| -o jsonpath='{.metadata.labels.argocd\.argoproj\.io/instance}' 2>/dev/null | grep -q .; then |
There was a problem hiding this comment.
This skip never matches. clustergroup 0.9.58 defaults resourceTrackingMethod: annotation, so ArgoCD-managed objects carry the argocd.argoproj.io/tracking-id annotation, not the argocd.argoproj.io/instance label. So make airgap-repoint-subs will still patch ArgoCD-owned subs, self-heal reverts them, and the "skipped (ArgoCD-owned)" counter always shows 0.
Suggest treating a sub as ArgoCD-owned if it has either the label or the tracking-id annotation.
| {{- if or (not $mirror) (contains "MIRROR_REGISTRY_HOST" $mirror) -}} | ||
| {{- fail "global.mirrorRegistry must be set to the mirror root (e.g. quay.example:443/mirror) when global.airgap is true" -}} | ||
| {{- end -}} | ||
| {{- $path := regexReplaceAll "^[a-zA-Z0-9.-]+(:[0-9]+)?/(.*)$" $image "${2}" -}} |
There was a problem hiding this comment.
The rewrite strips the first segment unconditionally, so an image that already points at the mirror gets prefixed twice: quay.example:443/mirror/ubi9/httpd-24@… becomes quay.example:443/mirror/mirror/ubi9/httpd-24@…. That's likely for anyone who kept the manual image override the old values-baremetal.yaml comments recommended. A reference with no registry host (e.g. library/httpd:2.4) also loses its first path segment.
Suggest returning the image unchanged when it already starts with $mirror, and only stripping the first segment when it looks like a host (contains . or :, or is localhost).
| and the mirror is unset or still a placeholder — a missing value here would | ||
| otherwise surface as ImagePullBackOff inside the CVM minutes later. | ||
| */}} | ||
| {{- define "kbs-access-curl.image" -}} |
There was a problem hiding this comment.
Nit: this helper is copied word for word into hello-openshift, kbs-access-curl and kbs-access-sealed, so fixes like the double-prefix one need to be made three times. A small type: library chart, or one shared tpl, would avoid that.
| # an overlay list would replace this one wholesale. | ||
| overrides: | ||
| - name: acm.mce_operator.source | ||
| value: '{{ if $.Values.global.airgap }}cs-redhat-operator-index-v4-22{{ else }}redhat-operators{{ end }}' |
There was a problem hiding this comment.
This still hardcodes v4-22, and step 3e only checks the catalogs named in the overlay, so a 4.21 mirror passes validation but MCE is created on a CatalogSource that doesn't exist. {{ default "redhat-operators" $.Values.global.catalogSource }} should work in both modes and drop the if/else.
| d = yaml.safe_load(f) | ||
| print(d.get('global', {}).get('clusterVersion', '')) | ||
| " 2>/dev/null || true) | ||
| default_source="cs-redhat-operator-index-v${cluster_version//./-}" |
There was a problem hiding this comment.
If global.clusterVersion is empty (or the python read fails), this becomes cs-redhat-operator-index-v, and the GitOps subscription never resolves. Nothing reports an error. Step 3d already handles this case (fallback to the live CatalogSources, then a hard error); worth doing the same here.
| error " $s" | ||
| done <<< "$stragglers" | ||
| error "ArgoCD-owned entries are declarative (airgap overlay); others need the repoint fixed." | ||
| exit 1 |
There was a problem hiding this comment.
This exit 1 also fires for subs the loop skipped on purpose (ArgoCD-owned, or target catalog missing) and for day-0 subs unrelated to the pattern. In the full bootstrap that aborts before charts are mirrored or the pattern is deployed. Maybe warn during bootstrap and only fail hard in --repoint-subs mode?
| 2>/dev/null | ||
| } | ||
|
|
||
| sub_source() { # $1 = rendered manifest, $2 = subscription name |
There was a problem hiding this comment.
With set -o pipefail, echo "$1" | awk '…exit' (and the grep -q / head -1 pipes elsewhere) can close the pipe before echo finishes on a large manifest. The pipeline then returns 141 (SIGPIPE), so the gate either exits partway under set -e or reports a false FAIL. It's intermittent. Use here-strings (awk … <<< "$1") or drop the early exit.
| # NOTE: quay.io/community-operator-pipeline-prod removed — oc-mirror 4.22 | ||
| # generates ITMS for it in itms-oc-mirror.yaml automatically. | ||
| # Kyverno chart images: the chart pulls from reg.kyverno.io, not ghcr.io. | ||
| - source: reg.kyverno.io |
There was a problem hiding this comment.
The comment says the chart pulls from reg.kyverno.io, but charts/vendor/kyverno/values.yaml pins registry: ghcr.io for some images, and the imageset only mirrors ghcr.io/kyverno/*. Can you confirm which registry each kyverno image actually comes from, and that every one is both mirrored and covered by an ITMS/IDMS entry?
| local repointed=0 skipped=0 missing_catalog=0 | ||
| while IFS= read -r line; do | ||
| [[ -z "$line" ]] && continue | ||
| local def="${def:-}" mirror="${mirror:-}" |
There was a problem hiding this comment.
Nit: local def="${def:-}" mirror="${mirror:-}" is overwritten on the next two lines; local def mirror is enough. More generally, values-global.yaml is now read by several inline python3 -c 'import yaml…' snippets (here, step 3e, step 10), each with different error handling. A single read_global_key <path> <default> helper would keep them consistent.
yuvalk
left a comment
yuvalk
left a comment
There was a problem hiding this comment.
Re-review after the latest fixes (7efafc8)
Thanks for the quick turnaround. Most of the first round is resolved, and scripts/verify/render-gate.sh passes in both modes.
Resolved: the repoint loop now reads line by line and fails loudly; the overlay reaches the root app (thanks for the operator code pointer); no placeholder registry, with a render-time check and a script check against MIRROR_REGISTRY; vault chart pinned to 0.1.11; the overlay sets only single values, so no list replacement; org-path docs are aligned.
Partly resolved:
- Repoint vs ArgoCD: the re-runnable
make airgap-repoint-subsis good, but the ArgoCD-owned skip checks theinstancelabel while clustergroup 0.9.58 uses annotation tracking, so it never matches (inline). - Connected wave gating: now set declaratively in
values-global.yaml, but it depends on pattern-install ≥ 0.0.19. The comments cite both pattern-install-chart#52 and #53; can you confirm which one is right and whether it has been released? - Hardcoded
v4-22: the script now derives the suffix, but the ACM MCE source invalues-baremetal.yamlstill hardcodes it and step 3e doesn't check it (inline).
New, should fix before merge
- ArgoCD-owned skip never matches (above). Blocking.
- The image helper double-prefixes images that already point at the mirror.
- The step 10 fallback builds
cs-redhat-operator-index-vwhenclusterVersionis empty. - The leftover-subscriptions
exit 1aborts the full bootstrap, even for subs it skipped on purpose.
Minor: pipefail/SIGPIPE can make the render gate fail at random; please confirm the kyverno registry mapping; the image helper is copied into 3 charts; there are repeated inline python YAML reads.
⛔ Blocked by an upstream fix — do not merge yet
The sync-wave gating half of this PR depends on an upstream defect:
pattern-install(the chart everymake installuses) never rendersgitops.applicationHealthCheckEnabledinto thepatterns-operator-configConfigMap, so the operator's app-of-apps wave gating stays default-off on
every install path.
This PR must not merge until that chart PR is merged and released
(v0.0.19). Once it is, the gating set here via
main.gitops.applicationHealthCheckEnabled: "true"works on every installpath with no out-of-band steps. All validation runs below were executed
against the fixed chart (v0.0.19 built from the #53 branch); the
disconnected bootstrap additionally writes the ConfigMap key directly
(
scripts/airgap-post-install.shstep 10) as a bridge until the releaselands.
Summary
Follow-up to #163. During the October disconnected-deploy qualification runs, a series of airgap fixes were developed and verified live against disconnected clusters, then stranded on a local deploy branch when the testing branches were retired. This PR lands that work connected-first and mode-agnostic (19 commits): every mode-dependent value flips automatically with the existing
global.airgapflag, sovalues-baremetal.yamlrenders correctly in both modes and the connected/disconnected branch pair keeps a config-only delta. It also makes the pattern's sync-wave layout actually execute, which eliminates the workload/KBS race at the source — and bootstraps the ArgoCD controller's cluster-admin binding ahead of wave 0 so gated clean installs don't deadlock on the first sync.Airgap fixes (mode-agnostic)
airgap-post-install.shstep 3d): operators created at runtime reference the default catalog sources, disabled in airgap — observed live as MCH stuckInstallingfor hours on a missing CRD. The repoint is derived fromglobal.clusterVersion, verifies the target CatalogSource exists and is READY, and is re-runnable (make repoint-subs).vault:2.1.2-ubi.sharedValueFilespath so the overlay provably reaches the root render. Workload overrides are tpl-rendered againstglobal.mirrorRegistry, and rendering fails fast in airgap mode while it is empty or a placeholder.{{ if $.Values.global.airgap }}conditional on thecollateralServiceparameter. The previous overlaykbs.tdxblock was inert dead config (overlays cannot flip inline application parameters) and is removed.MIRROR_REGISTRY(unauthorized/404 otherwise);helmRepoUrlis required formultiSourceConfigin airgap (the repo-server performs no IDMS redirect).Sync-wave gating (the K17 race, eliminated at the source)
gitops.applicationHealthCheckEnabled: "true"invalues-global.yaml: ArgoCD dropped the Application health check in 1.8 (Child applications should not effect parent application's health by default argoproj/argo-cd#3781); the patterns-operator can restore it but defaults it off. Without it, child Applications have unknown health and the root app applies all sync waves ungated — observed live: all 15 child apps created within ~4 seconds, wave-30 workloads racing the wave-20 trustee/KBS, pods wedging on stale initdata and requiring manual deletion.operator.open-cluster-management.io/v1). Wave 15 places acm after the operator subscriptions and before trustee.acm.mce_operator.source): the MCH operator creates the multicluster-engine subscription at runtime from themce-subscription-specannotation, which the ACM chart templates from this value. Born on the mirrored catalog instead of the disabledredhat-operators— MCH reaches Running with no manual repoint.clusterGroup.extraObjects): ArgoCD applies wave-0 resources by kind, so the imperative ServiceAccounts are attempted before the cluster-admin ClusterRoleBinding that authorises the controller to create them — the applies failforbiddenin the first seconds of the first sync. Ungated installs self-heal on the next auto-sync; with gating the first operation parks on wave-0 health (ESO Degraded ← vault unconfigured ← imperative cronjobs cannot start ← SA missing) and the retry never fires — a hard deadlock on clean installs. A distinct-named cluster-admin binding rendered at sync-wave −1 fixes it (bridge, not a fix: patterns-operator#887 tracks the root cause; TODO marker included).Validation
Static: render gate (
scripts/verify/render-gate.sh, CI) renders the full values chain in both modes with clustergroup chart 0.9.58 — connected mode renders byte-identical operator defaults (certified-operators sources, Intel PCS collateral URL, no workload overrides); airgap mode renders the mirrored sources,file://collateral, and mirror-prefixed workload images. Values chains pass the chart'svalues.schema.json. No lab-specific hostnames, IPs, or credentials (placeholder conventions throughout).Live — two full clean-install disconnected runs against a certified Quay mirror (OCP 4.22.17, from-scratch agent installs, zero stalls):
Live — full lifecycles at the final branch state (both against the fixed pattern-install 0.0.19):
deploy/node-02-connected):pattern.sh make installexit 0; wave −1 bridge applied at first sync, imperative SAs created with zeroforbidden; wave ordering 0 (10:03) → 10 (10:20) → 15 (10:35) → 20 (10:35) → 30 (11:09); 4× attest-200 Affirming; workload images stillregistry.redhat.io/ubi9/...(mode-agnostic confirmed); collateral backend Intel PCS https (connected default).7efafc8via deploy branchdeploy/node-02-disconnected): fresh SNO installed from the Quay-mirror ISO, pattern served from the lab git HTTP server with all charts fromquay…:443/mirror/validatedpatterns:reconcile complete,make installexit 0; MCE sub born oncs-redhat-operator-index-v4-22, MCH Running, local-cluster registered; kyverno Running (reg.kyverno.io ITMS); ESO ClusterSecretStore Valid; trustee collateralfile://; 4× attest-200 Affirming, 9× resource-200; all five CVM workloads Running underkata-ccwith 0 restarts, pullingquay…:443/mirror/ubi9/httpd-24@sha256:68a91ff…(digest-exact) inside the VMs with the Quay CA in the initdataextra_root_certificatesand the pull credential fetched viakbs:///default/credential/regcred.CreateContainerErroronly until the trust chain (PCK cache + fresh RVPS refvals) was provisioned — then self-healed via kubelet retry, 0 restarts, no manual pod deletion. This also answers the earlier open question affirmatively: pods wedged on attestation-not-ready self-heal; only stale initdata content (the original K17) still requires pod recreation.global.originURLHelm parameter normalizes away on apply); root operations all Succeeded and every child is Synced+Healthy.Notes for reviewers
global.airgap: trueplus the bootstrap operator sources invalues-global.yaml(config-only branch delta); everything else in this PR is automatic.global.mirrorRegistrydefaults to empty (connected-inert) and is set at deploy time on the disconnected branch; rendering fails fast in airgap mode if it is empty or a placeholder.gitops.applicationHealthCheckEnableddefault-off in the patterns-operator is tracked upstream: patterns-operator-config ConfigMap omits gitops.applicationHealthCheckEnabled, so app-of-apps sync-wave gating is silently inert pattern-install-chart#52 (issue) and Enable use of ROSA or HCP (in general) clusters for OSC #53 (fix). This PR sets the value declaratively invalues-global.yamland keeps the bootstrap-path write in step 10 until the chart release lands; it is blocked on Enable use of ROSA or HCP (in general) clusters for OSC #53 merging.