Skip to content

[CI] Rebuild the shared mqttv5 e2e ADR namespace when it disappears - #403

Merged
Ewerton Scaboro da Silva (ewertons) merged 8 commits into
mainfrom
ci/mqttv5-adr-namespace-repair
Oct 10, 2026
Merged

Ewerton Scaboro da Silva (ewertons) merged 8 commits into
mainfrom
ci/mqttv5-adr-namespace-repair

Conversation

@ewertons

Copy link
Copy Markdown
Contributor

The shared mqttv5 e2e environment's ADR namespace can disappear from ARM, removing its role assignments; DPS registrations then fail.

  • c/eng/e2e-mqttv5-adr-namespace.ps1: if the namespace is missing or not linked, re-creates it in the hub's region (or re-asserts its endpoints when ADR still holds it), re-grants the 8 role assignments, links hub + DPS (retries role-replication failures, stops on others), and pushes the namespace to DPS. Healthy namespace: one ARM read.
  • .github/workflows/ci-c-e2e-mqttv5-adr-namespace.yml: runs it hourly, on demand, and via workflow_call. Skipped while E2E_MQTTV5_SHARED_ADR_NAMESPACE is unset.
  • c/docs/eng/end-to-end-tests.md: inputs and required rights.

Setup: repository variables E2E_MQTTV5_SHARED_{SUBSCRIPTION_ID,RESOURCE_GROUP,HUB_NAME,DPS_NAME,ADR_NAMESPACE}; the OIDC identity needs Contributor and role assignment write on the resource group.

Validation: script exercised with a stubbed az (healthy, missing, still held by ADR, role-replication retry, non-retryable failure); actionlint and zizmor clean. Not yet run against Azure (shared environment not provisioned).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The repair path uses an unsupported DPS ARM API version, can misclassify stale endpoints as healthy, and may exceed its workflow timeout.

2 open findings
What changed in this PR

Adds automated recovery for the shared MQTTv5 ADR namespace.

Changes:

  • Adds namespace validation, rebuilding, RBAC restoration, and relinking.
  • Adds an hourly/reusable GitHub Actions workflow.
  • Documents configuration and permissions.
File Description
c/​eng/​e2e-mqttv5-adr-namespace.ps1 Implements ADR namespace recovery.
.github/​workflows/​ci-c-e2e-mqttv5-adr-namespace.yml Schedules and runs recovery.
c/​docs/​eng/​end-to-end-tests.md Documents setup requirements.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/ci-c-e2e-mqttv5-adr-namespace.yml Outdated
Comment thread c/eng/e2e-mqttv5-adr-namespace.ps1 Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 00:43
@ewertons

Copy link
Copy Markdown
Contributor Author

Re overview note on the DPS API version: 2026-11-02-preview is intentional. The ADR link needs it and ARM serves it to the target subscription; it is a parameter (-DpsApiVersion). No change.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Concurrent runs can race, and the stale DPS snapshot can overwrite tag updates.

1 open finding
2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Stale DPS snapshot can overwrite tags changed during recovery

c/​eng/​e2e-mqttv5-adr-namespace.ps1:190

$dps was read before namespace recovery, which can take about 81 minutes, and this PATCH replaces the complete tags map built from that stale snapshot. Any DPS tag added or changed during recovery is therefore reverted. Re-read the DPS immediately before copying its tags (and ideally use an ETag/retry if this resource has concurrent writers) before adding the trigger tag.

🧠 Review effort: Balanced

Comment thread .github/workflows/ci-c-e2e-mqttv5-adr-namespace.yml
Copilot AI balanced review requested due to automatic review settings October 10, 2026 00:51
@ewertons

Copy link
Copy Markdown
Contributor Author

Re overview note on stale DPS tags: applied in e446dd8; the DPS is re-read immediately before the tags PATCH. No ETag: ARM tag PATCH on DPS has no other automated writer here, and runs are now serialized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reusable-workflow secrets are undeclared, and DPS polling can accept stale pre-update state.

0 open findings

1 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Declare Azure login secrets in the workflow_call contract

.github/​workflows/​ci-c-e2e-mqttv5-adr-namespace.yml:27

The reusable entry point does not declare the two secrets consumed by azure/login. Callers therefore cannot pass AZURE_CLIENT_ID and AZURE_TENANT_ID explicitly—the call is rejected because those secrets are not part of the workflow_call contract—and invoking it without secrets: inherit leaves both values empty. Declare them as required secrets so the advertised workflow_call path works with normal explicit secret mapping.

Medium severity Require the updated marker before accepting DPS success

c/​eng/​e2e-mqttv5-adr-namespace.ps1:195

The DPS poll can succeed on the first stale GET because Succeeded / Active is also the state before the PATCH. That lets the workflow finish before ARM has applied the tag update that is supposed to push the namespace to DPS. Keep the marker value and require the GET to return it before accepting the terminal state.

🧠 Review effort: Balanced

@ewertons

Copy link
Copy Markdown
Contributor Author

Re latest overview notes, applied in 8b24a42: workflow_call declares required secrets AZURE_CLIENT_ID/AZURE_TENANT_ID; the DPS poll now requires the GET to return this run's adrNamespaceRestored marker before accepting Succeeded/Active.

Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The repair currently uses an invalid DPS ARM API version and misses required ADR linking and recovery behavior.

1 open finding
Previously missed (2)

In code that hasn't changed since last review

Medium severity Retry direct role-propagation failures during namespace repair

c/​eng/​e2e-mqttv5-adr-namespace.ps1:171

