Skip to content

Implement staged image tagging for safer deployments - #231

Open
kmandryk wants to merge 8 commits into
developfrom
AI-1607
Open

Implement staged image tagging for safer deployments#231
kmandryk wants to merge 8 commits into
developfrom
AI-1607

Conversation

@kmandryk

@kmandryk kmandryk commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Restructure deployment pipeline to use immutable SHA tags during build/deploy and only promote to floating tags after successful rollouts. This prevents partial image mix-ups from pod restarts.

Key changes:

  • Build matrix pushes only to SHA tags (-), not floating tags
  • Deploy verifies staged images exist before applying overlays
  • Rollouts must succeed before promotion to floating tag
  • Failed builds/deploys auto-cleanup staged tags via cleanup-on-failure job
  • New utility scripts: artifactory-login.sh (with retries), wait-for-rollouts.sh (with quota/diagnostic checks), plg-pre-delete.sh (delete immutable StatefulSets)
  • Artifactory API calls now have --connect-timeout 30 --max-time 120
  • Improved rollout diagnostics: pod status, FailedScheduling events, resource quota reporting
  • Updated documentation with staging model diagram and local/CI parity examples

Summary

AI-1607

Changes

Deploy Pipeline Hardening Plan

This ticket covers four independent problems in the same pipeline (.github/workflows/deploy-instance.yml). They share a common theme: partial or silent failure leaves the cluster or registry in an inconsistent state.

flowchart TD
  subgraph current [Current flow - risky]
    B1[Build matrix pushes floating tag] --> D1[Deploy uses floating tag]
    D1 --> R1[Rollout timeout = warning only]
    R1 --> C1[Cleanup only on full success]
  end

  subgraph proposed [Proposed flow]
    B2[Build pushes SHA tag only] --> D2[Deploy uses SHA tag]
    D2 --> R2[Rollout must succeed]
    R2 --> P2[Promote SHA to floating tag]
    P2 --> C2[Scheduled cleanup + rotation]
    B2 -.->|on failure| F2[Delete run SHA tags + orphan cleanup]
    D2 -.->|on failure| F2
    R2 -.->|on failure| F2
  end
Loading

Problem 1: Images should be staged, not immediately become the pull target

Root cause

Today, the build matrix pushes to the floating tag (bcgov-di, bcgov-di-test, or branch-derived tag) during build:

      - name: Compute image tags
        ...
            echo "${BASE}:${{ needs.metadata.outputs.image-tag }}"
            if [[ -n "${{ needs.metadata.outputs.sha-tag }}" ]]; then
              echo "${BASE}:${{ needs.metadata.outputs.sha-tag }}"
            fi

Deploy then references the same floating tag via Kustomize (--image-tag "${IMAGE_TAG}" at line 268). Because imagePullPolicy: Always is set on app containers, any pod restart during or after a partial build can pull a mix of new and old images across services. Prod already creates immutable SHA tags, but test/dev do not, and the floating tag is still overwritten before deploy succeeds.

Proposed staging model: build-to-SHA, deploy-from-SHA, promote-on-success

Phase Tag used Who references it
Build <instance>-<sha12> only Nothing running yet
Deploy + rollout <instance>-<sha12> Kustomize overlay + PLG chesAdapter.image.tag
Post-success promotion <instance> (floating) Updated via manifest copy, not rebuild
Rollback (prod) Keep last N <instance>-<sha12> tags Manual oc set image (existing docs)

Metadata changes (deploy-instance.yml metadata job, lines 47–95):

  • Always emit sha-tag for every environment:
    • developbcgov-di-test-${GITHUB_SHA::12}
    • mainbcgov-di-${GITHUB_SHA::12} (unchanged pattern)
    • workflow_dispatch${IMAGE_TAG}-${GITHUB_SHA::12}
  • Keep image-tag as the floating/live tag name (used only for promotion).

Build job changes:

  • Push only to sha-tag during docker/build-push-action (remove floating tag from build tags).
  • Add a build gate: after matrix completes, a lightweight verify-builds job (or deploy precondition step) confirms all four services (backend-services, frontend, temporal, ches-adapter) exist in Artifactory at the SHA tag before deploy proceeds.

