Skip to content

feat: support GA and preview runtimes - #948

Merged
Ketki Naik (ketkimnaik) merged 25 commits into
Azure:devfrom
ketkimnaik:feature/ga-preview-support
Oct 9, 2026
Merged

Ketki Naik (ketkimnaik) merged 25 commits into
Azure:devfrom
ketkimnaik:feature/ga-preview-support

Conversation

@ketkimnaik

@ketkimnaik Ketki Naik (ketkimnaik) commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Overview

  • Support GA and preview runtimes in a single CLI extension.
  • Keep GA as the default, with explicit opt-in and consent for preview deployments.
  • Preserve runtime compatibility during updates and upgrades.

Integration test run: https://github.com/Azure/azure-iot-ops-cli-extension/actions/runs/36269685104

Instance creations for preview:

image

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing full diff (head 10b405e0)

Scope note. 56 files / +6735. I read the new runtime subsystem (runtime.py, runtime_profiles.py, runtime_catalog.py, runtime_commands.py, runtime_dependencies.py, runtime_requirements.py, preview.py), the lifecycle changes (work.py, upgrade2.py, resources/instances.py, targets.py), the loader/params/help surface, and cross-checked both bundled ARM blueprints. I did not review the ~2.2k lines of new unit tests line-by-line, the generated template_preview.py payload, or the CI workflow/tox/docs changes in detail. No prior review comments exist on this PR.


Blocking

1. az iot ops update now fails when the cluster is disconnected — even for an ARM-only --tags change.
azext_edge/edge/providers/orchestration/resources/instances.py — update() now calls get_runtime_context(...) followed by runtime.require_ready() before any work. resolve_runtime appends "connected cluster is disconnected or its connectivity is unknown" to readiness_issues whenever cluster.properties.connectivityStatus != "connected", and require_ready() raises on any issue. So az iot ops update --tags env=prod, --description ..., and --spc-resource-id ... — all pure ARM writes against Microsoft.IoTOperations/instances that never touch the cluster — now hard-fail while the Arc agent is offline or the cluster is mid-reboot. The same applies to a merely Failed/Updating provisioning state on the instance, custom location, or extension.

Gating the OPC UA feature backfill on runtime readiness is clearly right. Gating a tags/description write on it is a behaviour regression. Suggest deferring require_ready() to the paths that actually need the runtime (the features/OPC UA branch), or scoping the readiness predicate so connectivity only blocks cluster-affecting selections.

2. Same call path adds new RBAC requirements to az iot ops update.
get_runtime_context issues connectedk8s.connected_cluster.get and clusterconfig.extensions.list against the cluster's subscription (instances.py, via _get_client_kwargs(subscription_id=cluster.subscription_id)). A caller who today updates an instance with only IoT Operations permissions will get an authorization failure on Microsoft.Kubernetes/connectedClusters/read and Microsoft.KubernetesConfiguration/extensions/read. This is a real least-privilege break, and it is invisible in the unit tests because they stub the clients. It is worth an explicit note in _help.py for iot ops update at minimum, and ideally the same narrowing as (1) so the reads only happen when a feature change requires them.


Suggestions

3. QUALIFICATION_IDENTITIES = () strands every previously-deployed integration/alpha build.
runtime_catalog.py registers the preview profile on train integration (template_preview.py → "TRAINS": {"iotOperations": "integration"}, version 1.6.0-preview.9), and RuntimeProfileCatalog.__init__ folds exactly that one identity into qualification_identities. resolve_runtime_identity then raises "AIO runtime ... on train 'integration' has no supported runtime profile mapping" for anything else. Concretely: a cluster already running 1.6.0-preview.8/integration (or any earlier alpha) can no longer be updated or upgraded by this CLI at all — including the upgrade that would move it onto preview.9. Since the whole point of pinning a preview profile is to let those clusters roll forward, QUALIFICATION_IDENTITIES probably needs the historical baselines populated, or the upgrade path needs to accept an installed identity that is one step behind the bundled target. The docstring says "Historical integration baselines can be supplied independently of the target catalog" — the tuple is just empty.

4. Upgrade preflight list failures changed from tolerant to fatal, reversing a previously-documented decision.
upgrade2.py — _check_default_registry_needed and _check_opcua_connector_template_needed both swapped logger.debug(...); return False for raise ValidationError(...) from e. The deleted comment stated the rationale explicitly: "Upgrade re-evaluates every run and has not mutated the instance, so a transient list failure is non-fatal here." That reasoning still holds — these run inside ClusterUpgradeState.__init__ during analyze_cluster, before any write. The new behaviour turns a throttled or transiently-503 GET .../connectorTemplates into a full az iot ops upgrade abort. The update side genuinely needed this change (it used to mutate first — see the reordering in instances._update, which is a good fix); the upgrade side did not obviously need it. If this is deliberate, worth saying so in the PR body, because it undoes a decision the code was carrying a comment about.

5. ExtensionUpgradeState.current_version is now inconsistent across extension types.
upgrade2.py — version_key = "currentVersion" if self.moniker == EXTENSION_MONIKER_OPS else "version". For the AIO extension this correctly reads the installed version; for cert-manager / secret-store it still reads properties.version, which is the requested version and is frequently None or a range. _has_delta_in_version, is_cli_behind_cluster and the rendered upgrade table therefore compute deltas against different semantics depending on the extension. Note DependencyRequirement.validate_installed (runtime_dependencies.py) already standardises on currentVersion for those same extensions, so the two subsystems disagree about what "current" means for a dependency.

6. The preview consent prompt defaults to accept.
preview.py — Confirm.ask("Accept the preview terms and create a preview instance?", default=True, ...). For a prompt whose purpose is recording acceptance of the Azure Previews Supplemental Terms, a bare Enter should not be an acceptance; default=False is the safer shape. Related, smaller: when the user declines, execute_ops_init does a bare return, so az iot ops create --use-preview exits 0 printing nothing at all — a short "Preview creation cancelled." would make the outcome legible.

7. runtime_validation_handler is registered without the idempotency guard applied to its neighbour.
azext_edge/__init__.py — runtime_notice_handler is guarded by cli_ctx.data["runtime_notice_registered"], but runtime_validation_handler (and the pre-existing version_check_handler) are registered unconditionally on every loader construction. If the loader is instantiated more than once against the same cli_ctx, validate wraps _command_validator twice and the original validator runs twice. The guard sitting immediately above makes the omission look unintentional rather than considered.


Nits

8. The ARM-expression branch of merge_template_object is unreachable with the bundled templates.
targets.py — the isinstance(defaults, str) and defaults.startswith("[") branch (and the feature_parameter["defaultValue"] = merged fallback below it) only fires when the features parameter carries an ARM-expression defaultValue. Neither blueprint declares a defaultValue for features at all (template.py and template_preview.py both have "features": {"$ref": "#/definitions/_1.Features", "nullable": True}), so defaults is always None and the function always takes the deepcopy(overrides) path. Worth either dropping the branch or noting why it is kept ahead of a template change — the apostrophe-doubling in the generated json('...') literal is the kind of thing that only gets exercised once it is actually reachable.


Checked and clean (recording the negatives so they are not re-derived)

  • Removing get_default_instance_config does not drop schemaRegistryRef / adrNamespaceRef. Both blueprints already declare them on the aioInstance resource, so the CLI no longer needs to inject them.
  • --feature still reaches the instance on the GA path. _apply_instance_features writes parameters["features"] and returns early because the stable blueprint's properties.features is the string "[variables('effectiveFeatures')]"; that variable is defined as if(parameters('disableOpcUaFeature'), union(coalesce(parameters('features'), ...), ...), parameters('features')), so the parameter is consumed. The preview blueprint binds parameters('features') directly.
  • --yes is available on iot ops create. confirm_yes is registered at the iot ops group level in params.py, so the new --use-preview help text's reference to --yes is accurate.
  • command_string is populated when runtime_notice_handler reads it. knack sets invocation.data['command_string'] before raising EVENT_INVOKER_PRE_PARSE_ARGS (knack/invocation.py, execute()), so the notice fires for both help and execution as intended.
  • instances._update(...) from upgrade2 is not a lint violation. protected-access is disabled in .pylintrc, and clone.py already calls self.instances._ensure_oidc_issuer(...).
  • The _apply_single_operation resource-group change is a genuine fix, not a regression: cluster extensions live in the cluster's resource group, and connected_cluster.resource_group_name is a real attribute.
  • Moving _process_extension_dependencies() / _raise_if_ops_deployed() earlier in _do_work is safe. _resource_map is assigned in execute_ops_init and _process_connected_cluster() runs first, so both have what they need.
  • Reordering the OPC UA backfill discovery ahead of the instance PUT in instances._update closes the failure mode discussed on fix: skip OPC UA connector template when opcua feature is disabled #929 (instance mutated, then a failing template list leaving OPC UA enabled without its template).
  • RuntimeIdentity accepts both bundled profiles. stable 1.4.112/stable (no prerelease) and preview 1.6.0-preview.9/integration (patch 0, prerelease matching preview\.[1-9][0-9]*) both validate.
  • No HISTORY.rst finding. CONTRIBUTING.md does not require an entry and HISTORY.rst on dev is a fresh "Initial baseline" stub, so the 2.10.0 → 2.10.0a1 bump without a changelog section is consistent with the branch's current state.

This is an automated review and may be incomplete — in particular it does not cover the new test modules, the generated preview template payload, or the CI workflow changes in depth.

Comment thread azext_edge/edge/providers/orchestration/runtime_catalog.py
Comment thread azext_edge/edge/providers/orchestration/runtime_profiles.py
Comment thread azext_edge/edge/providers/orchestration/runtime.py
Comment thread azext_edge/edge/providers/orchestration/preview.py Outdated
Comment thread azext_edge/edge/providers/orchestration/runtime_requirements.py
Comment thread azext_edge/edge/providers/orchestration/runtime.py Outdated
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 10b405e0 (head 6219c5b6) — one commit, Fix preview runtime compatibility handling (19 files, +327/-40).

This increment answers two of Yuelin Zhao (@cheatsheet1999)'s inline asks directly (profile-carried management API, autoUpgradeMinorVersion ownership) — I'm not restating those threads, only reviewing whether the implementations are complete. I've also left the SHARED_DEPENDENCY_REQUIREMENTS thread alone; the new comment block answers it in-code.