A role-propagation failure returned directly by this PUT aborts Invoke-Arm before the endpoint-state retry logic runs, so the advertised retry only covers failures accepted asynchronously. The pinned E2E harness documents both rejection modes and retries direct propagation errors (as well as ResourceProvisioningInProgress) around the PUT. Catch and classify retryable submission failures here so they consume the retry budget instead of ending the repair immediately.

Medium severity Reconcile namespaces failed after endpoints successfully provision

c/​eng/​e2e-mqttv5-adr-namespace.ps1:179

This treats provisioningState=Failed with both endpoints already Succeeded as a non-retryable link failure: Test-Linked is false, $done becomes true here, and $r.States is empty so line 181 throws. The pinned E2E provisioning harness explicitly handles this ADR terminal shape with a tags-only reconciliation (resubmitting endpoints is rejected as immutable); without reconciliation, registrations can fail with 403000. Return the final namespace from the probe and reconcile/poll it when its endpoints succeeded but the namespace remained Failed.

🧠 Review effort: Balanced

Comment thread c/eng/e2e-mqttv5-adr-namespace.ps1 Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:10
@ewertons

Copy link
Copy Markdown
Contributor Author

Re latest overview notes, applied in 92a3fd5:

  • Link PUTs rejected with role-propagation, transient or ResourceProvisioningInProgress errors are retried within the attempt budget; before a re-submit the namespace is given up to 5 min to settle.
  • Namespace Failed with hub-1/dps-1 Succeeded: tags-only PATCH to reconcile, then wait for Succeeded (up to 5 min).
  • Job timeout 120 min (worst case ~101 min).

DPS API version: unchanged, see earlier comment.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The script uses a nonexistent DPS ARM API version, preventing recovery from reaching the repair steps.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:17
@ewertons

Copy link
Copy Markdown
Contributor Author

Re DPS ARM api-version (latest overview): changed in 003737b. -DpsApiVersion now defaults to empty; the script reads the versions ARM serves for Microsoft.Devices/provisioningServices and uses 2026-11-02-preview if served, else 2026-03-01-preview (the e2e harness default), else the newest served. The healthy path is unchanged (one ARM read).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Namespace recovery mishandles Canceled and Deleting lifecycle states.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Medium severity Recreate namespace after Deleting state reaches absent

c/​eng/​e2e-mqttv5-adr-namespace.ps1:173

Creation is attempted only when the initial GET returns 404. If the namespace is observed in the documented Deleting state, the subsequent poll eventually sees absent but never re-enters this block, so the repair always times out instead of recreating it. Wait for deletion to finish and then run the existing create path.

Medium severity Handle Canceled namespace state during provisioning poll

c/​eng/​e2e-mqttv5-adr-namespace.ps1:177

Canceled is a terminal namespace provisioning state, but this poll only accepts Succeeded/Failed. A canceled create or recovery therefore cannot make progress and always times out instead of reaching the relink PUT; include it here as the later settle loop already does.

🧠 Review effort: Balanced

@ewertons

Copy link
Copy Markdown
Contributor Author

Re latest overview notes, applied in 32f3938:

  • Namespace seen Deleting: wait (up to 15 min) until it is absent, then the normal create path runs.
  • Namespace poll treats Canceled as terminal like Failed: with an identity it proceeds to the link PUT; without one it fails immediately instead of timing out.
  • Job timeout 135 min (worst case ~116 min).

Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The default ADR API version is subscription-gated and can make the recovery workflow consistently fail.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Use broadly enabled namespace API version or add a fallback

c/​eng/​e2e-mqttv5-adr-namespace.ps1:31

The workflow cannot reliably use this default outside subscriptions explicitly enabled for this preview. The pinned E2E provisioning harness defaults namespace linking to 2026-11-01-preview because 2026-11-02-preview repeatedly returns LinkableResourceNotReady on non-enabled subscriptions; this script classifies that as role propagation and eventually fails, and the workflow provides no -AdrApiVersion override. Use the broadly enabled namespace API version (the link body is the same), or add an explicit probe/fallback that restarts identity grants and linking on one consistent version.

🧠 Review effort: Balanced

The ADR namespace of the shared mqttv5 environment can disappear from ARM,
taking its role assignments with it; DPS registrations then fail.

c/eng/e2e-mqttv5-adr-namespace.ps1 re-creates the namespace (or re-asserts
its endpoints when ADR still holds it), re-grants the role assignments,
re-links hub and DPS, and pushes the namespace to DPS. A healthy namespace
costs one ARM read. ci-c-e2e-mqttv5-adr-namespace.yml runs it hourly, on
demand, and via workflow_call.
Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:34
@ewertons

Copy link
Copy Markdown
Contributor Author

Re ADR api-version (latest overview), 54ccb0e: kept 2026-11-02-preview as the default. The shared environment's namespace was linked with it on a subscription enabled for it, and grants and link must stay on the version the namespace was linked with (principalId differs per version), so switching the default would re-key an already working link. Added optional repository variable E2E_MQTTV5_SHARED_ADR_API_VERSION (passed as -AdrApiVersion) for environments linked with 2026-11-01-preview; documented in end-to-end-tests.md.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The workflow mutates Azure IAM and preview resources but has not yet been validated against the real shared environment.

0 open findings

🧠 Review effort: Balanced

@ewertons
Ewerton Scaboro da Silva (ewertons) merged commit 24f179c into main Oct 10, 2026
54 checks passed
@ewertons
Ewerton Scaboro da Silva (ewertons) deleted the ci/mqttv5-adr-namespace-repair branch October 10, 2026 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants