{AKS} az aks maintenancewindow: Add CRUD commands for peer MaintenanceWindow resources#9927
Conversation
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks maintenancewindow | sub group aks maintenancewindow added |
|
Hi @ibaboulfetouh, |
microsoft-github-policy-service
Bot
requested review from
yanzhudd,
yonzhan and
zhoxing-ms
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Azure CLI support in aks-preview for managing the new MaintenanceWindow peer ARM resource (preview API), including underlying resource construction helpers and parameter/help wiring.
Changes:
- Introduces
az aks maintenancewindowCRUD +waitcommands and client factory wiring. - Adds
maintenancewindow.pyhelper module to construct the SDK resource and detect schedule-arg presence. - Adds unit tests covering schedule construction and validation behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/aks-preview/azext_aks_preview/tests/latest/test_maintenancewindow.py | Adds unit tests for MaintenanceWindow resource construction and schedule-arg detection. |
| src/aks-preview/azext_aks_preview/maintenancewindow.py | Implements MaintenanceWindowResource construction and schedule-arg detection helpers. |
| src/aks-preview/azext_aks_preview/custom.py | Adds CLI handler implementations for maintenancewindow list/show/create/update/delete. |
| src/aks-preview/azext_aks_preview/commands.py | Registers the new aks maintenancewindow command group and operations binding. |
| src/aks-preview/azext_aks_preview/_params.py | Defines CLI arguments for maintenancewindow create/update. |
| src/aks-preview/azext_aks_preview/_help.py | Adds help text and examples for the new command group. |
| src/aks-preview/azext_aks_preview/_client_factory.py | Adds a client factory for maintenance windows. |
| src/aks-preview/HISTORY.rst | Documents the new command group in Pending release notes. |
| def aks_maintenancewindow_list( | ||
| cmd, # pylint: disable=unused-argument | ||
| client, | ||
| resource_group_name=None, | ||
| ): | ||
| if resource_group_name is None or resource_group_name == "": | ||
| return client.list_by_subscription() | ||
| return client.list(resource_group_name) |
There was a problem hiding this comment.
The vendored SDK MaintenanceWindowsOperations exposes list(resource_group_name) (returns ItemPaged[MaintenanceWindowResource]) and list_by_subscription() — confirmed in azext_aks_preview/vendored_sdks/azure_mgmt_preview_aks/operations/_operations.py line 9674. There is no list_by_resource_group method on this operations group.
Empirically confirmed by running az aks maintenancewindow list -g <rg> locally: the call resolved through to ARM and returned InvalidResourceType (RP-side; the test sub doesn't have the AKSSharedMaintenanceWindowPreview AFEC flag registered) rather than a Python AttributeError, which proves the SDK method exists and was invoked.
| if location is None: | ||
| existing = client.get(resource_group_name, maintenance_window_name) | ||
| raw_parameters["location"] = existing.location | ||
| if tags is None: | ||
| # Preserve existing tags on a schedule-only update so we don't | ||
| # accidentally clear them — PUT replaces the resource. | ||
| raw_parameters["tags"] = existing.tags |
There was a problem hiding this comment.
Good catch — the refetch-and-merge code that motivated this comment shouldn't have been there in the first place; it conflated tags-only PATCH with full PUT. Removed in 00d5c5f. The contract is now strict and documented in the help:
- tags-only → PATCH (sync,
--no-waitignored) - any schedule arg → full PUT; caller owns the entire resource body
PUT replaces the resource; tags must be re-passed if the caller wants to keep them. Updated _help.py long-summary on aks maintenancewindow update to state this explicitly. --location still defaults to the RG location when omitted (same fallback as create).
| schedule_args_present = hasAnyScheduleArg(raw_parameters) | ||
|
|
||
| # Tags-only fast path: PATCH via update_tags (sync). The CLI argument | ||
| # parser accepts --no-wait on every LRO-capable command, but the | ||
| # tags-only API is synchronous, so honor the flag by warning the user | ||
| # we ignored it rather than silently dropping it. | ||
| if not schedule_args_present: | ||
| if tags is None: | ||
| raise RequiredArgumentMissingError( | ||
| "Nothing to update. Provide --tags for a tags-only PATCH, or any of " | ||
| "--schedule-type / --interval-* / --day-of-week / --day-of-month / " | ||
| "--week-index / --duration / --utc-offset / --start-date / --start-time " | ||
| "for a full PUT." | ||
| ) |
There was a problem hiding this comment.
Addressed in 00d5c5f via option (b) — the help long-summary now states the minimum required set explicitly: --schedule-type + --start-time + --duration + per-type fields (--interval-weeks / --day-of-week / etc.).
Not pursuing option (a) (refetch + merge missing schedule fields) — that would re-introduce the same "implicit hidden state" pattern that comment #2 was flagging on tags, and would conflict with the PATCH-or-full-PUT contract the command intentionally documents.
…-PUT contract The first iteration of `aks_maintenancewindow_update` quietly refetched the existing resource on the schedule-change path to fill in `location` and preserve `tags` when omitted. That conflated tags-only PATCH with full PUT and contradicted the documented contract: - tags-only -> PATCH (sync, --no-wait ignored) - any schedule arg -> full PUT; caller owns the entire resource body Bringing the code back in line: the schedule path no longer GETs the existing resource. --location still defaults to the RG location when omitted (same fallback as create). Help text now states explicitly that PUT replaces the body and tags must be re-passed if the caller wants to keep them. Addresses Copilot review comment on PR Azure#9927 (tags-cleared edge case).
|
AKS |
| short-summary: Commands to manage MaintenanceWindow peer ARM resources. | ||
| long-summary: | | ||
| MaintenanceWindow is a resource-group-scoped ARM resource that defines a | ||
| reusable maintenance schedule. Once the `maintenanceWindowId` field is |
There was a problem hiding this comment.
nit: When we talk about it being available - do we think that's a detail customers need here?
We could just say "Users can link this resource to a pre-existing maintenanceconfiguration via the maintenanceWindowId. Multiple managedCluster maintenance configurations to a single MaintenanceWindow so the schedule lives in one place"
There was a problem hiding this comment.
Done in fc075fb. Rewrote the long-summary per your suggestion (drop the "Once available" framing in favor of a customer-focused description). New text:
"MaintenanceWindow is a resource-group-scoped ARM resource that defines a reusable maintenance schedule. Users can link this resource to a pre-existing maintenanceConfiguration via
maintenanceWindowId, so multiple managedCluster maintenance configurations share a single MaintenanceWindow and the schedule lives in one place."
| g.custom_command("list", "aks_maintenancewindow_list") | ||
| g.custom_show_command("show", "aks_maintenancewindow_show") | ||
| g.custom_command("create", "aks_maintenancewindow_create", supports_no_wait=True) | ||
| g.custom_command("update", "aks_maintenancewindow_update", supports_no_wait=True) |
There was a problem hiding this comment.
Double confirmation on delete
The command is registered with confirmation=True and the handler manually calls prompt_y_n. Users will be prompted twice. We should pick one way to confirm
There was a problem hiding this comment.
+1. confirmation=True in commands.py registration gives users a uniform --yes flag handled by the CLI core (which honors AZURE_CORE_DISABLE_CONFIRM_PROMPT and --yes consistently with every other AKS delete), while the handler-side prompt_y_n is hand-rolled. Drop the handler-side prompt and rely on confirmation=True — the yes parameter on the handler can also go away then.
There was a problem hiding this comment.
Done in fc075fb. Kept confirmation=True on the command registration (auto-injects --yes/-y via CLI core, respects AZURE_CORE_DISABLE_CONFIRM_PROMPT), dropped the handler-side prompt_y_n and the yes kwarg. Matches aks delete / aks nodepool delete. Verified with az aks maintenancewindow delete --help — single --yes -y arg now.
| # Full PUT path: the caller owns the resource body. No refetch / merge — | ||
| # this is a true PUT, mirroring `aks_maintenancewindow_create`. Default | ||
| # --location to the RG location when omitted (same fallback as create). | ||
| if location is None: |
There was a problem hiding this comment.
If the window was created in a different location, the PUT may attempt an invalid location change. Maybe we should fetch the existing resource's location?
There was a problem hiding this comment.
confirmed: ARM rejects PUTs that change the location of a TrackedResource (it's immutable post-create). Defaulting to RG location on update will hard-fail any MW that was created in a non-default region.
Two-line fix without going full read-modify-write: in aks_maintenancewindow_update, when location is None, fetch client.get(rg, name).location instead of get_rg_location. The fetch is one round-trip; cheap. Better still: combine with the not_allowed_dates fix and refactor update to read-modify-write end-to-end.
There was a problem hiding this comment.
Done in fc075fb — subsumed by the RMW refactor (see thread 3392454224 for the framing). aks_maintenancewindow_update now does client.get() first and uses existing.location for the PUT body. If --location is supplied with a different value than the existing one, we warn and keep the existing location rather than letting ARM 400 the PUT.
Test: test_location_mismatch_is_warned_not_erroring pins this — supplying --location eastus on a westus2 window emits a warning and the PUT body keeps westus2.
| operation_group="maintenance_windows", | ||
| ) | ||
|
|
||
| properties = MaintenanceWindowResourceProperties( |
There was a problem hiding this comment.
The constructed resource body never includes not_allowed_dates, existing blackout dates would get erased on schedule updates
There was a problem hiding this comment.
+1 — and the same applies to any future schedule fields beyond what constructMaintenanceWindowResource enumerates. The structural fix is either: (a) accept a --not-allowed-dates arg and add it to construct, OR (b) for an update semantics, refetch the existing MW, deep-merge user-supplied fields onto it, then PUT — which is the conventional CLI update shape (matches az aks update, az aks nodepool update, etc.). Option (b) also resolves @anushkasingh16's location-mismatch concern below in one go.
The current shape is functionally "replace" with a fast-path tags-only PATCH, named update. That's surprising — users expect update to be read-modify-write.
There was a problem hiding this comment.
Both addressed in fc075fb. Two-part fix:
-
updateis now read-modify-write — Ye's suggestion (thread 3398625216). The whole shape now matchesaz aks update/az aks nodepool update/az containerapp update(all do fetch → merge → PUT). After surveying the CLI,update_tags()is only used byaks nodepool snapshot updatebecause snapshots have no other mutable fields; there's no precedent for the "PATCH if tags-only, PUT otherwise" branching shape we had. Refetch picks upexisting.properties.not_allowed_datesfor free, so any subset-of-fields update preserves blackout dates without per-field merge code. Also future-proofs against new MW properties the RP adds. -
--config-filesupport added (MTC parity) — for the case where users do need to changenotAllowedDates. Mirrorsaks maintenanceconfiguration add --config-fileexactly: JSON-suppliedpropertiesblock replaces the existingpropertieswholesale on update. Help carries an example JSON withnotAllowedDates.
Tests: test_tags_only_update_preserves_schedule_and_dates asserts the blackout dates are preserved on a tags-only update; test_config_file_replaces_properties_keeps_tags_location asserts the JSON path works end-to-end.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| } | ||
|
|
||
|
|
||
| class TestConstructMaintenanceWindowResource(unittest.TestCase): |
There was a problem hiding this comment.
glad to see these unit test cases. In addition, could we include an end-to-end scenario test as well?
There was a problem hiding this comment.
Added in the latest revision — AksMaintenanceWindowScenarioTest::test_aks_maintenancewindow_crud in this file. It's a recorded ScenarioTest that exercises the full lifecycle end-to-end against ARM: create (weekly schedule + tags) → show (asserts the schedule/tags round-trip) → update (--duration change, asserts the existing schedule/tags are preserved) → list → delete → show (expects 404). The cassette is committed alongside so it replays offline in CI.
|
First-pass review on commit Replied inline on 3 of @anushkasingh16's threads (not_allowed_dates erasure, location-immutability on PUT, double-confirmation on delete) — all real bugs, all share a root cause: the Recommend: refactor One additional concern of my own:
Otherwise the CRUD shape, the |
|
Thanks for the structural framing — the RMW refactor lands all three of the cross-thread bugs (A2 double-confirm aside) with one change, and aligns the command shape with every other multi-field-mutable resource in the CLI. Posting the same response shape inline on each of the three threads you replied to, plus the new All five new tests ( Y1 ( |
|
LGTM! |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
…eWindow resources
Wires up `az aks maintenancewindow {create,show,list,update,delete,wait}`
against the existing MaintenanceWindowsOperations exposed by the vendored
azure_mgmt_preview_aks SDK (API version 2026-04-02-preview).
The MaintenanceWindow peer resource is RG-scoped and uses the same Schedule /
DailySchedule / WeeklySchedule / AbsoluteMonthlySchedule / RelativeMonthlySchedule
models that already power maintenanceConfiguration's
`aksManagedAutoUpgradeSchedule`. The construction helpers
(constructSchedule + per-type builders) are reused from maintenanceconfiguration.py
as-is; only an outer MaintenanceWindowResource wrapper is added.
LRO support: create/delete/update accept --no-wait (begin_create_or_update +
begin_delete are LRO on the SDK). `update` branches at runtime — tags-only
goes through the synchronous update_tags PATCH, schedule changes go through
the full begin_create_or_update PUT.
Requires the Microsoft.ContainerService/AKSSharedMaintenanceWindowPreview
feature to be registered on the subscription (auto-approval). Documented in
the command help.
…-PUT contract The first iteration of `aks_maintenancewindow_update` quietly refetched the existing resource on the schedule-change path to fill in `location` and preserve `tags` when omitted. That conflated tags-only PATCH with full PUT and contradicted the documented contract: - tags-only -> PATCH (sync, --no-wait ignored) - any schedule arg -> full PUT; caller owns the entire resource body Bringing the code back in line: the schedule path no longer GETs the existing resource. --location still defaults to the RG location when omitted (same fallback as create). Help text now states explicitly that PUT replaces the body and tags must be re-passed if the caller wants to keep them. Addresses Copilot review comment on PR Azure#9927 (tags-cleared edge case).
Structural change: update is now read-modify-write.
- custom.py: aks_maintenancewindow_update refetches the existing MW via
client.get(), applies user-supplied changes onto it, then PUTs.
Matches the CLI convention used by `az aks update`,
`az aks nodepool update`, and `az containerapp update` — every
multi-field-mutable resource in the CLI follows this shape.
Side effects:
* Anushka's location-immutability bug (A3) — refetch picks up
existing.location; supplying a mismatching --location is warned
and ignored instead of letting ARM 400 the PUT.
* Anushka's not_allowed_dates silent-erasure bug (A4) — refetch
preserves blackout dates the caller didn't specify.
* Future-proofs against new MW fields without per-field merge logic.
Drops the prior tags-only PATCH fast path entirely (the SDK
update_tags() call); no precedent in the CLI for that branching
shape — snapshot is the only existing CLI user of update_tags() and
that's because snapshots are immutable apart from tags.
- custom.py + commands.py: drop double-delete-confirmation (A2 / Y2).
Keep CLI-core `confirmation=True` (auto-injects --yes/-y and
respects AZURE_CORE_DISABLE_CONFIRM_PROMPT). Drop handler-side
prompt_y_n + yes kwarg. Matches `aks delete` / `aks nodepool delete`.
- custom.py + _params.py + _help.py: add --config-file support for
create and update (mirrors `aks maintenanceconfiguration add
--config-file` and closes A4 properly). On create the JSON wholly
defines the resource; on update its `properties` block replaces the
existing properties block. The only path today to set / clear
notAllowedDates (until a dedicated --not-allowed-dates flag lands).
- _validators.py + _params.py: add validate_duration_hours enforcing
the 4-24 hour range locally so users get a clear argparse error
instead of an opaque ARM 400 (Ye's Y1).
- _help.py: rewrite group long-summary per Anushka's wording (drop
"Once available" framing; customer-focused description). Rewrite
update long-summary to describe the new RMW semantics. Add
--config-file example with notAllowedDates JSON to create + update.
- maintenancewindow.py: drop hasAnyScheduleArg helper (no callers
after the RMW refactor).
- tests/latest/test_maintenancewindow.py: +18 new test cases:
* 9 RMW path tests (tags-only preserves schedule, schedule-only
preserves tags, location-mismatch warning, no-args no-op,
get-before-put assertion, etc.)
* 3 --config-file path tests on update (replace properties keep
tags/location, --tags overrides JSON tags, must-be-dict)
* 2 --config-file path tests on create (location from JSON,
fallback to RG location)
* 8 validate_duration_hours tests (bounds inclusive/exclusive,
None is ok, negative/zero raise)
All 28 tests pass locally; existing MTC scenario tests (10) still
pass with no regressions. Sibling commands' --help renders cleanly
including the new update long-summary and --config-file arg.
… move HISTORY to Pending - tests: add AksMaintenanceWindowScenarioTest covering the standalone MaintenanceWindow lifecycle end-to-end (create -> show -> update -> list -> delete -> show 404) with a recorded cassette for playback. Remove the stale orphaned test_aks_maintenancewindow.yaml recording that had no backing test method. - _help.py: fix faulty_help_example_parameters lint by using the `--utc-offset=<value>` form in the create examples (the space form parsed the leading `-` as a flag), and add the missing help entry for the `aks maintenancewindow wait` command (missing_command_help). - HISTORY.rst: move the maintenancewindow changelog entry into the Pending section (a rebase had folded it into the released 21.0.0b5 block alongside unrelated changes).
ibaboulfetouh
force-pushed
the
iaboulfetouh/aks-preview-maintenancewindow
branch
from
fc075fb to
879a175
Compare
|
@yewmsft thanks — pushed a new revision that lands your recommendation plus the rest of the outstanding feedback. Summary of what changed since Your points
Anushka's points (also addressed in the same pass)
E2E test (Fuming's ask)
Housekeeping
|
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Related command
az aks maintenancewindowSummary
Wires up
az aks maintenancewindow {create,show,list,update,delete,wait}against the existingMaintenanceWindowsOperationsexposed by the vendoredazure_mgmt_preview_aksSDK (API version2026-04-02-preview, already in21.0.0b4).MaintenanceWindowis a resource-group-scoped ARM peer resource that defines a reusable maintenance schedule. When themaintenanceWindowIdlink field becomes available onmaintenanceConfigurations(May 2026 preview API), customers will be able to link multiple managedCluster maintenance configurations to one sharedMaintenanceWindowso the schedule lives in a single place.Approach
Hand-written commands modeled after the existing
aks maintenanceconfigurationblock. The peer-resource shape is simpler than MTC's because there's no cluster context, no legacy time-in-week schedule shape, and no per-config-name validation gymnastics — just oneMaintenanceWindowResource(Location + Tags + Properties) wrapping the sameScheduleenum/types that already poweraksManagedAutoUpgradeSchedule.Reuse, not duplication:
constructSchedule,constructDailySchedule,constructWeeklySchedule,constructAbsoluteMonthlySchedule, andconstructRelativeMonthlyScheduleinazext_aks_preview/maintenanceconfiguration.pyare imported as-is — they validate cross-arg exclusivity for all four schedule types and build theSchedulemodel. The newmaintenancewindow.pyonly adds the outerMaintenanceWindowResourcewrapper. No copy-paste; one source of truth for schedule construction.LRO support:
create/delete/updateaccept--no-wait(the SDK exposesbegin_create_or_update+begin_deleteas LRO).updatebranches at runtime — tags-only goes through the synchronousupdate_tags()PATCH; any schedule arg present routes through the fullbegin_create_or_update()PUT.Feature gate: Requires
Microsoft.ContainerService/AKSSharedMaintenanceWindowPreviewto be registered on the subscription (auto-approval). Following the established convention (e.g.EnableFIPSPreviewin_help.py), this is documented in the command help, not enforced client-side — the RP returns 403 if missing.Changes
_client_factory.pycf_maintenance_windowscommands.pymaintenance_window_sdkCliCommandType +aks maintenancewindowcommand groupmaintenancewindow.pyconstructMaintenanceWindowResource,hasAnyScheduleArgcustom.pyaks_maintenancewindow_{list,show,create,update,delete}(5 handlers)_params.pyargument_contextblocks (location auto-bound via CLI core, matching nodepool-snapshot pattern)_help.pytests/latest/test_maintenancewindow.pyHISTORY.rstTest plan
python -m unittest azext_aks_preview.tests.latest.test_maintenancewindow— 11/11 pass (4 schedule happy-paths + 4 validation cases + 3hasAnyScheduleArg)python -m unittest azext_aks_preview.tests.latest.test_maintenanceconfiguration— 10/10 still pass (no regressions from the new imports incustom.py)az aks maintenancewindow --help— group lists all 6 commands with the AFEC-flag noteaz aks maintenancewindow create --help— all 7 schedule args present,--location -lauto-bound,--no-waitauto-added, examples renderaz aks maintenancewindow update --help— documents the tags-only-vs-PUT contractGeneral Guidelines
azdev style <YOUR_EXT>locally? (localazdevnot installed;py_compileclean on all 7 touched files)python scripts/ci/test_index.py -qlocally? (no setup.py version bump in this PR; the next aks-preview release will roll all pending changes together)