Deploy job changes:

  • Pass needs.metadata.outputs.sha-tag to generate_instance_overlay and Helm --set chesAdapter.image.tag instead of image-tag.

New step: Promote images to floating tag (after successful rollout, before existing cleanup):

# For each service, point floating tag at the same manifest as the SHA tag
docker buildx imagetools create \
  "${REGISTRY}/kfd3-fd34fb-local/${svc}:${FLOATING_TAG}" \
  "${REGISTRY}/kfd3-fd34fb-local/${svc}:${SHA_TAG}"

Requires docker/setup-buildx-action in the deploy job (or a dedicated promote job with registry login).

Local script parity:

  • scripts/oc-build-push.sh: add --sha-tag or default to appending -$(git rev-parse --short=12 HEAD) for staging pushes; document --promote to update floating tag after manual deploy.
  • scripts/lib/image-tag.sh: add helper for SHA tag computation mirroring CI.
  • scripts/oc-deploy-instance.sh: accept --image-tag as deploy tag (already does); document that callers should pass SHA tag and promote separately.

Docs: Update docs-md/openshift-deployment/AUTO_DEPLOY.md tagging table and flow diagram.


Problem 2: Cleanup Artifactory artifacts on failure

Root cause

Cleanup runs only on full success:

      - name: Artifactory cleanup (reclaim storage)
        if: success()
        continue-on-error: true

Failed or partial builds leave SHA manifests, orphan sha256__* folders, and stale _uploads/ blobs until a later successful run.

Proposed changes

New script scripts/artifactory-delete-run-tags.sh:

  • Inputs: --tag <sha-tag>, --delete, service list (default: all four images).
  • Deletes named tag ${image}:${sha-tag} via Artifactory Docker API (reuse DELETE pattern from scripts/artifactory-cleanup.sh lines 200–207).
  • After tag deletion, invoke artifactory-cleanup.sh --delete (orphan + uploads phases) to reclaim unreferenced layers.

New workflow job: cleanup-on-failure:

cleanup-on-failure:
  needs: [metadata, build, deploy]
  if: always() && (needs.build.result == 'failure' || needs.deploy.result == 'failure')
  steps:
    - delete run SHA tags (if any were pushed)
    - artifactory-cleanup.sh --delete
  • continue-on-error: true so cleanup itself does not mask the original failure.
  • On partial matrix failure: only services that successfully pushed will have the SHA tag; delete script is idempotent for missing tags.

Keep existing success-path cleanup (prod rotation --keep 3 --match) unchanged, running after promotion.

Docs: Add section to scripts/ARTIFACTORY_README.md.


Problem 3: "Delete immutable PLG StatefulSets before upgrade" — logging disruption?

The step at lines 446–463 deletes loki and prometheus StatefulSets with --cascade=orphan:

Layer Disruption?
Application stdout / PVC logs No — apps and tee sidecars keep writing
Promtail → Loki ingest Brief yes — during subsequent helm upgrade --wait, Loki may restart; Promtail buffers or backs off
Grafana log viewing Brief yes — Grafana uses Recreate strategy
Historical Loki data No — PVCs preserved

The delete step itself is low-disruption (orphan keeps pods running); disruption occurs during Helm reconciliation, not at delete time.

Proposed changes (documentation + small hardening, not removal)

  1. Document in docs-md/PLG_DEPLOYMENT_INTEGRATION.md a new "Upgrade impact on logging" section with the table above and expected duration (Helm --timeout 300s bound).
  2. Add alertmanager to the pre-delete loop — it also has immutable volumeClaimTemplates in deployments/openshift/helm/plg/templates/alertmanager-statefulset.yaml but is currently omitted.
  3. Parity fix: Port the pre-delete step into scripts/oc-deploy-instance.sh before its Helm upgrade (local deploys currently lack this workaround and can fail on PVC size changes).
  4. Optional guard (low priority): only run pre-delete when Helm values change loki.pvcSize or prometheus.pvcSize vs current StatefulSet spec — avoids unnecessary Loki restarts on no-op upgrades. Defer unless scope allows.

