ci(release): reuse verified pipeline for IDC images - #722
Conversation
37d7c07 to
981babc
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 51d58a7.
Requesting changes for the publication error-handling regression below. The native build -> digest smoke -> manifest assembly dependency chain is sensible, and separating candidates/cache from the runtime repository is a useful improvement. The final publication step, however, no longer preserves the existing fail-closed immutable-tag contract.
I executed the workflow's actual "Publish or verify immutable IDC image" Bash block with a stateful fake crane, without contacting a registry. With a target already pointing to another digest:
- A successful target lookup correctly exits 1 without copying.
- A TLS handshake timeout, HTTP 403, or HTTP 503 on that lookup causes crane copy to run, replaces the existing target in the fixture, and exits 0.
The equality check after copying validates the newly overwritten value, so it cannot detect that immutability has already been violated. Please restore the distinction between confirmed absence and an unreadable target before publication.
Validation:
- All 15 tests in scripts/ci/test_release_build_shells.py passed.
- test_release_manifest_contract.sh and test_release_candidate_tags.sh passed.
- git diff --check passed.
- GitHub Test Suite, Static Checks, and PR Title are green for this head.
- The broader local repository validator failed in the unchanged scripts/dev/test_edge_process_contract.sh; I am not attributing that failure to this PR.
- Verification here was offline; I did not independently repeat the reported live IDC build/smoke/publication run.
The new topology test checks string presence and therefore misses this executable failure path. A regression should run the real publisher with target states absent, identical, conflicting, and unreadable; only confirmed absence may invoke copy, and errors must preserve the prior tag.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of df63766.
The revision is materially better: publication now has an executable owner, and the added behavioral test correctly covers the previously reported plain TLS/401/403/503 failures, an absent target, and matching/conflicting digests. Those earlier examples should not be treated as unchanged defects.
However, the immutable-tag guarantee is still bypassable through a different failure path: the new classifier treats a 404 from the authentication service as proof that the image tag is absent. I reproduced an actual overwrite with the checksum-verified crane v0.20.6 binary used by the workflow and two isolated local HTTP registry fixtures. No fake crane was involved in this final reproduction, and no external registry was modified.
Reproduction:
- The source serves verified manifest A; the target already serves a different manifest B under the release tag.
- During the initial target digest lookup, /v2/ returns a Bearer challenge and the token endpoint returns 404 for both crane's HEAD attempt and GET fallback. The terminal error identifies GET /token?...: unexpected status code 404 Not Found.
- The token-service fault clears before copy. The script interprets the earlier authentication error as absence and invokes crane copy.
- The target records PUT /v2/team/astra/manifests/release, changes from B to A, and the publisher exits 0. Its post-copy digest check passes after the overwrite.
This remains a blocking correctness issue; please distinguish an explicit missing target manifest from failure to authenticate or reach the registry. The inline comment identifies the unsafe classification and the regression needed.
I also reviewed the complete seven-file PR diff, source/controller selection, credential boundary, native platform matrix, digest artifact flow, smoke-to-stage-to-publish dependencies, and the differences from the existing release candidate workflow. I found no additional independently substantiated blocker in those areas.
Validation on this head:
- python3 scripts/ci/test_release_build_shells.py: all 16 tests passed.
- bash scripts/ci/test_release_manifest_contract.sh: passed.
- bash scripts/ci/test_release_candidate_tags.sh: passed.
- git diff --check against the PR base: passed.
- Actual publisher plus real crane/local registry failure-recovery reproduction: demonstrated the overwrite described above.
I did not rerun the full repository gate or the live IDC build/smoke/publication pipeline. The reported live AMD64 success does not establish this failure-path property.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of d6e7ecc.
The previous P1 error-classification issue is fixed in the tested cases. Requiring successful tag enumeration removes the unsafe interpretation of token-service diagnostics. With the checksum-verified crane v0.20.6 binary and the actual publisher, I verified that token-service 404, tag-list 401/403/503, an existing target's digest-read failure, a conflicting tag on a later pagination page, and a failed later page all stop without a manifest PUT or modification of the existing target. Missing tags in an existing repository publish correctly, and identical targets remain idempotent.
Requesting changes for a separate P2 first-publication regression introduced by that prerequisite: a new runtime repository can legitimately return 404/NAME_UNKNOWN from its tags endpoint until its first image is pushed. The workflow never initializes that repository: all preceding build/stage writes go to the separate -candidates repository. Consequently, configuring a fresh CONTAINER_MIRROR_IMAGE can complete build/smoke/staging but cannot publish its first image.
Independent reproduction used the real crane binary against isolated local HTTP registry fixtures. For a fresh target, /v2/ succeeded and /v2/team/astra/tags/list returned the Distribution API's 404 NAME_UNKNOWN response. The actual publisher exited 1 with 'could not safely enumerate tags' and performed no PUT. A control invocation of crane copy against the same source and fresh target succeeded and created the verified manifest. No external registry was modified. This missing-repository case is distinct from the existing test's 'missing' state, which always returns a successful list containing other-tag.
Please provide a safe, explicit bootstrap path for a genuinely absent runtime repository without restoring generic stderr/404 matching. The previous P1 protection should remain intact. The inline comment describes the regression and coverage needed.
Scope: rechecked the full seven-file diff and surrounding controller/source selection, credentials, native platform builds, digest artifacts, smoke/stage/publication dependencies, and configuration documentation; no additional independently substantiated blocker found.
Validation on this head:
- All 16 release-shell tests passed.
- Release manifest and candidate-tag contract scripts passed.
- git diff --check against the PR base passed.
- Real crane/local registry cases verified missing/equal/conflicting targets, read failures, pagination, token-service failure, and the fresh-repository failure described above.
- The broader repository validator failed in unchanged scripts/dev/test_edge_process_contract.sh; I am not attributing that failure to this PR.
- The combined-status API returned no entries, so I cannot claim all CI checks are green. I did not rerun the live IDC build/smoke flow.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of 4ef88a3.
The previous first-publication P2 is fixed for the tested single-component Harbor repository. Moving the existence decision to Harbor's exact artifact endpoint also preserves the earlier fail-closed behavior for the ordinary cases: the actual publisher rejects structured 401/403/5xx, malformed 404, network failure, and conflicting digests; a structured artifact NOT_FOUND permits a fresh repository or missing tag; an identical digest is idempotent.
Requesting changes for one P1 integrity issue in the new Harbor lookup. The workflow validation permits nested repository paths, but inspect-harbor-artifact.py URL-encodes the repository name only once. Harbor documents that a repository parameter containing / must be double encoded (a/b -> a%252Fb); see the Harbor maintainers' confirmed guidance in goharbor/harbor#18355 and the documentation correction in goharbor/harbor#19635. For target registry/team/nested/astra:release, this code requests .../repositories/nested%2Fastra/artifacts/release, not the required nested%252Fastra.
That is not only an availability problem. Because every structured Harbor 404/NOT_FOUND is interpreted as proof that the target artifact is absent, a route/repository miss caused by the wrong path authorizes crane copy against the correctly parsed OCI target. I reproduced the full outcome with the actual publisher and checksum-verified crane v0.20.6 against an isolated Harbor/registry HTTP fixture: the Harbor API exposes the existing digest at the documented double-encoded path and returns its structured NOT_FOUND envelope for the single-encoded path; the publisher then performs PUT /v2/team/nested/astra/manifests/release, replaces the pre-existing conflicting digest, and exits 0. No external registry was modified.
The inline comment gives the required fix and regression coverage. I reviewed the complete eight-file PR, including the source/controller boundary, credential use, native build matrix, digest artifacts, smoke/stage/publish ordering, fresh-repository behavior, and the new structured inspector. I found no additional independently substantiated blocker.
Independent validation on this head:
- All 16 release-shell tests passed (with the existing Python SyntaxWarning for
digest\\ *). - release manifest and candidate-tag contract scripts passed.
- git diff --check against the PR base passed.
- Real-crane/local-registry validation reproduced the nested-repository overwrite described above.
- The combined-status API returned no entries, so I cannot claim all CI checks are green.
I did not rerun the live IDC build/smoke/publication flow or the full repository validator in this run.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of a807ec3. No blocking findings remain.
The nested Harbor repository fix addresses the previous P1 at the correct ownership boundary. The inspector now double-encodes the repository-name path parameter while keeping the project and artifact reference separately encoded. The regression test exercises a nested repository, asserts the exact raw nested%252Fastra request path, returns an existing conflicting digest, and proves that the publisher does not invoke copy.
I reran the previous real-crane reproduction against this head using the checksum-verified crane v0.20.6 binary and an isolated Harbor/registry HTTP fixture. The publisher requested the documented double-encoded artifact path, received the existing conflicting digest, exited nonzero, performed no registry PUT, and preserved the original target manifest. This directly reverses the overwrite reproduced on 4ef88a3.
I also re-reviewed the complete eight-file PR from first principles. The current design has a coherent trust and identity chain: the current protected controller revision selects an immutable source SHA; native per-platform builds publish by digest to the IDC-only candidate repository; smoke tests consume those exact digests; staging reconciles the verified platform set; and final publication uses Harbor's structured artifact response before copying the verified manifest. The runtime repository does not receive build caches or pre-verification candidates. The fresh-repository path is supported without returning to ambiguous crane stderr classification.
The unhappy-path contract is now meaningful at the publisher layer: missing/fresh targets copy, identical targets are idempotent, conflicting targets reject without copy, and 401/403/5xx, malformed 404, network failure, and nested-repository conflicts fail closed. The old token-service-404 and generic digest-lookup overwrite paths are no longer used for the publication decision.
Independent validation on this head:
python3 scripts/ci/test_release_build_shells.py: all 16 tests passed.test_release_manifest_contract.shandtest_release_candidate_tags.sh: passed.- Python compilation of the inspector/test module and Bash syntax validation of the publisher: passed.
git diff --checkagainst the PR base: passed.- Real crane/local Harbor-registry nested-conflict reproduction: original target preserved, no PUT.
Limits:
scripts/ci/validate_repository.pystill fails in the unchangedscripts/dev/test_edge_process_contract.sh; I am not attributing that failure to this PR.- The combined-status API returned no status entries, so this review does not claim all CI checks are green.
- I did not independently rerun the live IDC build/smoke/publication flow or the full multi-architecture path.
Merge Queue Status
This pull request spent 15 minutes 50 seconds in the queue, including 15 minutes 31 seconds running CI. Required conditions to merge
|
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of f13db65. No blocking findings remain.
This commit only merges current main into the branch. Relative to the previously approved a807ec33, the resulting delta is limited to upstream dependency lockfile updates and the already-merged actions/upload-artifact@v7.0.1 pin in two release workflows; the IDC publication implementation and its tests are unchanged. I rechecked the merged result rather than relying on the previous approval.
Executed validation:
python3 scripts/ci/test_release_build_shells.py: all 16 tests passed. The publisher contract exercises confirmed-missing/new repositories, identical targets, conflicting targets (including a nested Harbor repository), HTTP 401/403/503, malformed 404, and an unreachable registry. Copy occurs only for confirmed absence; identical targets are no-ops; conflicting and unreadable targets fail without calling copy.- A separate fixture using real
crane v0.20.6confirmed that a nested conflicting target is queried asnested%252Fastra, exits non-zero, performs no registry PUT, and preserves the original digest. test_release_manifest_contract.sh,test_release_candidate_tags.sh, Python compilation, shell syntax, and diff whitespace checks passed.
GitHub's Static Checks and Test Suite were still running at review time. I did not execute the live IDC or multi-architecture workflows. The repository-wide validator still reports the pre-existing, unchanged scripts/dev/test_edge_process_contract.sh: contract failed (); no files in that contract changed in this PR.
2 participants
Summary
Fix the manual IDC image workflow after its self-hosted BuildKit failed while pulling Dockerfile base images. Reuse the current release pipeline's native per-platform build, digest-based smoke verification, manifest assembly, and immutable publication flow, while publishing only to IDC.
The workflow supports Linux AMD64 (default) or the existing AMD64+ARM64 matrix. Final publication resolves the exact target through Harbor's structured artifact API: a validated artifact
NOT_FOUNDpermits first publication, while every authentication, authorization, network, malformed-response, and registry failure stops before copy. Nested Harbor repository names are double encoded at the API routing boundary so lookup and OCI publication always address the same repository.Failed run being fixed: https://github.com/matrixorigin/Astra/actions/runs/34051357192
Related issue
N/A
Change type
User and compatibility impact
build_push_to_idckeeps its existing manual entrypoint and source selection. It now offersamd64andall, uses native GitHub-hosted platform runners, and writes only to IDC. Docker Hub releases and their topology are unchanged. Existing IDC variables and Environment credentials remain valid; the self-hosted IDC runner variable is no longer used. A fresh runtime repository is created on first publication. Existing matching tags are idempotent; conflicting or unreadable tags are never overwritten.Architecture and complexity delta
.github/workflows/build_push_to_idc.ymlremains the IDC publication controller;.github/workflows/idc-container-candidates.ymlis the IDC destination copy of the established release-candidate workflow;scripts/copy-immutable-container-tag.showns final cross-repository immutable-tag reconciliation;scripts/inspect-harbor-artifact.pyowns structured Harbor artifact lookup.release.yml,release-container-candidates.yml, manifest reconciliation scripts, the merged IDC workflow, the failed IDC run, repository validation, and release documentation.craneerror text from the final publication decision.Verification
python3 scripts/ci/test_release_build_shells.pypassed (16 tests);python3 scripts/ci/validate_repository.pypassed with Python 3.12; actionlint v1.7.12 passed for both IDC workflows;git diff --check, Python compilation, and Bash syntax validation passed. The preceding head also passed the complete GitHub Test Suite and Static Checks.a807ec33, the current AMD64 workflow was run locally in its real order with sourcee5547ffae43391aa068069889863070898a4ca4e: Docker Hub base-image resolution, Buildx build with provenance/SBOM, first-time IDC cache miss and cache upload, digest candidate push, run-scoped candidate tag, pull back from IDC, all-in-one MatrixOne/Memoria/Astra startup, image label checks, readiness/health and exact memory round trip, staged manifest assembly, and final immutable publication. The fresh nested runtime repositorymocloud/pr722/astra-fullrunpublished and then retried idempotently at digestsha256:4f112c9d9c39e27ddff16cfece14460981c8417cabc91ada582daec344ed9faf.nested%252Fastraand proves an existing conflicting digest cannot invoke copy.Final checklist