Blocking

  1. az iot ops show now hard-fails when runtime discovery fails — azext_edge/edge/commands_edge.py (~line 333) passes resolve_api=True, so Instances.show (resources/instances.py ~line 224) runs get_runtime_context(...) → resolve_runtime(...) before returning. That function raises ValidationError on conditions that have nothing to do with the read: "Expected exactly one AIO extension on the associated connected cluster", "The custom location is not associated with the AIO extension", "Unable to determine installed AIO version: extension currentVersion is missing", and resolve_runtime_identity's "...has no supported runtime profile mapping".

    That last one is load-bearing here. QUALIFICATION_IDENTITIES is still () (runtime_catalog.py line 44), and RuntimeProfileCatalog.__init__ folds in only integration-train profile identities — i.e. exactly one entry, 1.6.0-preview.9. So az iot ops show against a cluster on 1.6.0-preview.8 (or any earlier alpha on the integration train) now errors out where it previously returned the instance record. show is the command people reach for when an install is broken; making it depend on healthy discovery inverts that. I raised the empty-tuple issue on the previous iteration for update/upgrade — the point here is that this commit widened its blast radius to a read command.

    Two secondary costs of the same line: show goes from 1 ARM call to 4 (your own test_instance_show bumps len(mocked_responses.calls) 1 → 4), and it now requires Microsoft.Kubernetes/connectedClusters/read + Microsoft.KubernetesConfiguration/extensions/read in the cluster's subscription, which show never needed before.

    Suggested shape: wrap the resolve_api block so a discovery/identity failure degrades to the GA record already in hand rather than failing the command.

  2. _require_manual_upgrade blocks repair as well as upgrade, with no CLI-provided remedy — upgrade2.py line 1314. autoUpgradeMinorVersion is optional on the Arc extension contract and defaults to True (see the vendored clusterconfigmgmt/operations/_operations.py docstrings: "autoUpgradeMinorVersion": True, # Optional. Default value is True.). Your bundled blueprints all pin it False (template.py lines 702/722/1530, template_preview.py line 813), so CLI-created instances are fine — but an instance created via the portal, az k8s-extension create, or third-party IaC that doesn't set the field will have it true/absent and is now permanently rejected.

    _validate_and_resolve_version calls this on both branches (line 1294 version-delta, line 1309 failed-state reconcile), so it also blocks the repair path for a Failed extension — that's the recovery flow, and it's the sharper half of the regression. --force explicitly can't override, and the CLI never sends the field itself (your test_ops_version_pinning_requires_manual_ownership asserts "autoUpgradeMinorVersion" not in patch_properties), so nothing in this extension can transition a cluster into the required state.

    The message says ownership "must be disabled through an explicit ownership decision" without naming the action. Please put the concrete remedy in the error text (az k8s-extension update --cluster-name ... --name ... --auto-upgrade false) and mention the prerequisite in az iot ops upgrade help — _help.py currently says nothing about auto-upgrade. Even better for the repair branch: allow reconcile-in-place when no version pin is actually being sent.

Suggestions

  1. Profile API routing covers three entry points; every other provider still pins GA. resolve_api / use_runtime_profile are reachable only from show_instance, Instances.update (line 333) and UpgradeManager.__init__ (line 149). brokers.py, dataflows.py (two sites), dataflow_graphs.py, sync_rules.py, connector/opcua/certs.py, commands_secretsync.py, adr/helpers.py, clone.py, deletion2.py, migration.py, and the standalone az iot ops connector template / registry-endpoint command paths all build Instances(cmd) and stay on DEFAULT_IOTOPS_MGMT_API_VERSION (2026-07-01). That leaves the original concern — preview-only child resources such as mcpAuthorizationPolicies — unresolved for everything outside upgrade/update. Worth either scoping that explicitly in the PR description or resolving the API inside Instances itself.

  2. The instances= injection is order-dependent and nothing guards the order. ConnectorTemplates.__init__ / RegistryEndpoints.__init__ snapshot self.instances.iotops_mgmt_client into self.ops / self.registry_endpoints at construction, while use_runtime_profile replaces instances.iotops_mgmt_client. It works today only because the swap happens first in both callers (upgrade2.py 149 before 150–151; Instances._update after update() already swapped). A future caller that constructs either provider before the swap silently reverts to the GA API with no test failing. A lazy property on the child providers (or having them read self.instances.iotops_mgmt_client at call time) would make this structural rather than incidental.

  3. Record why the management API and the blueprint apiVersion differ. test_bundled_profiles_select_management_api_without_changing_preview_blueprint locks mgmt 2026-11-01-preview against aioInstance apiVersion 2026-09-01-preview. The skill doc now says to report the approved difference in the handoff, but there's no in-repo marker next to iotops_api_version=IoTOpsMgmtApiVersion.V20261101_preview.value (runtime_catalog.py line 42). One comment there would stop the next template sync from "correcting" it.

Nits

  1. _validate_version_upgrade (upgrade2.py ~1540): the minor-jump message ends with "including with --force" but the sibling major-version message doesn't, even though --force can't override that one either. Worth making the two consistent.

Checked and clean (recording the negatives so they don't get re-derived)

  • All four blueprint sites already set autoUpgradeMinorVersion: False, so the new gate does not break instances this extension created.
  • use_runtime_profile short-circuits on an API match, so GA paths add no extra GET; the re-GET only fires for preview.
  • The drift check properties.get("autoUpgradeMinorVersion") is not observed.auto_upgrade_minor_version is sound for JSON true/false/null (all singletons).
  • The relaxed preview policy is safe on ordering: downgrades are still caught earlier by validate_upgrade_boundary, and the minor arithmetic only runs once majors are proven equal.
  • RuntimeProfile.iotops_api_version defaults to DEFAULT_IOTOPS_MGMT_API_VERSION, so the stable profile and any future profile are unchanged without an explicit opt-in.
  • Test-mock plumbing (set_instance_mock(iotops_api_version=...), the regex tightening on the connector-template create URL) correctly asserts the API version on the write URLs, not just the reads.

This is an automated review and may be incomplete; I did not exercise the preview API against a live service, and the field-preservation tests are mocked — they don't establish server-side preservation of preview-only child resources.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 6219c5b6 (head 5b732820) — one commit, Use structured runtime readiness issues, 3 files (+125/-23): runtime.py, test_runtime_unit.py, test_runtime_registration_unit.py. The recorded SHA is still in /pulls/948/commits, so the compare is a true increment.

The refactor itself is correct. I checked the new code filter against the string-prefix filter it replaces, issue by issue, and the recovery set is preserved exactly:

issue old startswith(...) verdict new (resource, code) verdict
instance provisioning state is X blocker blocker (resource="instance")
custom location provisioning state is X blocker blocker
connected cluster is disconnected … blocker blocker (CLUSTER_CONNECTIVITY)
extension status information is invalid blocker blocker (INVALID_EXTENSION_STATUS not in the exempt set)
extension provisioning state is X exempt exempt (PROVISIONING_STATE + resource="extension")
extension reports an error exempt exempt (EXTENSION_ERROR)
requested version … differs … exempt exempt (VERSION_MISMATCH)

INVALID_EXTENSION_STATUS is the one that would have been easy to get wrong — it is an extension-scoped issue, so a filter written as "exempt everything on the extension" would have silently widened recovery. Giving it its own code and leaving it out of the exempt set is right. pyflakes is clean on all three changed files.

Blocking

None.

Suggestions

  1. The commit answers only half of the review comment it closes, and the reply does not say so. runtime.py#L86-L104 — HangyiWang's inline comment (4127714867) asked for two things:

    we should treat Failed and Canceled states for: instances, custom locations, and extensions as recoverable during upgrade/update operations, and block only while the resource is in an in-progress state (Creating, Updating, Deleting, or Accepted). We should also mark issues as recoverable using structured error codes rather than relying on message-prefix matching.

    The commit delivers the second ask. The first is declined — the reply on that thread says "preserving existing recovery rules", which is accurate but reads like the whole comment was addressed. Concretely, require_upgradeable still gates on the extension's provisioning_state alone (_lower(self.provisioning_state) not in {"failed", "canceled"}), so an instance or custom location sitting in Failed/Canceled with a healthy extension never reaches the blocker filter at all — require_ready() raises first, and az iot ops upgrade cannot repair it. That is exactly the case the comment named.

    Worth noting that the new tests now lock in the narrower behaviour rather than leaving it open: test_unready_runtime_rejected asserts require_upgradeable() raises for instance/custom_location in Failed and Canceled, and test_repair_does_not_ignore_other_readiness_failures parametrizes those same four cases as expected blockers. So if the wider semantics are still wanted, this increment makes them a larger change, not a smaller one. Please either implement the per-resource gate or state the decline explicitly on that thread so it does not get resolved as done.

  2. resource is a free-form str matched by a literal, which is the same coupling the commit set out to remove — one layer down. runtime.py#L95 compares issue.resource == "extension", and the only producer of that value is the display-name literal in the loop at runtime.py#L148 (("instance", instance), ("custom location", custom_location), ("extension", extension)) — the same strings that are interpolated into the user-facing message. Rewording "extension" to, say, "AIO extension" for readability would silently turn every extension issue into an upgrade blocker, with no test failure signal beyond the equality assertions in test_unready_runtime_rejected. A RuntimeResource enum (or module constants) used for both the comparison and the message interpolation would finish the job the issue codes started.

Nits

  1. RuntimeIssue.state is written but never read. All four call sites in resolve_runtime populate it, and nothing in azext_edge/edge/** consumes it — grep -rn "issue\.state" over the tree at head returns nothing; the only readers are the equality assertions in test_runtime_unit.py. Either drop it or wire it into the message/diagnostics. It is also overloaded: for CLUSTER_CONNECTIVITY it carries connectivityStatus, for every other code it carries a provisioningState, so a future consumer reading issue.state uniformly would be wrong.

  2. The wording-independence assertions rewrite every message to the same string (test_runtime_unit.py#L123-L128 and L152-L156), so a match= on that string cannot tell which issue produced the error. Rewording only the exempt issues (or only the blocking ones) would make the assertion prove that the classification, not the text, drives the outcome.

Checked and clean

  • Classification parity with the old prefix filter — the table above; no issue changes side.
  • test_unready_runtime_rejected's new require_upgradeable branch is consistent with the gate: for non-Failed/Canceled extension states the method short-circuits into require_ready(), which raises, so the else arm is correct for Creating/Updating/Deleting/Accepted/Unknown/None. The added "Unknown"/"Accepted" cases are genuinely new coverage.
  • No production consumer reads readiness_issues structurally — only require_ready() / require_upgradeable() are called (instances.py:326, runtime_commands.py:169, upgrade2.py:147, plus azext_edge/tests/runtime_checks.py:49), so changing the tuple's element type from str to RuntimeIssue breaks nothing outside this file and its tests.
  • RuntimeIssueCode(str, Enum) keeps the values JSON-serialisable if these are ever surfaced.
  • from enum import Enum added; no unused imports introduced (pyflakes clean on all three files).
  • Per the earlier review on this PR: HISTORY.rst needs no entry in this repo, and CONTRIBUTING.md does not require one.

Carried forward (unchanged by this increment, not re-derived)

QUALIFICATION_IDENTITIES is still () in runtime_catalog.py:44; show_instance still passes resolve_api=True (commands_edge.py:334), so az iot ops show still runs the full resolve_runtime validation path; and _require_manual_upgrade still names no remedy for autoUpgradeMinorVersion=True. Those were raised on the 10b405e0 and 6219c5b6 reviews and are out of scope for this commit.


This is an automated review and may be incomplete; scope was the three files in the 6219c5b6...5b732820 increment, not the full PR.

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.

Copilot review overview

🟡 Changes recommended

Preview identity/secret-sync writes can use the GA API, and container cleanup repeats the API-version failure seen in the referenced integration run.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Adds bundled GA and preview runtime profiles with explicit preview consent, channel-aware lifecycle handling, and dual-channel qualification.

Changes:

  • Introduces runtime catalogs, discovery, compatibility validation, preview consent, and profile-specific templates/APIs.
  • Expands unit and integration coverage across stable and preview channels.
  • Reworks CI to test one hash-verified candidate wheel across isolated channel jobs.
File Description
tox.ini Uses the isolated wheel-based integration runner.
tools/​integration_runner.py Verifies, installs, and tests candidate wheels.
setup.cfg Exempts the generated preview template from line limits.
docs/​tox-testing.md Documents channel-aware tox execution.
docs/​integration-tests.md Documents dual-channel qualification and upgrades.
azext_edge/​tests/​settings.py Adds runtime test environment variables.
azext_edge/​tests/​runtime_checks.py Adds live runtime identity assertions.
azext_edge/​tests/​edge/​support/​test_support_unit.py Tests both schema registry layouts.
azext_edge/​tests/​edge/​support/​create_bundle_int/​test_schemaregistry_int.py Supports GA and preview schema workloads.
azext_edge/​tests/​edge/​orchestration/​test_work_unit.py Updates deployment expectations and request mocks.
azext_edge/​tests/​edge/​orchestration/​test_upgrade_int.py Verifies channel-preserving upgrades.
azext_edge/​tests/​edge/​orchestration/​test_template_preview_unit.py Validates the preview blueprint and consent flow.
azext_edge/​tests/​edge/​orchestration/​test_targets_unit.py Updates template parameter expectations.
azext_edge/​tests/​edge/​orchestration/​test_runtime_unit.py Tests runtime discovery and eligibility.
azext_edge/​tests/​edge/​orchestration/​test_runtime_registration_unit.py Tests command registration and validation hooks.
azext_edge/​tests/​edge/​orchestration/​test_runtime_profiles_unit.py Tests profiles, catalog selection, and boundaries.
azext_edge/​tests/​edge/​orchestration/​test_runtime_dependencies_unit.py Tests foundation compatibility policies.
azext_edge/​tests/​edge/​orchestration/​test_runtime_commands_unit.py Tests consent, routing, and upgrade guards.
azext_edge/​tests/​edge/​orchestration/​test_runtime_channels_int.py Adds live cross-channel checks.
azext_edge/​tests/​edge/​orchestration/​test_runtime_activation_unit.py Exercises bundled preview lifecycle behavior.
azext_edge/​tests/​edge/​orchestration/​test_get_versions_unit.py Tests runtime profile reporting.
azext_edge/​tests/​edge/​orchestration/​resources/​test_instances_unit.py Tests API routing and field preservation.
azext_edge/​tests/​edge/​orchestration/​resources/​registry_endpoint/​test_registry_endpoints_unit.py Supports profile-specific test endpoints.
azext_edge/​tests/​edge/​orchestration/​resources/​connector/​akri/​test_connector_templates_unit.py Supports profile-specific connector endpoints.
azext_edge/​tests/​edge/​init/​int/​test_init_int.py Provisions and verifies channel-specific runtimes.
azext_edge/​tests/​conftest.py Registers markers and runtime qualification checks.
azext_edge/​edge/​util/​az_client.py Adds the preview management API version.
azext_edge/​edge/​providers/​support/​schemaregistry.py Collects both schema registry layouts.
azext_edge/​edge/​providers/​orchestration/​work.py Selects profiles and validates dependencies before creation.
azext_edge/​edge/​providers/​orchestration/​targets.py Builds deployments from profile-specific blueprints.
azext_edge/​edge/​providers/​orchestration/​runtime.py Implements runtime discovery and capability validation.
azext_edge/​edge/​providers/​orchestration/​runtime_requirements.py Defines command and parameter restrictions.
azext_edge/​edge/​providers/​orchestration/​runtime_profiles.py Defines runtime identities, profiles, and boundaries.
azext_edge/​edge/​providers/​orchestration/​runtime_dependencies.py Defines foundation compatibility checks.
azext_edge/​edge/​providers/​orchestration/​runtime_commands.py Integrates runtime validation with command parsing.
azext_edge/​edge/​providers/​orchestration/​runtime_catalog.py Registers bundled stable and preview targets.
azext_edge/​edge/​providers/​orchestration/​template_preview.py Supplies the generated preview deployment blueprint.
azext_edge/​edge/​providers/​orchestration/​resources/​registryendpoints.py Allows reuse of a profile-selected client.
azext_edge/​edge/​providers/​orchestration/​resources/​instances.py Adds runtime-aware instance reads and updates.
azext_edge/​edge/​providers/​orchestration/​resources/​connector_templates.py Reuses profile-selected clients for backfills.
azext_edge/​edge/​providers/​orchestration/​resources/​clusters.py Preserves cluster subscription routing.
azext_edge/​edge/​providers/​orchestration/​preview.py Implements preview terms and consent.
azext_edge/​edge/​params.py Adds preview selection and updated help text.
azext_edge/​edge/​commands_edge.py Exposes preview creation and profile reporting.
azext_edge/​edge/​_help.py Documents inline runtime profile output.
azext_edge/​constants.py Advances the package to 2.10.0a1.
azext_edge/​__init__.py Registers runtime command hooks.
.github/​workflows/​int_test.yml Runs stable/preview jobs using one candidate wheel.
.github/​workflows/​container_int_test.yml Adds dual-channel container qualification.
.github/​test-scenarios.yml Adds runtime and explicit upgrade-path scenarios.
.github/​test-container-scenarios.yml Defines the container test scenario.
.github/​skills/​sync-aio-bicep-templates/​SKILL.md Extends template synchronization guidance.
.github/​skills/​sync-aio-bicep-templates/​references/​runtime-profiles.md Documents profile-aware synchronization.
.github/​actions/​build-int-test-matrix/​build_matrix.py Expands scenarios by channel and validates baselines.
.github/​actions/​build-int-test-matrix/​action.yml Exposes channel and baseline inputs.
.dockerignore Includes the runner while excluding generated results.
.coveragerc Normalizes installed-wheel coverage paths.

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

Comment thread azext_edge/edge/providers/orchestration/resources/instances.py Outdated
Comment thread .github/workflows/container_int_test.yml
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 5b732820 (head 4dacedf8) — one commit, Use supported preview management API. Scope is the increment only (6 files, +8/-8): the preview runtime profile's IoT Operations control-plane API version moves 2026-11-01-preview → 2026-09-01-preview, with the enum member in azext_edge/edge/util/az_client.py renamed accordingly and four unit suites updated to the new literal.

No blocking findings. The change is coherent and, as far as I can verify in-repo, correct:

  • It aligns the profile with the bundled preview blueprint. All 8 "apiVersion" declarations in azext_edge/edge/providers/orchestration/template_preview.py are 2026-09-01-preview, so PREVIEW_PROFILE.iotops_api_version now matches what the deployment actually creates. That is exactly what cheatsheet1999 asked for in the inline thread on runtime_profiles.py ("The Preview template uses 2026-09-01-preview …"), so this closes the real gap rather than papering over it.
  • The rename is fully propagated. grep -rn "2026-11-01-preview\|V20261101" over the tree at 4dacedf8 returns zero hits across *.py, *.json, *.md, *.yml. IoTOpsMgmtApiVersion.V20260901_preview has exactly one producer (runtime_catalog.py line 42); the clone.py mapping only references GA members, so nothing there needed an edit.
  • The swap is mechanically safe at the client layer: azext_edge/edge/vendor/clients/iotopsmgmt/v20260701/_configuration.py takes api_version as an unvalidated kwargs.pop("api_version", "2026-07-01"), so get_iotops_mgmt_client(api_version=...) just changes the query string. Instances.use_runtime_profile re-GETs through the new client, which is what the updated mocked endpoints in test_instances_unit.py / test_runtime_activation_unit.py / test_upgrade2_unit.py exercise.

Suggestions

  1. azext_edge/tests/edge/orchestration/test_runtime_profiles_unit.py (lines ~56-58) — the drift this commit fixes is still not locked. test_bundled_profiles_select_management_api_without_changing_preview_blueprint asserts the profile version and the blueprint version against two independent string literals, which is precisely the shape that allowed 2026-11-01-preview vs 2026-09-01-preview to diverge in the first place. An equality assertion would catch the next one:

    assert preview.iotops_api_version == preview.copy_instance_blueprint().content["resources"]["aioInstance"]["apiVersion"]

    (keep the literal too, so an intentional bump is still an explicit edit).

  2. Evidence for "supported". The commit message asserts the new version is the supported one, but neither 2026-09-01-preview nor 2026-11-01-preview is published in Azure/azure-rest-api-specs (latest public Microsoft.IoTOperations preview there is 2025-07-01-preview), so this is not externally verifiable from the diff. Since az iot ops show --resolve-api, update, and preview upgrade now all route through this version, it would be worth stating in the PR whether 2026-09-01-preview was confirmed against the live RP in the clouds/regions the preview targets — a wrong value here fails those commands at GET time, not at deploy time.

Nits

  1. Stale review thread — your reply on the runtime_profiles.py thread (#discussion_r4129974447) still says "Added release-profile API selection using 2026-11-01-preview". Worth a one-line correction so the next reader of that thread doesn't re-derive the mismatch.

  2. Unchanged by this commit but adjacent to the thread it answers: the vendored client ships only v20260701 (azext_edge/edge/vendor/clients/iotopsmgmt/__init__.py), so preview API coverage is an api-version swap over GA-shaped operations. Preview-only fields survive because the client is dict-based (your previewOnlyProperty test proves that), but preview-only resources do not — mcpAuthorizationPolicies, named in cheatsheet1999's original comment, has zero references in the repo. If that is intentional for this PR, saying so would close the second half of that comment.

I have not restated the two inline comments Copilot posted earlier today on resources/instances.py and .github/workflows/container_int_test.yml; both are outside this increment.

This is an automated review and may be incomplete.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 4dacedf8 (head 7b039cef) — 2 commits (60cd8c2b "Preserve diagnostic instance show access", 7b039cef "Fix runtime writes and help"), 5 files.

Net product change is three things: Instances.show(resolve_api=True) now tolerates runtime-discovery failure and falls back to the GA API with a warning; a new _get_instance_for_write routes the four identity/secret-sync workflows through strict runtime resolution before any write; and _require_manual_upgrade's error now names the remediation command.

Two things worth recording up front, because they close prior threads:

  • The _get_instance_for_write change is a correct fix for Copilot's inline comment 4136554578 (_update running on the GA 2026-07-01 client for a preview instance). I verified all four call sites now route through it — remove_mi_user_assigned (~line 450), add_mi_user_assigned (~503), enable_secretsync (~591), disable_secretsync (~708) — and that in add_mi_user_assigned and enable_secretsync the read was moved ahead of the federation / keyvault role-assignment work, so a resolution failure can no longer leave a half-applied side effect. I am not restating that comment.
  • The upgrade2.py message now spells out az k8s-extension update --subscription … --cluster-type connectedClusters --name <aio-extension-name> --auto-upgrade false, which answers the "the error names no remedy" point from my review at 6219c5b6. Considered closed.

Blocking

None.

Suggestions

  1. azext_edge/edge/providers/orchestration/resources/instances.py, show() (~lines 224-235) — the fallback does not cover catalog.get(...). The try wraps only get_runtime_context; catalog.get(runtime.identity.channel) is evaluated in the else: clause, outside it. RuntimeProfileCatalog.get (runtime_profiles.py ~line 130) raises ValidationError(f"No reviewed {channel.value} runtime profile is bundled in this CLI."), and runtime_catalog.py declares PREVIEW_PROFILE: Optional[RuntimeProfile] guarded by if PREVIEW_PROFILE is not None, so a GA-only build is an explicitly supported state. In such a build, az iot ops show against a preview-train instance raises — and that failure is in neither category the new tests define: test_instance_discovery_failure_only_falls_back_for_show parametrizes only errors raised by get_runtime_context, and test_instance_show_does_not_hide_instance_read_errors covers only the two instance GETs. It is also not an instance read error, so the "do not hide read errors" rationale does not justify surfacing it; the user is told about CLI bundle contents while trying to read their instance. Moving catalog.get(runtime.identity.channel) inside the try (leaving use_runtime_profile's GET outside, as the read-error test requires) closes it in one line. Latent today because PREVIEW_PROFILE is non-None on this branch.

  2. Same file — the four workflows resolve the runtime but never call runtime.require_ready(), while Instances.update (~line 339) does. So after this change az iot ops update --tags still fails when the Arc agent is disconnected or the extension is mid-reconcile, but az iot ops identity assign/remove and secretsync enable/disable — which perform the same instance PUT — proceed. I think the new behaviour is the right one and update's require_ready() is the outlier (that is the readiness concern I raised at 6219c5b6), but the two paths should agree deliberately rather than by omission.

  3. runtime_catalog.py line 44 — QUALIFICATION_IDENTITIES is still (), so catalog.qualification_identities contains exactly one integration identity (PREVIEW_PROFILE.identity, 1.6.0-preview.9), and resolve_runtime_identity raises AIO runtime '<v>' on train 'integration' has no supported runtime profile mapping. for every other integration build. This increment extends that hard failure from show/update/upgrade to four more commands: on a 1.6.0-preview.8 cluster, az iot ops identity assign, identity remove, secretsync enable and secretsync disable now fail where they previously worked. test_instance_workflow_api_failure_prevents_writes[unmapped_runtime] locks that in, so it reads as intended — but the raised message names no remedy and there is no escape hatch. I raised the empty tuple on the first review of this PR; restating only because the affected command surface just grew.

  4. remove_mi_user_assigned gains a cluster-subscription dependency it did not have. Its get_resource_map(...).connected_cluster.resource call is commented out (the # TODO - @digimaun block), so today the command touches only the instance; _get_instance_for_write → get_runtime_context now issues connectedClusters.get plus extensions.list, requiring Microsoft.Kubernetes/connectedClusters/read and Microsoft.KubernetesConfiguration/extensions/read in the cluster's subscription. The other three already read the cluster, but the extensions.list call is new for them too. Worth a release note, since a caller scoped only to the instance's resource group now gets AuthorizationFailed on an operation that previously succeeded.

Nits

  1. _get_instance_for_write (~line 256) re-implements the three lines inside show()'s resolve_api branch rather than sharing them, so the strict and tolerant paths can drift — a future change to how the profile is selected has to be made twice. A strict flag on show(resolve_api=...), or having _get_instance_for_write call the same private helper, would keep them in lockstep.

  2. _help.py net is +3/-0: 60cd8c2b added a detailed paragraph plus an az k8s-extension update … --auto-upgrade false example, and 7b039cef removed the example and condensed the paragraph to three lines. The end state keeps the full command only in the runtime error string. That is defensible, but the help example was the discoverable half — a user planning an upgrade reads -h before they hit the error. Flagging in case the removal was collateral from the condensation rather than intentional.

Summary — the increment does what it says: show is no longer a casualty of runtime-discovery failure, the four write workflows can no longer PUT through the GA client on a preview instance, and the upgrade-ownership error is now actionable. The new tests are well-targeted (the show-vs-update parametrization proving the fallback is read-only is the right shape). The one concrete gap is the unguarded catalog.get(...) in show; the rest are scoping and release-note calls.

Automated review — may be incomplete. I did not re-review the runtime subsystem outside this increment, and I have not restated the existing inline comments on this PR.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 7b039cef (head dfa98ca5) — one commit, Refresh preview runtime and tests, 15 files (+405/-55).

Scope: the six product files in the increment (template_preview.py, runtime_catalog.py, connector_templates.py, resources/instances.py, targets.py, upgrade2.py) plus the matching unit tests. I did not re-review the runtime subsystem outside this increment, and I am not restating the existing inline threads on this PR.

Blocking

1. The new Microsoft.OpcUa.WoT endpoint type cannot reach an existing instance, which defeats the runtime_profile plumbing this commit adds.

create_default_opcua_template now sources deviceInboundEndpointTypes from the selected blueprint instead of the hardcoded OPCUA_CONNECTOR_ENDPOINT_TYPE, and the preview blueprint grows a second entry:

"deviceInboundEndpointTypes": [
    {"endpointType": "Microsoft.OpcUa"},
    {"endpointType": "Microsoft.OpcUa.WoT"},
],

But both production callers — Instances._update (resources/instances.py ~line 435) and Upgrade._create_default_opcua_connector_template (upgrade2.py line 303) — only run after check_default_opcua_template_needed, and that helper returns (False, None) the moment a prefix-matching template exists in any non-Failed state (connector_templates.py lines 320-329):

if (template.get("provisioningState") or "").lower() == PROVISIONING_STATE_FAILED.lower():
    return True, template.get("name")
logger.debug("Default OPC UA connector template already exists.")
return False, None

So the only paths that can ever observe the new endpoint type are (a) a brand-new instance, where ARM deploys opcUaConnectorTemplate from the blueprint directly, and (b) in-place repair of a template that is already Failed. Every preview instance created by this branch before today has a Succeeded template and will keep a single Microsoft.OpcUa entry through az iot ops upgrade and through az iot ops update --feature opcua.mode=....

That is the same create-or-confirm shape as the LIVE_DATA_TOPIC_TEMPLATE rename on #931: the helper confirms existence, it does not reconcile content. If WoT is only meant for new instances this is fine and worth a comment saying so; if it is meant to roll forward, the upgrade path needs a drift check on deviceInboundEndpointTypes rather than a presence check.

Suggestions

2. The preview runtime version moved 1.6.0-preview.9 → 1.6.0-preview.19, which re-points the single qualification mapping. QUALIFICATION_IDENTITIES in runtime_catalog.py is still (), so resolve_runtime_identity recognises exactly one integration identity — previously preview.9, now preview.19. Every cluster this branch has already provisioned sits on preview.9 and, after this commit, will fail with "has no supported runtime profile mapping" on show, update, upgrade, identity assign/remove and secretsync enable/disable. I raised the empty tuple on earlier iterations and am not re-litigating it; the new fact is that this bump invalidates the previously-qualified build, so the window is no longer theoretical. Carrying both identities, or documenting the re-create requirement, would close it.

Relatedly, the docstring change drops the release-policy override note ("pin upstream 1.6.0-preview.11 to the release owner's recommended 1.6.0-preview.9") in favour of "The pinned source deploys 1.6.0-preview.19 without a runtime-version override." Worth confirming the override was actually retired by the release owner rather than just removed from the comment.

3. sku on the preview aioInstance is inert on this branch, and a second branch implements it differently. The parameter is declared (template_preview.py line 740) and consumed (line 853), but sku does not appear in get_ops_instance_template's param_to_target map (targets.py lines 266-278), and there is no --sku argument anywhere in azext_edge/edge on this branch. Since _handle_apply_targets skips None, the parameter is simply omitted and the expression always resolves to null — harmless, but it does mean the capability is untestable here. Note that test/aio-essentials-sku-preview (#950 / #957) implements the same feature by mutating the resource in Python (instance.pop("sku", None) / instance["sku"] = {"name": self.sku} in targets.py) rather than through the ARM parameter. Whichever merges second will need to reconcile the two mechanisms — worth agreeing on one now.

4. mcpDefaultPolicy only fires on mcp.mode=Preview. The condition is [equals(tryGet(tryGet(parameters('features'), 'mcp'), 'mode'), 'Preview')]. parse_feature_kvp_nargs validates feature shape only — any {component}.mode key is accepted with values Stable, Preview or Disabled — so --feature mcp.mode=Stable is a valid invocation that enables MCP with no default authorization policy deployed. If Stable is not a supported MCP mode yet, consider rejecting it explicitly; if it is, the condition should probably be not(equals(..., 'Disabled')) to match how opcUaConnectorTemplate gates itself a few lines above.

Nits

5. create_default_opcua_template deep-copies a whole blueprint to read one list. blueprint = runtime_profile.copy_instance_blueprint() followed by blueprint.get_resource_by_key("opcUaConnectorTemplate")["properties"]["deviceInboundEndpointTypes"] copies the entire template per call. A read-only accessor on RuntimeProfile for the endpoint types would avoid the copy and make the dependency explicit.

Checked and clean

  • The preview connector version is now self-consistent: OPCUA_CONNECTOR_VERSION defaults to 1.5.12 in template_preview.py and PREVIEW_PROFILE.opcua_connector_version is "1.5.12", so the ARM create path and the CLI backfill path agree. GA is untouched at 1.4.14 in both common.py and template.py.
  • The mcpDefaultPolicy rename in targets.py is guarded with if mcp_default_policy is not None, so the GA blueprint — which has no such resource — is unaffected.
  • runtime_profile is threaded correctly end to end: Instances.update → _update(runtime_profile=profile) → the backfill call, and upgrade2 passes self.runtime_profile. Callers that omit it keep the single OPCUA_CONNECTOR_ENDPOINT_TYPE entry, matching the new docstring.
  • nullable: True without defaultValue is optional, not required — _handle_apply_targets (targets.py lines 220-222) already omits every None-valued parameter, which is how aioInstanceName and adrNamespaceId behave today. No deployment break from the new sku parameter.
  • mcpDefaultPolicy declares dependsOn: ["aioInstance", "customLocation"], consistent with its siblings.
  • No HISTORY.rst entry or constants.py VERSION bump is needed in this repo.

This is an automated review and may be incomplete; please treat the findings as input rather than as a gate.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since dfa98ca5 (head 56798fd5) — one commit, Refresh preview runtime to 22, 7 files (+104/-27).

Scope: the preview blueprint version/connector refresh in template_preview.py, the VERSIONS filter in targets.py, the catalog pin in runtime_catalog.py, and the four test modules that follow them. No blocking defects.

Blocking

None.

Suggestions

1. A >32-character instance name silently drops the MCP connection and leaves the policy orphaned.
The new resource is gated on two conditions:

# template_preview.py, "mcpAioConnection"
"condition": "[and(equals(tryGet(tryGet(parameters('features'), 'mcp'), 'mode'), 'Preview'),
              lessOrEquals(length(coalesce(parameters('aioInstanceName'), format('aio-{0}', variables('HASH')))), 32))]"

but its sibling mcpDefaultPolicy is gated only on the first:

"condition": "[equals(tryGet(tryGet(parameters('features'), 'mcp'), 'mode'), 'Preview')]"

So for az iot ops create --instance <33+ chars> with MCP preview enabled, the deployment succeeds, mcpAuthorizationPolicies/aio-mcp-policy-v1 is created, and the mcpServerConnections/aio that is its only consumer is not — an orphaned policy plus a silently missing feature, with no warning anywhere.

Nothing on the Python side knows about the limit either. InitTargets.__init__ caps only the custom location name (if len(custom_location_name) > 63: raise InvalidArgumentValueError(...)), and instance_name just goes through _sanitize_k8s_name, so 33+ characters is reachable. get_ops_instance_template then renames the resource unconditionally:

mcp_aio_connection = template.content["resources"].get("mcpAioConnection")
if mcp_aio_connection is not None:
    mcp_aio_connection["name"] = f"{self.instance_name}/aio"
    mcp_aio_connection["properties"]["service"]["name"] = f"{self.instance_name}-mcp"

Two options, either is fine: mirror the existing custom_location_name precedent and raise when MCP preview is requested with an over-length instance name (the explicit, discoverable choice), or at minimum emit a warning at template-build time. Silently skipping is the one outcome worth avoiding, since the user asked for the feature. Gating mcpDefaultPolicy on the same length condition would at least keep the two consistent.

If the 32 is a service-side name limit (e.g. {instance}-mcp or the connection resource), a short comment next to the literal would help — it currently reads as a magic number, and note that {instance_name}-mcp is 4 characters longer than the value actually being measured.

Nits

2. The TRAINS loop can KeyError if the two maps ever disagree.
In targets.py::get_extension_versions, the new filter guards the first loop but not the second:

for moniker in template_vars["VERSIONS"]:
    if moniker not in EXTENSION_TYPE_TO_MONIKER_MAP.values():
        continue
    version_map[moniker] = {"version": template_vars["VERSIONS"][moniker]}
for moniker in template_vars["TRAINS"]:
    version_map[moniker]["train"] = template_vars["TRAINS"][moniker]   # unguarded

Safe today — TRAINS is {"iotOperations": "integration"} and iotOperations is in the map — but the moment a non-extension moniker is added to TRAINS (exactly the kind of edit this commit just made to VERSIONS), the second loop raises KeyError instead of skipping. Applying the same continue guard, or iterating version_map instead, removes the asymmetry.

3. The blueprint now depends on a manual post-processing step. The new module docstring says:

The 1.6.0-preview.22 tag defaults to 1.6.0-preview.21. The approved runtime version override to 1.6.0-preview.22 is applied to exported Bicep before compiling.

So source_ref/source_commit alone no longer reproduce the committed blueprint — regenerating from 5e54c8e8 yields .21. The docstring is the right place to record it, but naming the exact override (which parameter/value is edited) would make this reproducible by someone who is not the author.


Checked and clean (recording the negatives so they don't get re-derived):

  • The connectors key cannot leak into extension versions. EXTENSION_TYPE_TO_MONIKER_MAP.values() is {cm, platform, ssc, acs, iotOperations} — connectors is absent, so the new VERSIONS entry is filtered out of get_extension_versions. test_preview_extension_versions_exclude_connector_metadata locks it with an exact-dict assertion, which is the right strength here: a looser in check would not catch a leak.

  • Hoisting the connector version into VERSIONS did not change the resolved value. OPCUA_CONNECTOR_VERSION moved from a hardcoded '1.5.12' fallback to variables('VERSIONS').connectors, which is "1.5.12" — same value, and the coalesce(tryGet(tryGet(parameters('advancedConfig'), 'connectors'), 'version'), ...) override path is untouched. The test assertion was also strengthened in the right direction: it previously did a substring check on the ARM expression string ("'" + version + "'" in ...), which would have kept passing against a stale literal; it now compares against the VERSIONS.connectors variable directly.

  • The new repository knob is plumbed end to end. advancedConfig.connectors.repository is declared in the _1.AdvancedConfig definition ("nullable": True, matching its version/registry/imageRegistry siblings), read by the new CONNECTORS_CHART_REPOSITORY variable with a default of aio-connectors/helmchart/microsoft-aio-connectors, and consumed as connectors.image.repository alongside the existing connectors.image.tag/.registry. No half-wired parameter.

  • The .19 → .22 requalification is deliberate and consistent. PREVIEW_PROFILE.identity.version moves to 1.6.0-preview.22, and test_unqualified_preview_integration_versions_are_not_retained was widened to include 1.6.0-preview.19 and .21, so the previously-bundled .19 is now explicitly not a qualified runtime. Worth being aware that anyone who deployed from an earlier iteration of this branch will hit resolve_runtime_identity's "no supported runtime profile mapping" on update/upgrade — acceptable on the integration train, but it is a real one-way step for existing qualification clusters. test_upgrade2_unit.py was updated to match, so the suites agree.

  • train is unchanged (TRAINS: {"iotOperations": "integration"}), so this is a version refresh and not a channel change.

  • "nullable": True on the instance features additionalProperties map is consistent with how the other optional object definitions in this blueprint are declared, and _apply_instance_features still binds through parameters('features') on the preview template, so the --feature path is unaffected.

  • No HISTORY.rst entry expected in this repo — CONTRIBUTING.md does not require one and HISTORY.rst on dev is still the "Initial baseline" stub. (This differs from azure-iot-cli-extension, where a VERSION bump without a matching section is a finding.)

  • Not restating the open inline threads from Copilot and Hangyi (@HangyiWang); the instances.py runtime-API selection one is marked fixed and the Storage API pin was explicitly deferred.

This is an automated review and may be incomplete.

Comment thread azext_edge/edge/providers/orchestration/resources/instances.py Outdated
Comment thread azext_edge/constants.py Outdated
Comment thread azext_edge/edge/_help.py Outdated
Comment thread azext_edge/edge/_help.py Outdated
Comment thread azext_edge/edge/params.py Outdated
Comment thread azext_edge/edge/providers/orchestration/upgrade2.py Outdated
Comment thread azext_edge/edge/providers/orchestration/upgrade2.py
Comment thread azext_edge/edge/providers/orchestration/template_preview.py Outdated
Comment thread azext_edge/edge/providers/orchestration/resources/instances.py Outdated
Comment thread azext_edge/edge/providers/orchestration/resources/instances.py
@digimaun

Paymaun (digimaun) commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

The linked integration run tested the #947 snapshot at ffa96ec on 9/26. Its test steps passed and only the preview edge job's storage cleanup failed. Since then the preview runtime moved from 1.6.0-preview.9 to 1.6.0-preview.22, and show, update, identity assign and remove, and secretsync enable and disable now select the management API from runtime discovery. Both channels should run against the final revision before merge.

The description should also note that changing opcua.mode with update --feature now keeps the instance's existing opcua settings, where dev replaced the component and dropped them.

Comment thread azext_edge/edge/providers/orchestration/targets.py Outdated
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 5fced44a (head 4f0a7d74)

One commit, Refresh GA and preview runtimes and simplify preview opt-in — 26 files. Scope: the 12 product files in the increment plus targets.py, instances.py, _help.py, params.py and az_client.py read at head for context. I did not re-read the 56-file base diff, and I am not restating the open inline threads from Paymaun (@digimaun).

Blocking

  1. azext_edge/edge/providers/orchestration/template.py — the stable blueprint now deploys from the integration train. variables.TRAINS goes {"iotOperations": "stable"} → {"iotOperations": "integration"} and VERSIONS.iotOperations 1.4.112 → 1.5.33, while every resource in that same blueprint moves from apiVersion 2026-07-01 to 2026-09-01-preview (aioInstance, broker, brokerAuthn, brokerListener, dataflowProfile, dataflowEndpoint, artifactRegistryEndpoint, opcUaConnectorTemplate). Net effect: a plain az iot ops create — no --use-preview — now provisions an integration-train build through preview API versions. The comment in runtime_catalog.py pre-authorises this ("in an alpha build this may still deploy from integration; it is not silently promoted to stable"), so I am flagging it for an explicit decision rather than calling it a defect: please confirm this is intended for an alpha build only, and that a shipping build will not go out with the GA profile on integration. This also answers Paymaun (@digimaun)'s open azext_edge/constants.py thread — the 2610 GA sync is in this PR — but the resulting AIO_RELEASE = "2610" now labels a runtime that is not on the stable train, which is a different question from the one that thread asked.

  2. template_preview.py — mcpDefaultPolicy and mcpAioConnection lost their condition with no Python-side replacement. Both previously carried condition: equals(tryGet(tryGet(parameters('features'), 'mcp'), 'mode'), 'Preview'); both are now unconditional, so every --use-preview create deploys the MCP authorization policy and the aio MCP server connection. Contrast with opcUaConnectorTemplate, whose dropped condition was replaced — targets.py (~line 338) pops that resource when opcua.mode=Disabled. Nothing analogous exists for the two MCP keys; targets.py only renames them (~lines 311‑317). If "MCP is always on in the preview runtime" is the intent, that is fine, but it should be stated, because mcpAioConnection also lost lessOrEquals(length(...aioInstanceName...), 32). That guard is still load-bearing: the template hardcodes service.name = "aio-mcp", but targets.py overwrites it with f"{self.instance_name}-mcp" whenever --name is supplied, and the CLI does not length-check the instance name (validate_resource_name in _validators.py only checks the character class, and it binds resource_name, not instance_name). A long --name therefore reaches whatever failure that guard was added to avoid.

  3. template_preview.py — apiVersion: "2026-09-02-preview" on the two MCP resources. Every other resource in that blueprint is 2026-09-01-preview, the preview profile declares iotops_api_version=IoTOpsMgmtApiVersion.V20260901_preview, and 2026-09-02-preview does not appear in the IoTOpsMgmtApiVersion enum (azext_edge/edge/util/az_client.py). It may well be a genuine second MCP preview, but a one-character slip here fails the whole --use-preview deployment at ARM validation, and nothing in the unit suite would catch it. Please confirm the RP serves 2026-09-02-preview.

Suggestions

  1. runtime_catalog.py — the stable profile's source_ref/source_commit can now drift from the blueprint it describes. They were TEMPLATE_BLUEPRINT_INSTANCE.commit_id on both fields; they are now the literals "releases/v1.5.x/2610" / "67613dc8b60abdfa08bfb138f1d8fa3cbef329f7", while the blueprint's own commit_id is ed3a14b063cdd62eed67f17c3aae452290f6986f. Naming the upstream bicep ref is more meaningful than echoing the capture commit (and it matches the preview profile's shape), so this is the right direction — but the two values used to be impossible to desync and now nothing ties them together. test_runtime_profiles_unit.py just echoes the new literals, so a future template refresh that forgets the catalog will report a stale commit through get-versions. Consider asserting in the sync skill / a unit test that the profile literals and instance_blueprint.commit_id were updated in the same change.

Nits

  1. azext_edge/edge/params.py:1269 — context.ignore("confirm_yes") is now dead. create_instance in commands_edge.py no longer declares confirm_yes, so knack never builds that argument for iot ops create and there is nothing for ignore to suppress. Harmless, but it reads as if --yes were still reachable on this command.

Checked and clean (recording the negatives so they are not re-derived)

  • The opcUaConnectorTemplate condition removal from both blueprints is safe for the CLI path, which matters given the 2608 regression (an unconditioned connector template hanging create to deployment timeout when OPC UA is disabled). targets.py pops the resource on opcua.mode=Disabled, and the old preview condition's features.opcua ?? features.connectors alias is unreachable from the CLI because COMPAT_FEAT_KEY_SET = {"opcua.mode"} (providers/orchestration/common.py:82) rejects any other key in ensure_feature_key_compat.
  • Converting the iot ops create help block to an f-string is safe: the 3.8k-character block contains exactly four braces, all of them the two intended {PREVIEW_NOTICE} / {PREVIEW_AGREEMENT_URL} substitutions — no literal {/} that would raise at import.
  • The consent-prompt removal is complete: zero remaining references to confirm_preview_creation, orchestration.preview, preview_notice or preview_agreement_url anywhere in the tree.
  • OPCUA_CONNECTOR_VERSION 1.4.14 → 1.5.18 (providers/orchestration/common.py) matches VERSIONS.connectors in the stable blueprint and the preview profile's opcua_connector_version, and the template now derives OPCUA_CONNECTOR_VERSION from variables('VERSIONS').connectors instead of a duplicated literal — one less drift site.
  • DEFAULT_IOTOPS_MGMT_API_VERSION = IoTOpsMgmtApiVersion.V20261001 resolves; V20261001 = "2026-10-01" is declared in the enum. This restores the October default per Paymaun (@digimaun)'s thread, though note the stable blueprint now deploys at 2026-09-01-preview while the control plane reads/writes at 2026-10-01 — worth keeping in mind for the round-trip PUT question still open on instances.py.
  • enableGdsManager / connectors.values.gdsManager.enabled are gone from both regenerated templates with no dangling references, matching the reply on that thread.

This is an automated review and may be incomplete; please treat the findings as input rather than a gate.

Comment thread .github/actions/build-int-test-matrix/build_matrix.py Outdated
Comment thread azext_edge/edge/providers/orchestration/upgrade2.py
Comment thread azext_edge/edge/providers/orchestration/template.py
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 4f0a7d7 (head 94fb661) — one commit, Fix preview CI, GA force upgrades, and broker checks, 7 files (+54/-136).

This increment responds directly to Paymaun (@digimaun)'s three inline comments from 02:26Z, so I am not restating them. Instead I verified whether each fix actually holds, and below is what that turned up.


Blocking

1. The --force restoration does not cover the case the integration GA train actually creates — upgrading from the previous GA build.
azext_edge/edge/providers/orchestration/upgrade2.py — the new escape sits at line 1355, but installed = resolve_runtime_identity(current_version, current_train, self.qualification_identities) still runs first at line 1345. For an installed instance whose releaseTrain is integration, resolve_runtime_identity (runtime_profiles.py:169-176) requires an exact (version, "integration") match in qualification_identities, otherwise it raises AIO runtime '<v>' on train 'integration' has no supported runtime profile mapping.

What is in that set on this head:

  • runtime_catalog.py:34 — QUALIFICATION_IDENTITIES: Tuple[RuntimeIdentity, ...] = ()
  • runtime_profiles.py:120-122 — the catalog folds in only profile.identity for profile in self._profiles.values() if profile.identity.train == "integration"

Both bundled blueprints are on the integration train (template.py:1490 → 1.5.33, template_preview.py:810 → 1.6.0-preview.43), so the qualified set is exactly those two versions. Consequences:

  • An instance created by an earlier alpha of this branch (any GA build before 1.5.33) fails at line 1345 before the new self.force block can be reached, so --force cannot rescue it.
  • This is not a one-off migration artifact: as soon as the GA blueprint moves 1.5.33 → 1.5.34, every existing 1.5.33/integration instance becomes unresolvable the same way. Upgrading from the immediately-preceding GA build will fail for every future refresh, with or without --force.

Paymaun (@digimaun)'s specific example (1.4.112 on stable → 1.5.33 --ops-train integration --force) does work now, because current_train == "stable" resolves without qualification at runtime_profiles.py:167. It is the integration→integration path that is still closed.

Suggested shape: either hoist the self.force escape above the installed resolution (or wrap that resolution so an unresolvable installed identity is tolerated under --force), or populate QUALIFICATION_IDENTITIES with the shipped GA baselines so prior builds remain resolvable.

Test gap for the same point: test_ga_catalog_train_change_preserves_force_behavior (test_upgrade2_unit.py) builds both built_in_version/built_in_train from catalog.get(RuntimeChannel.STABLE).identity, i.e. the target is always the one version the catalog qualifies, and current_train="stable". It cannot observe the failure above. A case with current_train="integration" and a current_version that is not target.version would lock the behaviour you actually want from --force.


Suggestions

2. The check/mq.py change is byte-identical to open PR #960, which targets the same base (dev) — and #948 carries only half of it.
I diffed the changed lines: this commit's hunks in azext_edge/edge/providers/check/mq.py (+1/-99) and azext_edge/tests/edge/checks/base/test_access_denied_unit.py (+0/-21) are the same lines as #960's. Two consequences worth deciding deliberately rather than at merge time:

  • Whichever PR lands second takes a conflict in both files.
  • fix(check/support): handle retired broker diagnostics service #960 also updates azext_edge/edge/providers/stats.py, providers/support/mq.py, providers/support/base.py, providers/base.py, edge/params.py, edge/_help.py, plus the test_mq_removal_regression_unit.py / test_mq_traces_unit.py regression suites and a HISTORY.rst entry. None of that is here. At this head, stats.py lines 16, 33 and 57 still use AIO_BROKER_DIAGNOSTICS_SERVICE as the diagnostics pod prefix, so if feat: support GA and preview runtimes #948 merges first, az iot ops check is fixed on 2610 while az iot ops broker stats and the support bundle still target the retired Service.

Given Ketki Naik (@ketkimnaik)'s reply says "equivalent check-side fix from #960 implemented", it may be cleaner to take #960 first and rebase, so the retirement lands as one coherent change with its regression tests.

3. _validate_version_upgrade lost its same-train condition, which widens --force further than the boundary fix needs.
upgrade2.py:1546 went from self.force and installed.channel == target.channel == STABLE and self.current_version[1].lower() == self.desired_version[1].lower() to just the channel test. Combined with the new escape in _validate_ops_boundary, upgrade --force --ops-train stable --ops-version <older> against an integration install now skips the cross-train guard and the downgrade/minor-skew checks (a stable train resolves to a STABLE identity for any version string). Previously the cross-train guard raised first. validate_upgrade_boundary's own docstring says it "deliberately has no force/preflight override", so please confirm this widening is intended rather than a side effect of making the GA train move work.

4. "integration" is hardcoded again at upgrade2.py:1356.
That is now the fourth literal (runtime_profiles.py:169, template.py:1490, template_preview.py:810), and RuntimeChannel has no INTEGRATION member to anchor it. A shared module-level constant would keep the next train rename from silently leaving one site behind — the same drift class flagged on the API-version pins.


Checked and clean

Recording these so they do not get re-derived:

  • The --yes removal is correct, and it withdraws an earlier nit of mine. azext_edge/edge/params.py:1269 does context.ignore("confirm_yes") inside with self.argument_context("iot ops create"), so --yes really is rejected by az iot ops create even though the group-level argument exists at params.py:77-82. In my review of 4f0a7d7 I called that context.ignore dead code now that create_instance dropped the kwarg — that was wrong; it is the line that makes the rejection happen, and this commit's CI fix is the evidence. Withdrawn.
  • No stray --yes left on a create path. The only remaining occurrences are test_integration_support_unit.py:54 (a baseline fixture's own create_args, not the channel selector) and the new negative assertion at line 906.
  • check/mq.py imports are consistent after the removal. NewLine is dropped and has zero remaining uses; get_namespaced_service, build_access_denied_text, ClusterAccessDeniedError, CheckTaskStatus and List are all still used by the listener-service path. AIO_BROKER_DIAGNOSTICS_SERVICE has no remaining reference anywhere under providers/check/.
  • The new force parametrization on test_reconcile_cannot_bypass_release_train_guard is meaningful, not decorative. Both cases drive a preview target train, which fails the new escape's target_train.lower() in (STABLE, "integration") test, so the Cross-train upgrades guard still raises under --force — the preview direction stays closed.
  • The new GA test builds from get_runtime_catalog() directly, which addresses Paymaun (@digimaun)'s "the unit tests miss it because mocked_upgrade_manager rewrites integration to stable".
  • qualification_identities is plumbed through the new build_ext_upgrade_state kwarg to ExtensionUpgradeState (upgrade2.py:1162, 1176), matching the production wiring at upgrade2.py:208.

This is an automated review and may be incomplete; it is scoped to the single commit 94fb661 and does not re-review the rest of the PR.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 94fb6615 (head 2d7865a5) — one commit, Fix preview listener API, 3 files +64/-5 (targets.py, template.py, test_runtime_profiles_unit.py).

The change itself is correct and the second new test is a genuine lock. My one substantive point is that the fix is gated to the preview channel while the same drift now also exists on the GA channel — and the first new test pins it there.

Blocking

  1. azext_edge/edge/providers/orchestration/targets.py (lines 332-340) — the GA/stable path keeps the exact mismatch this commit fixes, and this PR is what introduced it.
    The new api_version= argument is supplied only when self.runtime_profile.channel == RuntimeChannel.PREVIEW; otherwise get_insecure_listener falls back to DEFAULT_IOTOPS_MGMT_API_VERSION. On this head that default is IoTOpsMgmtApiVersion.V20261001 = 2026-10-01 (azext_edge/edge/util/az_client.py, lines 210/219), but the stable profile's blueprint is TEMPLATE_BLUEPRINT_INSTANCE (runtime_catalog.py line 47), and every Microsoft.IoTOperations/* resource in that blueprint — including brokerListener itself — is declared at 2026-09-01-preview (template.py, the brokerListener entry at ~line 1647, plus the resources at ~1592/1608/1632/1676/1684/1707/1718).
    So az iot ops create --add-insecure-listener on the GA channel still emits brokerListenerInsecure at 2026-10-01 sitting next to brokerListener at 2026-09-01-preview in the same deployment — one API version apart, in one template, for two instances of the same resource type.
    This is new in this PR, not pre-existing: on dev the two agree (DEFAULT_IOTOPS_MGMT_API_VERSION = IoTOpsMgmtApiVersion.V20260701 and the blueprint's brokerListener is "2026-07-01"), which is exactly why the unconditional DEFAULT_IOTOPS_MGMT_API_VERSION was safe before and is not now.
    The channel gate does not look load-bearing — broker_listener = template.get_resource_by_key("brokerListener") is fetched unconditionally ~45 lines above (line ~285) for both channels and for the runtime_profile is None fallback, so passing broker_listener["apiVersion"] with no gate works everywhere and makes the optional listener inherit whichever blueprint is in play. If the GA listener genuinely must stay on 2026-10-01 while its sibling is on 2026-09-01-preview, that deserves a comment, because it reads as an oversight.

Suggestions

  1. Two sources of truth for the per-channel API version. RuntimeProfile.iotops_api_version already carries this value per channel — default 2026-10-01 for stable (runtime_profiles.py line 69) and IoTOpsMgmtApiVersion.V20260901_preview for preview (runtime_catalog.py line 32) — and it is what resources/instances.py (lines 232-238) uses to re-point the instance client. This commit instead derives the template's value from the blueprint resource. Both are defensible (the blueprint is the source of truth for the template, iotops_api_version for the client), but they can now disagree silently, and on stable they already do. Worth a one-line comment on get_insecure_listener's new parameter saying which one it is meant to track.

  2. azext_edge/tests/edge/orchestration/test_runtime_profiles_unit.py — test_optional_listener_uses_selected_blueprint_api is the only guard on the non-preview case, and it asserts the drift. The for key, resource in original["resources"].items() loop cannot cover brokerListenerInsecure (that key is added by targets.py and is not in the blueprint), so the whole non-preview assertion is the hardcoded assert expected["apiVersion"] == "2026-10-01". If finding 1 is intentional, please at least assert it as DEFAULT_IOTOPS_MGMT_API_VERSION.value rather than a literal, so the next default bump does not turn a deliberate decision into a failing literal that gets "fixed" by editing the string.

Nits

  1. test_preview_optional_listener_follows_blueprint_api_changes uses dataclasses.replace(original_profile, instance_blueprint=blueprint). That is valid here — instance_blueprint is an InitVar so replace requires it, and identity / _instance_blueprint are field(init=False) and get rebuilt by __post_init__ from the mutated blueprint — but it is non-obvious enough to be worth a one-line comment, since the test would silently stop proving anything if RuntimeProfile ever moved the blueprint off InitVar.

Checked and clean (recording the negatives so they are not re-derived):

  • get_insecure_listener has exactly one non-test call site (targets.py line 332), so no other caller needs the new argument.
  • broker_listener is resolved before the add_insecure_listener block for every path, so the new expression cannot KeyError or depend on ordering.
  • api_version or DEFAULT_... in template.py is safe: "" is not a reachable value from the only caller, which passes either a blueprint apiVersion or None.
  • test_preview_optional_listener_follows_blueprint_api_changes is a real lock rather than a tautology — it mutates the blueprint to three values the code has never seen and asserts the emitted listener follows each.
  • Preview TEMPLATE_BLUEPRINT_INSTANCE_PREVIEW's brokerListener is 2026-09-01-preview (template_preview.py line 965), so the preview half of the fix lands on the version the preview profile declares.
  • Test-only/azext_edge change needs no HISTORY.rst entry in this repo.

I did not restate digimaun's three open inline threads (4213984573, 4213984676, 4213984766); note that 4214667638 (04:29Z) reasserts the --force / integration-train regression after the previous iteration, and this increment does not touch upgrade2.py, so that one is still open.

This is an automated review and may be incomplete; please verify findings before acting on them.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 7e34485e (head 902772e7) — 2 commits, 5 files (instances.py, runtime.py, runtime_profiles.py, upgrade2.py, test_upgrade2_unit.py).

Context: this increment answers Paymaun (@digimaun)'s inline thread on upgrade2.py:1372. I verified all three bullets from comment 4223118554 are addressed, and am not restating that thread:

  • --ops-version 1.5.40 --force without --ops-train — fixed by the new train inheritance in desired_version (upgrade2.py L1195-1201), which makes target_train resolve to the installed integration rather than the bundled train. Covered by the new case ("1.5.40", "integration", True, "1.5.40", None, None).
  • --ops-version 1.6.0-preview.50 --ops-train integration --force from 1.6.0-preview.43 and from 1.6.0-preview.50 — fixed by the allow_unbundled_integration fallback in resolve_runtime_identity (runtime_profiles.py L178-182). Covered by cases 9 and 10.
  • Plain upgrade failing during discovery — fixed by allow_unbundled_integration=True on the get_runtime_context call in Upgrade.__init__ (upgrade2.py L141-148). Covered by ("1.5.40", "integration", False, None, None, None).

Blocking

  1. azext_edge/edge/providers/orchestration/upgrade2.py L1364-1370 — upgrade --force with no --ops-version still fails on an unbundled integration GA build, and fails where the same command without --force succeeds.

    _resolve_ops_identity (L1338-1342) sets allow_unbundled_integration = installed or bool(self.force and self.override.version). The forced-stable early-return branch calls it with neither flag set when the user passes --force alone, so the resolve at L1368 runs with allow=False.

    Concrete trace, using the values actually bundled on this head (template.py L1489-1490 → 1.5.33/integration; template_preview.py L809-810 → 1.6.0-preview.43/integration; runtime_catalog.py L34 QUALIFICATION_IDENTITIES = (), so qualification_identities is exactly those two profile identities via RuntimeProfileCatalog.__init__ L120-122). Instance installed at 1.5.40 on integration, i.e. the release-candidate-newer-than-the-CLI case this whole feature exists for:

    • L1355 installed = _resolve_ops_identity("1.5.40", "integration", installed=True) → allow=True → STABLE/1.5.40/integration. Fine.
    • L1358-1360 target_version = max("1.5.40", "1.5.33") = "1.5.40".
    • L1361-1363 _has_delta_in_train() is False (bundled train is also integration), so target_train = "integration".
    • L1364 self.force and installed.channel == STABLE and "integration" in (...) → branch entered.
    • L1368 _resolve_ops_identity("1.5.40", "integration") → allow = False or bool(True and None) = False → no qualification match → ValidationError: AIO runtime '1.5.40' on train 'integration' has no supported runtime profile mapping.

    Without --force the branch is skipped and L1373-1377 reuses installed (because override.version is None and (target_version, target_train) == (current_version, current_train)), so it passes — that is the new passing case ("1.5.40", "integration", False, None, None, None). Adding force=True to that exact parametrization turns it into an error, which is why the existing suite does not catch it.

    The same installed-reuse shortcut you added at L1373-1377 looks like the right fix at L1368 too, or make the resolve at L1368 use installed=True semantics when target_version == current_version. Note this is STABLE-channel-only: an unbundled preview install skips the L1364 branch (it requires installed.channel == RuntimeChannel.STABLE) and falls through to the installed reuse, so 1.6.0-preview.50 + --force with no version is fine. Suggested coverage: add ("1.5.40", "integration", True, None, None, ...) alongside the existing force=False case.

Suggestions

  1. azext_edge/edge/providers/orchestration/runtime_profiles.py L178-182 — the fallback builds RuntimeIdentity(channel, version, "integration"), which re-enters RuntimeIdentity.__post_init__ (L52-58). For the PREVIEW branch that enforces version.patch == 0 and re.fullmatch(r"preview\.[1-9][0-9]*", ...). So an integration build such as 1.6.1-preview.2 or 1.6.0-rc.1 still hard-fails — now with Version '1.6.1-preview.2' conflicts with the preview runtime profile. rather than the mapping error. That is deliberate for override values (cases 21 and 22 lock it), but it also applies to discovery of an already-installed build via resolve_runtime → Upgrade.__init__, where the user has no way to proceed at all. If patch-level or non-preview.N RC builds are in scope for the "test an RC with a released CLI" workflow, this is the remaining hole; if they are not, it is worth a comment saying so.

  2. azext_edge/edge/providers/orchestration/upgrade2.py L1193-1201 + new case ("1.4.112", "stable", True, "1.5.40", None, None) — this now accepts pinning an integration-only version while leaving the extension on the stable train: the test asserts patches[0]["version"] == "1.5.40" and "releaseTrain" not in patches[0]. Validation passes because the inherited train is stable (L1196-1197) and L1368 resolves ("1.5.40", "stable") through the public-train shortcut at runtime_profiles.py L169-170, which never consults the qualification set. Worth confirming the Arc extension PATCH can actually resolve that chart on the stable train — if it cannot, this trades a clear client-side error for an opaque server-side one, and the new test locks the behavior in.

Nits

  1. azext_edge/edge/providers/orchestration/upgrade2.py L1338 — the installed: bool keyword shadows the installed: RuntimeIdentity local in both callers (_validate_ops_boundary L1355, _validate_version_upgrade L1556). Something like is_installed_runtime would read better, and the combined expression installed or bool(self.force and self.override.version) folds two distinct policies ("this is the live runtime" vs "the user explicitly forced a pin") into one line.

Summary: the three paths Paymaun (@digimaun) reported are genuinely fixed and well covered by the 23 new command-level parametrizations. The one remaining defect is --force with no --ops-version on a stable-channel unbundled integration build (finding 1), which is a strict regression against the --force-less path that this same commit makes work. No changes needed for HISTORY.rst or constants.py — no CLI surface changed in this increment.

This is an automated review and may be incomplete; please verify findings before acting on them.

Comment thread azext_edge/edge/providers/orchestration/upgrade2.py Outdated
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 902772e7 (head 23880ef8)

One commit, one line: azext_edge/edge/providers/orchestration/upgrade2.py drops , including with --force from the ">1 minor ahead" preview error. This is the change digimaun asked for (--force is hidden on upgrade, so the message should not advertise it).

Blocking

None introduced by this commit.

Suggestions

None.

Nits

None.

Checked and clean

  • The shortened message is still accurate for the preview path. _validate_version_upgrade only short-circuits on --force at the if self.force and installed.channel == target.channel == RuntimeChannel.STABLE: return guard, so a PREVIEW install never reaches that early return and --force genuinely cannot bypass either preview rule. Dropping the clause removes information, not correctness.
  • Wording is now consistent with its sibling "Preview upgrades across major versions are not supported." two lines up.
  • No test breakage: the three assertions that pin this string match on the substring more than one minor version, which survives the edit — azext_edge/tests/edge/orchestration/test_upgrade2_unit.py (the 1.8.0-preview.1 case and the ("preview", "integration", True, "1.8.0-preview.50", ...) case) and azext_edge/tests/edge/orchestration/test_runtime_commands_unit.py (1.6.0-preview.10 → 1.8.0-preview.11). Grepping the tree at 23880ef8 for the old literal returns no remaining references.

Still open from the previous review (not re-derived here)

The --force without --ops-version blocker I raised on 902772e7 is unchanged by this commit, and digimaun independently reproduced it at this exact head in his latest inline comment. _resolve_ops_identity computes allow_unbundled_integration=installed or bool(self.force and self.override.version), so with --force and no --ops-version the flag is False, and _validate_ops_boundary's forced-stable branch then calls self._resolve_ops_identity(target_version, target_train) with allow=False — hence no supported runtime profile mapping on an unbundled integration build. Flagging only so it is not lost behind the wording fix; no new analysis needed.

Summary: cosmetic, responsive to the request, and verified safe. Nothing to change in this increment.

This is an automated review and may be incomplete.

Comment thread azext_edge/edge/providers/orchestration/targets.py
Comment thread azext_edge/edge/params.py Outdated

@digimaun Paymaun (digimaun) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved with a couple suggestions.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 23880ef (head 4d8f29d4)

Increment is one product commit plus a dev merge:

  • 470281d9 "Reuse installed runtime for upgrade reconciliation" — upgrade2.py (+9/-7), test_upgrade2_unit.py (+58/-3)
  • 4d8f29d4 merge of dev — touches only .github/workflows/codeql.yml (+1/-1), no PR-file impact

I am not restating Paymaun (@digimaun)'s inline threads; this review verifies the fix against the code and reports what is new.

Blocking

None.

Suggestions

  1. azext_edge/edge/providers/orchestration/upgrade2.py — the train-only PATCH now reachable here skips _require_manual_upgrade(). With this commit, upgrade --ops-train integration on an unbundled-integration instance whose installed version is at or ahead of the bundled one takes this route: _has_delta_in_version() is False (desired_version[0] is the bundled 1.5.33/1.6.0-preview.43, which is not greater than 1.5.40/1.6.0-preview.50) and provisioningState is Succeeded, so _validate_and_resolve_version returns None without ever calling _require_manual_upgrade; get_patch then appends releaseTrain from _has_delta_in_train(). The result is a {"releaseTrain": "integration"} PATCH issued against an extension with autoUpgradeMinorVersion: true. _require_manual_upgrade is only invoked from the version-delta and non-success branches, so nothing checks the pin state on a train-only move — and moving the train while auto-upgrade is enabled lets Arc pick a version on its own, which is the class of uncontrolled move that guard exists to prevent. The new test pins this as intended behaviour (auto_upgrade parametrized [False, True, None], asserting success for Succeeded + ops_train), so if it is deliberate it is worth a comment; the path was unreachable for unbundled integration before this commit because the target resolve raised first.

  2. azext_edge/tests/edge/orchestration/test_upgrade2_unit.py — the new test_unbundled_installed_runtime_force_and_repair_contract only covers installed runtimes ahead of the bundle (1.5.40 > bundled 1.5.33; 1.6.0-preview.50 > bundled 1.6.0-preview.43), which is the branch where _resolve_ops_target_identity returns installed. The other side of that if — installed version below the bundled one, where the helper delegates to _resolve_ops_identity — is not exercised for an unbundled integration install. It resolves today only because RuntimeProfileCatalog.__init__ folds the bundled integration-train profile identities into qualification_identities, i.e. correctness depends on a second mechanism entirely. A current_version row below the bundled version would lock both branches of the new helper.

Nits

  1. azext_edge/edge/providers/orchestration/upgrade2.py — _resolve_ops_target_identity calls train.lower() before delegating, so it bypasses resolve_runtime_identity's if not isinstance(train, str) or not train: raise ValidationError("Unable to determine AIO release train.") guard; a None train would surface as AttributeError instead. Not reachable from the three current call sites (_validate_ops_boundary validates current_train up front and _has_delta_in_train() requires desired_version[1] truthy; _validate_version_upgrade resolves installed first, which raises on a missing train), so this is purely defensive.

Verification of the fix

The open ask from inline comment 4224200946 / 4224406505 is genuinely closed, and the mechanism is the one that was suggested:

  • The forced-GA branch now calls _resolve_ops_target_identity(installed, target_version, target_train) instead of _resolve_ops_identity(...). For upgrade --force with no --ops-version on 1.5.40/integration: override.version is None and target_version is max("1.5.40", "1.5.33") = "1.5.40", which equals installed.version, and target_train.lower() equals installed.train — so the helper returns installed (resolved with installed=True, hence allow_unbundled_integration=True), channel is STABLE, and the branch returns early. No second resolve, so allow_unbundled_integration=False is never hit.
  • The parametrize row flipped from ("1.5.40", "integration", True, None, "integration", "supported runtime profile mapping") to ... None, plus the two requested force=True rows next to the existing force=False ones, for both the GA and preview unbundled versions. That is exactly the regression lock asked for.
  • Train comparison is now case-insensitive on both sides (train.lower() vs installed.train, which RuntimeIdentity.__post_init__ lowercases), where the replaced inline expression compared the raw current_train. Strictly wider, no case where it newly misses.
  • The target = None / target = target or ... restructure is sound: RuntimeIdentity is a frozen dataclass with no __bool__/__len__, so it is always truthy and the force branch's already-computed target is reused rather than recomputed; the helper is deterministic, so reuse is equivalent to the previous recomputation.

Checked and clean

  • _validate_version_upgrade's switch to the helper is safe: installed there is resolved with installed=True, and when the helper returns it the subsequent installed.channel == target.channel and version comparisons are identity comparisons, so no preview minor/major rule can misfire.
  • Returning installed early from the force branch skips validate_upgrade_boundary, but with target is installed both the channel/train equality and the downgrade check are trivially satisfied.
  • No new finding on QUALIFICATION_IDENTITIES = () in runtime_catalog.py: bundled targets still resolve because RuntimeProfileCatalog.__init__ appends every profile identity whose train == "integration", and both blueprints ship TRAINS: {"iotOperations": "integration"} (template.py 1.5.33, template_preview.py 1.6.0-preview.43).
  • Context for suggestion 2: UpgradeScenario._build_defaults rewrites the ops extension's train to "stable" whenever the real value is not stable, so the default scenario matrix never exercises the shipped integration train. The explicit current_train="integration" rows in these two tests are the only coverage of it, which is why their parametrization carries more weight than usual here.
  • The dev merge is base-branch churn only (codeql.yml action bump); nothing in the PR's own file set moved.
  • No HISTORY.rst / version-bump convention finding applies in this repo.

This is an automated review and may be incomplete.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 4d8f29d4 (head 2287fccd)

Increment is one commit, Remove obsolete create confirmation ignore, touching a single line: context.ignore("confirm_yes") removed from the iot ops create argument context in azext_edge/edge/params.py (~line 1266). This directly answers digimaun's inline comment on that file, so I am not restating it.

Blocking

None.

Suggestions

None.

Nits

None.

Checked and clean

  • The ignore really was dead, and removing it changes no CLI surface. confirm_yes is registered once, on the iot ops group context (params.py ~line 78, --yes/-y, get_three_state_flag()), and azure-cli only materialises an argument for a command whose handler declares it. create_instance in azext_edge/edge/commands_edge.py (line 155) does not declare confirm_yes — its only catch-all is **kwargs, which does not generate argparse arguments — so --yes was never surfaced on iot ops create, before or after this commit.
  • Net effect versus the base branch is zero. context.ignore("confirm_yes") does not exist in params.py on dev, and create_instance on dev likewise has no confirm_yes; the line was introduced earlier in this PR and is now withdrawn. params.py at head now contains no context.ignore(...) calls at all.
  • No confirmation path is left stranded. should_continue_prompt (azext_edge/edge/util/common.py line 216) is the single consumer of confirm_yes, and work.py — the create orchestration path this PR rewrites — contains no prompt, confirmation, or input() call, so there is no create-time prompt that would now be unbypassable in CI.
  • The four real confirm_yes consumers are untouched: upgrade_instance, delete, clone_instance, migrate_assets in commands_edge.py still declare and forward it, alongside the schema / mq / dataflowgraph / registry-endpoint / mgmt-actions command modules.
  • Grepped the full tree at 2287fcc for confirm_yes: no test, help string, or doc references the create-specific ignore, so nothing is left dangling by the removal.

This is an automated review and may be incomplete; please verify findings before acting on them.

@ketkimnaik
Ketki Naik (ketkimnaik) merged commit fbe4b36 into Azure:dev Oct 9, 2026
15 checks passed
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.

7 participants