No change to PLG being required for deploy; the ticket asks whether it causes disruption


Problem 4: Intermittent "Login to Artifactory" timeouts

Root cause

docker/login-action (line 135) has no retry and no explicit timeout. The error (Client.Timeout exceeded while awaiting headers) is a transient network/registry issue.

Proposed changes

CI — wrap login with retry using nick-fields/retry (or equivalent shell loop):

- name: Login to Artifactory
  uses: nick-fields/retry@v3
  with:
    timeout_minutes: 2
    max_attempts: 3
    retry_wait_seconds: 15
    command: |
      echo "${{ secrets.ARTIFACTORY_SA_PASSWORD }}" | docker login \
        "${{ secrets.ARTIFACTORY_URL }}" \
        -u "${{ secrets.ARTIFACTORY_SA_USERNAME }}" \
        --password-stdin

Apply the same pattern to the deploy job if/when it needs registry access for imagetools create promotion.

Local — add retry loop (3 attempts, 15s backoff) around docker login in scripts/oc-build-push.sh.

Artifactory API scripts — add --connect-timeout 30 --max-time 120 to curl calls in scripts/artifactory-cleanup.sh and scripts/artifactory-usage.sh for consistency.


Problem 5: Rollout failures fail silently when namespace lacks resources

Root cause

              oc rollout status ... --timeout=300s || \
                echo "::warning::Rollout timed out for $DEPLOY"

Same pattern in scripts/oc-deploy-instance.sh lines 451–452. Timeouts and quota exhaustion (FailedScheduling, Pending pods) produce warnings only; the job exits 0 and prints success URLs — contradicting US-007 acceptance criteria.

Proposed changes

Extract shared rollout waiter — new scripts/lib/wait-for-rollouts.sh:

