Repository navigation
fix(ev2): retain global identity retry budgets (AROSLSRE-2101) - #6939
openshift-merge-bot[bot] merged 5 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The workspace-wide Azure SDK impact needs intentional rollout and validation, and required reviewers still need to be tagged.
Pull request overview
Adds bounded Microsoft Graph owners-relationship retries for global identity deployments while preserving retry budgets during fail-fast execution.
Changes:
- Adds retry handling for
mise-identityin global and staging pipelines. - Enables retry-budget honoring for both identity steps.
- Updates pipeline tooling dependencies and checksums.
File summaries
| File | Description |
|---|---|
tooling/templatize/go.sum |
Updates dependency checksums. |
tooling/templatize/go.mod |
Updates tooling and transitive Azure SDK dependencies. |
tooling/pipeline-documentation/go.sum |
Updates dependency checksums. |
tooling/pipeline-documentation/go.mod |
Updates pipeline dependencies. |
tooling/helmtest/go.sum |
Updates dependency checksums. |
tooling/helmtest/go.mod |
Updates tooling and transitive Azure SDK dependencies. |
dev-infrastructure/global-pipeline.yaml |
Updates global identity retry configuration. |
dev-infrastructure/global-pipeline-stg.yaml |
Updates staging identity retry configuration. |
Review details
Suppressed comments (3)
dev-infrastructure/global-pipeline.yaml:223
- The PR checklist still leaves “Specific reviewers tagged” unchecked, while CONTRIBUTING.md:124-126 requires tagging the owners/SMEs for the changed pipeline and any affected cross-team owners. Please tag the relevant reviewers before approval.
honorRetryBudgetWhenFailingFast: true
tooling/helmtest/go.mod:23
- This indirect SDK bump has the same workspace-wide effect:
tooling/helmtestis listed ingo.work, so its resolvedazcore/azidentityversions are selected for unrelated modules even though most module manifests remain on 1.21.1/1.13.1. This conflicts with the repository's explicit workspace-isolation rationale atgo.work:21; please make the rollout intentional and validate all affected modules, or avoid introducing these versions here.
github.com/Azure/azure-sdk-for-go/sdk/azcore v1.23.0 // indirect
github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.14.0 // indirect
tooling/templatize/go.mod:19
- These SDK upgrades now enter the main Go workspace through
templatize(and the resolved indirect graph inhelmtest).go.work:21deliberately keeps the same newerazcore/azidentityversions out because workspace selection raises them for every module, while modules such asbackend/go.mod:7-8still pin the older versions. Please either isolate this parser dependency or deliberately roll out and validate the workspace-wide SDK upgrade; otherwise unrelated services silently build against new Azure SDKs.
github.com/Azure/azure-sdk-for-go/sdk/azcore v1.23.0
github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.14.0
- Files reviewed: 5/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
da06759 to
5fab623
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved moderate findings remain regarding ineffective STG retry blocks and the retry count providing two rather than three retries.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
dev-infrastructure/global-pipeline-stg.yaml:281
- This STG V2 step cannot produce the owners-relationship error this retry is meant to catch:
geneva-identities-stg.tmpl.bicepparamsetsgenevaActionApplicationManage = false, andgeneva-identities.bicepconditionally creates the app/owners module on that flag. The added retry is therefore ineffective here; if the intent is to fix the live STG rollout, apply it to the canonical pipeline, otherwise remove or clarify this dead configuration.
honorRetryBudgetWhenFailingFast: true
dev-infrastructure/global-pipeline-stg.yaml:295
- This STG V2 step cannot produce the owners-relationship error this retry is meant to catch:
mise-identity-stg.tmpl.bicepparamsetsmiseApplicationDeploy = false, andmise-identity.bicepconditionally creates the only app/owners module on that flag. The added retry is therefore ineffective here; if the intent is to fix the live STG rollout, apply it to the canonical pipeline, otherwise remove or clarify this dead configuration.
honorRetryBudgetWhenFailingFast: true
dev-infrastructure/global-pipeline-stg.yaml:281
maximumRetryCountis defined as the total number of step executions, not the number of retries (docs/pipeline-concept.md:235). Both identity blocks in this STG copy still use3, so they allow only two retries after the initial attempt, not the three one-minute retries stated in the PR description. If three retries are required, use4for both blocks (and the canonical copy), or correct the stated budget.
honorRetryBudgetWhenFailingFast: true
- Files reviewed: 33/64 changed files
- Comments generated: 1
- Review effort level: Lite
|
/retest-required |
|
Pushed 6f4cde8 to make the retry budget match the intended three retry attempts. maximumRetryCount is total executions, including the initial attempt, so both global identity steps now use maximumRetryCount: 4. That gives one initial Graph owners write plus three one-minute retries while keeping fail-fast manual mitigation disabled. Validated with make validate-config-pipelines and make verify. |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate retry-policy and STG-scope issues block approval; the dependency refresh also needs justification.
Review details
Suppressed comments (4)
dev-infrastructure/global-pipeline-stg.yaml:292
- Adding an explicit
automatedRetryhere replaces sdp-pipelines' default MSI/connection-failure policy, somise-identitywill stop retryingfailed to establish a new connection,failed to get token, andFailed to connect to MSI. The repository documents this replacement intest/e2e-pipeline.yaml:36-40; include those default phrases in this list so the Graph-owner retry does not regress other transient identity failures.
errorContainsAny:
- "Failed to update one or more relationships in 'owners'"
dev-infrastructure/global-pipeline-stg.yaml:295
- This STG V2 step passes
configurations/mise-identity-stg.tmpl.bicepparam, which hardcodesmiseApplicationDeploy = false;templates/mise-identity.biceptherefore skips the Entra app module that writes the Graphownersrelationships. The new retry condition cannot match this step's deployment, so the claimed STG mitigation is not exercised. Please either apply it to the pipeline that actually deploys the MISE app or remove this no-op policy and update the stated scope.
errorContainsAny:
- "Failed to update one or more relationships in 'owners'"
maximumRetryCount: 4
durationBetweenRetries: 1m
honorRetryBudgetWhenFailingFast: true
dev-infrastructure/global-pipeline.yaml:234
- Adding an explicit
automatedRetryhere replaces sdp-pipelines' default MSI/connection-failure policy, somise-identitywill stop retryingfailed to establish a new connection,failed to get token, andFailed to connect to MSI. The repository documents this replacement intest/e2e-pipeline.yaml:36-40; include those default phrases in this list so the Graph-owner retry does not regress other transient identity failures.
errorContainsAny:
- "Failed to update one or more relationships in 'owners'"
tooling/templatize/go.mod:19
- The PR description calls this pipeline-configuration-only, but this change also upgrades
azcore,azidentity, andtestifyacross nearly every workspace module (including this module andinternal). Those are independent build/runtime dependency changes; please either split this refresh into a separate PR or document why the new ARO-Tools pipeline dependency requires the workspace-wide upgrade and its compatibility/test impact.
- Files reviewed: 33/64 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/test image-updater-images Retesting image-updater-images: the PR changes only pipeline YAML plus the workspace dependency sync needed by make verify, and the image-updater module build/test path passes locally. I also reproduced the automation image build path from the CI variant with GOFLAGS=-mod=readonly: make -C tooling/image-updater build, make -C tooling/templatize templatize, make -C tooling/yamlwrap, make all-tidy, and go work vendor. That path succeeds on this branch without producing uncommitted changes. The Prow artifact endpoint for job 2099512988410056704 returns 500 through gcsweb and AccessDenied through direct GCS from this environment, so I cannot extract a job-local error line here. The same image-updater-images check is passing on nearby open ARO-HCP PRs, so this rerun is the discriminator for a transient CI image-build failure versus a PR-specific issue. |
|
/retest |
The previous commit accidentally bundled a repo-wide go.mod/go.sum
bump (azcore, azidentity, testify) unrelated to the ev2 retry budget
change. Reverting those files to main's pinned versions; this PR is
scoped to dev-infrastructure/global-pipeline{,-stg}.yaml only.
|
Pushed a fix, the previous head accidentally bundled a repo-wide go.mod/go.sum dependency bump (azcore 1.21.1→1.23.0, azidentity 1.13.1→1.14.0, testify 1.11.1→1.12.0 across ~60 modules) alongside the intended pipeline change. That bump is what broke ci/prow/image-updater-images (failed identically on the last 3 runs), it's unrelated to this PR's actual goal of retaining the ev2 retry budgets in global-pipeline.yaml. Reverted those go.mod/go.sum files back to main's pinned versions, this PR now only touches dev-infrastructure/global-pipeline.yaml and global-pipeline-stg.yaml. Confirmed tooling/image-updater builds clean locally after the revert. |
|
/lgtm |
|
Rael Garcia (@raelga): you cannot LGTM your own PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
🟡 Changes recommended
Both pipeline files need their retry limits corrected to provide the promised three retries.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
dev-infrastructure/global-pipeline-stg.yaml:295
maximumRetryCountincludes the initial execution (docs/pipeline-concept.md:235), so this setting leaves only two retries. The PR promises three retries after the initial run; please set this retry block tomaximumRetryCount: 4.
honorRetryBudgetWhenFailingFast: true
dev-infrastructure/global-pipeline.yaml:237
maximumRetryCountincludes the initial execution (docs/pipeline-concept.md:235), so this setting leaves only two retries. The PR promises three retries after the initial run; please set this retry block tomaximumRetryCount: 4.
honorRetryBudgetWhenFailingFast: true
dev-infrastructure/global-pipeline.yaml:223
maximumRetryCountincludes the initial execution (docs/pipeline-concept.md:235), so this setting leaves only two retries. The PR promises three retries after the initial run; please set this retry block tomaximumRetryCount: 4.
honorRetryBudgetWhenFailingFast: true
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
|
/retest |
|
global-pipeline-stg.yaml - please double check the params referenced by the mise-identity nd geneva-identities steps here. If that's right, doesn't that mean the Entra app/owners module never deploys on these two steps in this file, so the new automatedRetry blocks (and their comments about the Graph owners error) can never actually trigger here? Is this intentional (e.g. forward-looking for when V2 takes over app management at decommission), or did the canonical retry get copied over without accounting for the false flags in the STG V2 params? |
There was a problem hiding this comment.
🟡 Changes recommended
Add the retry policy and fail-fast retry-budget handling to both staging identity steps.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
|
Confirmed. The STG V2 parameter files set I removed both dead retry blocks in commit |
|
/test e2e-parallel |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raelga, roivaz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/test e2e-parallel |
|
/retest |
|
/hold Revision 21205e5 was retested 3 times: holding |
|
/unhold |
|
/test e2e-parallel |
AROSLSRE-2101
What
Add the Microsoft Graph owners relationship retry policy to
mise-identityand enablehonorRetryBudgetWhenFailingFastfor both global identity ARM steps inglobal-pipeline.yaml:geneva-identitiesmise-identityEach step allows three retries after the initial execution, with one minute between retries.
The parallel STG V2 pipeline is intentionally unchanged. Its parameter files set
genevaActionApplicationManage = falseandmiseApplicationDeploy = false, so it does not deploy either Entra app/owners module during coexistence.Why
Microsoft Graph can return the generic owners relationship error while committing application and service principal ownership:
The same error affects both global identity deployments. Fail-fast generation normally reduces the available retry budget, while
mise-identityhad no matching retry policy.The retries remain bounded. Invalid or deleted owner IDs still exhaust the retry budget and must be corrected in the service configuration.
Testing
make validate-config-pipelinesmake verifycheck-ev2-render.sh 6939No new unit tests. This is pipeline configuration only, and validation confirms the schema, pipeline metadata, and generated EV2 resolve manifests are accepted.
Special notes for your reviewer
This depends on the
sdp-pipelinesgenerator change tracked by AROSLSRE-2100 before the generated EV2 artifacts change behavior.PR Checklist