wait_for_rollouts() {
  local namespace="$1" instance="$2"
  shift 2
  local services=("$@")
  local failed=()

  for svc in "${services[@]}"; do
    local deploy="${instance}-${svc}"
    if ! oc get deployment "$deploy" -n "$namespace" &>/dev/null; then continue; fi
    if ! oc rollout restart "deployment/$deploy" -n "$namespace"; then
      failed+=("$deploy:restart")
      continue
    fi
    if ! oc rollout status "deployment/$deploy" -n "$namespace" --timeout=300s; then
      failed+=("$deploy:timeout")
    fi
  done

  if [[ ${#failed[@]} -gt 0 ]]; then
    # Emit diagnostics: pod phases, waiting reasons, recent FailedScheduling events
    diagnose_rollout_failures "$namespace" "$instance"
    return 1
  fi
}

Diagnostics on failure (reuse patterns from scripts/oc-list-instances.sh determine_instance_status):

  • oc get pods -l app.kubernetes.io/instance=<instance> -o wide
  • oc get events --field-selector reason=FailedScheduling
  • oc describe resourcequota when quota-related events found
  • GitHub: emit ::error:: (not just ::warning::) with deploy name

Wire into:

Pre-deploy quota check (recommended, lightweight):

Ordering with image staging: Promotion to floating tag must happen after rollout succeeds, so a failed rollout never updates the live pull tag.


Testing plan

Area Verification
SHA staging Push to develop, confirm build pushes only bcgov-di-test-<sha>, deploy uses SHA, floating tag unchanged until rollout succeeds, then promoted
Partial build failure Simulate one matrix service Dockerfile failure; confirm deploy skipped, floating tag unchanged, cleanup job deletes partial SHA tags
OC restart safety After build completes but before promote, manually oc delete pod on one deployment; confirm it pulls previous floating tag, not staged SHA
Artifactory login Temporarily block registry (or use retry with bad URL in a branch test); confirm 3 attempts before failure
Rollout quota Deploy to a namespace at quota limit; confirm job fails with ::error:: and scheduling events in log
PLG upgrade Run deploy with DEPLOY_PLG=true; confirm Loki brief unavailability documented; verify alertmanager included in pre-delete
Local scripts oc-build-push.sh + oc-deploy-instance.sh with SHA tag flow

Checklist

By submitting this pull request, I acknowledge that I have attempted to meet the following:

  • a self-review of my code
  • commented code particularly in hard-to-understand areas
  • corresponding changes to the documentation where required
  • changes tested to the best of my ability
  • no new errors or non-functional code

Restructure deployment pipeline to use immutable SHA tags during build/deploy and only promote to floating tags after successful rollouts. This prevents partial image mix-ups from pod restarts.

Key changes:
- Build matrix pushes only to SHA tags (<floating>-<sha12>), not floating tags
- Deploy verifies staged images exist before applying overlays
- Rollouts must succeed before promotion to floating tag
- Failed builds/deploys auto-cleanup staged tags via cleanup-on-failure job
- New utility scripts: artifactory-login.sh (with retries), wait-for-rollouts.sh (with quota/diagnostic checks), plg-pre-delete.sh (delete immutable StatefulSets)
- Artifactory API calls now have --connect-timeout 30 --max-time 120
- Improved rollout diagnostics: pod status, FailedScheduling events, resource quota reporting
- Updated documentation with staging model diagram and local/CI parity examples
check_namespace_quota compared hard/used quota values with
float(str(v).rstrip('m')), which is unsound because ResourceQuota
status does not guarantee matching units between `hard` and `used`.

Verified against live fd34fb-test:
- requests.cpu hard=4 used=3230m was read as 3230/4 = 80750%, a false
  positive that failed the "Restart deployments and wait" step (and via
  cleanup-on-failure could delete the just-applied SHA tag) on every
  deploy.
- requests.memory (Gi/Mi) and requests.storage (64Gi/64Gi, genuinely
  100% full) raised ValueError and were silently skipped, so the actual
  resource-exhaustion case this check targets went undetected.

Replace with a proper resource.Quantity parser handling binary
(Ki..Ei) and decimal SI (n,u,m,k,M,G,T,P,E) suffixes, normalizing both
sides to a common base before comparing. Unknown/unparseable values are
skipped rather than miscompared.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alex-struk

Copy link
Copy Markdown
Collaborator

Pushed a fix for the namespace quota parser (5e3f7aeb).

Bug: check_namespace_quota in scripts/lib/wait-for-rollouts.sh compared hard/used with float(str(v).rstrip('m')). Kubernetes ResourceQuota status doesn't guarantee matching units between hard and used, so this is unsound in both directions. I verified against the live fd34fb-test namespace (the one develop deploys to):

  • False positive (breaks the happy path): requests.cpu is hard=4, used=3230m — genuinely 80.75%. rstrip('m') reads it as 3230 / 4 = 80750%, tripping the ≥95% guard. Since check_namespace_quota runs first in wait_for_rollouts, the "Restart deployments and wait" step fails before restarting anything — and because that's after the overlay oc apply, cleanup-on-failure then deletes the just-applied SHA tag. On current cluster state this would fail every develop deploy.
  • False negative (misses the actual target case): requests.memory (5736Mi/16Gi) and requests.storage (64Gi/64Gi — genuinely 100% full) end in i, so float("64Gi") raises ValueError and the resource is silently skipped. The storage-exhaustion scenario this check is meant to catch (Problem 5) went undetected.

Fix: proper resource.Quantity parsing — binary suffixes (Ki…Ei) and decimal SI (n,u,m,k,M,G,T,P,E) normalized to a common base before comparing; unparseable values are skipped rather than miscompared. Re-verified against the live quota values: 3230m/4 → 80.75%, 5736Mi/16Gi → 35%, 64Gi/64Gi → 100% (now correctly flagged).

This is scoped to the parsing bug only. Two related items from my review are still open and I left them untouched here:

  1. check_namespace_quota evaluates all quotas in the namespace. In shared fd34fb-test, a quota that's near/at limit due to other instances (e.g. storage-quota at 64Gi/64Gi) will now correctly-but-perhaps-undesirably block a deploy whose rollout-restart requests no new storage. Worth deciding whether the pre-check should scope to resources the rollout actually consumes.
  2. cleanup-on-failure deletes the run's SHA tag even after the deploy already oc apply'd Deployments pinned to it, risking ImagePullBackOff on a post-apply failure.

kmandryk and others added 2 commits July 8, 2026 09:37
cleanup-on-failure deleted the run's SHA tag whenever build OR deploy
failed. But the deploy job runs `oc apply` early (step ~3 of ~12),
pinning the live Deployments to `:<sha-tag>`. A failure in any later
step (PLG helm timeout, rollout timeout, promote, secret creation) then
triggered deletion of the tag those applied Deployments reference —
causing ImagePullBackOff on the next pod restart with no floating
fallback, since Deployments no longer reference the floating tag.

Restrict cleanup to build failures. A build failure means `deploy` was
skipped, so `oc apply` never ran and the SHA tag is referenced by
nothing in the cluster — safe to delete (this is also the primary
Problem-2 case: orphan manifests from a partial matrix push). Deploy
failures are excluded: the tag is either in use (must keep) or, for a
pre-apply verify failure, reclaimed by the routine --keep rotation on
the next successful run.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alex-struk

Copy link
Copy Markdown
Collaborator

Pushed a fix for the second issue from the review — cleanup-on-failure deleting an already-applied SHA tag (63c09013).

Problem: the deploy job runs oc apply early (step ~3 of ~12), which pins the live Deployments to :<sha-tag>. cleanup-on-failure fired on build or deploy failure, so a failure in any later deploy step — PLG helm --wait timeout, rollout timeout (the Problem-5 case), the un-retried promote imagetools create, even a secret-creation hiccup — would delete the exact tag those applied Deployments reference. With imagePullPolicy: Always and no floating fallback (Deployments no longer reference the floating tag), the next pod restart → ImagePullBackOff, and the follow-on orphan cleanup can GC the layers too.

Fix: gate cleanup to build failures only:

needs: [metadata, build]
if: always() && needs.build.result == 'failure'

A build failure means deploy was skipped, so oc apply never ran and the SHA tag is referenced by nothing in the cluster — safe to delete. This is also the primary Problem-2 case (orphan manifests from a partial matrix push, since fail-fast: false). Deploy failures are deliberately excluded: the tag is either in use (must keep) or, for a rare pre-apply verify failure, reclaimed by the routine --keep rotation on the next successful run — self-healing, never a safety issue.

Both review fixes are now on the branch:

  1. 5e3f7ae — quota-quantity parser (High)
  2. 63c09013 — cleanup-on-failure gate (this one)

Still open (lower priority, untouched):

  • The quota pre-check evaluates all namespace quotas; in shared fd34fb-test a full storage-quota (currently 64Gi/64Gi, driven by other instances) will now correctly-but-perhaps-undesirably block a rollout-restart that requests no new storage. Worth scoping the check to resources the rollout actually consumes, or dropping it in favor of the oc rollout status --timeout hard-fail.
  • Retry hardening only wraps docker login; the verify-images curl and promote imagetools create can still hit the intermittent registry timeout (Problem 4).

check_namespace_quota gated the deploy on every ResourceQuota in the
namespace being under 95%. In the shared fd34fb-test namespace this
produces false blocks: a quota can be at its limit because of other
instances (e.g. storage-quota at 64Gi/64Gi), and a rollout-restart of
already-sized deployments requests no new storage — so the gate would
fail a deploy for resources it never consumes.

Rely on `oc rollout status --timeout=300s` instead: genuine resource
exhaustion (pods stuck Pending / FailedScheduling) surfaces as a hard
rollout-timeout failure with full diagnostics, which is the correct and
sufficient fix for the "fails silently" problem. This also removes the
quota-quantity parser added earlier, since the function it lived in is
gone.

Right-sizing namespace capacity (HPA tuning so instances don't cap out
and don't scale unnecessarily) is tracked separately.

Docs updated to drop the pre-check claim while keeping the
rollout-timeout / diagnostics behaviour.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alex-struk

Copy link
Copy Markdown
Collaborator

Removed the pre-rollout namespace quota check (d389c576).

Why: even with correct quantity parsing, check_namespace_quota gated the deploy on every ResourceQuota in the namespace being under 95%. In shared fd34fb-test that produces false blocks — a quota can be at its limit because of other instances (e.g. storage-quota at 64Gi/64Gi), and a rollout-restart of already-sized deployments requests no new storage, so the gate would fail a deploy for resources it never consumes.

Instead: rely on oc rollout status --timeout=300s. Genuine resource exhaustion (pods stuck Pending / FailedScheduling) surfaces as a hard rollout-timeout failure with full diagnostics (diagnose_rollout_failures still dumps pod status, FailedScheduling events, and oc describe resourcequota). That's the correct and sufficient fix for the original "fails silently" problem (Problem 5) — the pre-check was only ever an early-warning layer on top of it. A failed deploy also leaves the old pods serving and, thanks to the build-only cleanup gate, keeps the SHA tag, so the rollout self-completes once capacity frees up.

This also supersedes the earlier quota-quantity parser fix (5e3f7ae) — the function it lived in is now gone.

Follow-up: the real capacity fix (right-sizing HPAs so instances don't cap out the namespace, and tuning them so they don't scale up unnecessarily) is tracked in AI-1700.

Docs (AUTO_DEPLOY.md, PLG_DEPLOYMENT_INTEGRATION.md, MANUAL_LOAD_TEST_INSTANCE.md) updated to drop the pre-check claim while keeping the rollout-timeout/diagnostics behaviour.

Remaining open item from the review (lower priority): retry hardening only wraps docker login — the verify-images curl and promote imagetools create can still hit the intermittent registry timeout (Problem 4).

alex-struk and others added 2 commits July 10, 2026 15:28
Resolve docs-md/operations/AUTO_DEPLOY.md (relocated from
openshift-deployment/ on develop): keep the staged-image deploy flow,
fold in develop's accurate operational detail (secret names, alert-rule
generation, migrate-db init container, DEPLOY_PLG gating), and correct
the cleanup-on-failure description + staging diagram to match the
build-only cleanup gate.
Problem 4 (intermittent Artifactory `Client.Timeout`) previously only
wrapped `docker login`. The deploy job's staged-image existence check
and the `docker buildx imagetools create` promotion still made
un-retried registry calls, so a transient timeout there failed the
deploy.

Add a shared `with_retries` helper (scripts/lib/retry.sh, 3 attempts /
15s backoff) and use it for:
- the verify-images check (accepts only HTTP 200; retries transient
  000/5xx, still fails fast-enough on a genuinely-missing 404 after
  attempts are exhausted),
- the CI promote step, and
- the local `oc-build-push.sh --promote` path (CI/local parity).

Docs updated to describe the broadened retry coverage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alex-struk

Copy link
Copy Markdown
Collaborator

Finished Problem 4 — the retry hardening now covers the remaining un-retried registry calls (2108f70b).

Previously only docker login was wrapped. Added a shared with_retries helper (scripts/lib/retry.sh, 3 attempts / 15s backoff) and applied it to:

  • Verify staged images (deploy job): the manifest existence check now only accepts HTTP 200 and retries transient failures (000 from a curl timeout, or 5xx); a genuinely-missing image (404) still fails once attempts are exhausted.
  • Promote (docker buildx imagetools create) in both the CI deploy job and the local oc-build-push.sh --promote path (CI/local parity).

Verified the helper with unit-style tests (immediate success, permanent failure propagating the real exit code, and flaky fail-twice-then-succeed) — which caught a bug in the first draft where the exit status was captured after fi (yielding the if's status, 0) instead of in an else, so permanent failures wrongly reported success; fixed. Docs updated (AUTO_DEPLOY.md, ARTIFACTORY_README.md).

With this, all five ticket problems are addressed in code. Remaining items are the two the PR body explicitly defers (Problem 3 pvcSize-change guard; Problem 5 rotate-prod-secrets.sh rollout-wait), plus the HPA capacity follow-up tracked in AI-1700. The only merge gate now is review approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants