Conversation
|
Hi suyadav1, |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
microsoft-github-policy-service
Bot
added
Auto-Assign
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
|
AKS |
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
suyadav1
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Moderate validation, warning-handling, and test-discovery issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Azure Monitor Profile-based Container Insights onboarding and tuning controls for AKS, with managed-identity validation, DCR lifecycle handling, and expanded tests and documentation.
Changes:
- Adds syslog, Prometheus scraping, and CNL configuration.
- Updates monitoring authentication warnings and legacy compatibility behavior.
- Adds AMP provisioning, validation, cleanup, help text, and regression tests.
File summaries
| File | Summary |
|---|---|
src/aks-preview/linter_exclusions.yml |
Excludes new long CLI options from linting. |
src/aks-preview/HISTORY.rst |
Documents monitoring behavior changes. |
src/aks-preview/azext_aks_preview/tests/latest/test_validators.py |
Adds validator coverage; the direct-execution entry point must move below the new tests. |
src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py |
Adds AMP and monitoring lifecycle tests. |
src/aks-preview/azext_aks_preview/tests/latest/test_custom.py |
Tests warnings and argument registration. |
src/aks-preview/azext_aks_preview/managed_cluster_decorator.py |
Implements AMP monitoring lifecycle and settings. |
src/aks-preview/azext_aks_preview/custom.py |
Exposes new options and warning integration. |
src/aks-preview/azext_aks_preview/addonconfiguration.py |
Adds legacy authentication warnings; exact addon matching and explicit-option handling need adjustment. |
src/aks-preview/azext_aks_preview/_validators.py |
Adds Container Insights validation. |
src/aks-preview/azext_aks_preview/_params.py |
Registers parameters; scraping options need create-time validator coverage. |
src/aks-preview/azext_aks_preview/_help.py |
Updates help and examples. |
src/aks-preview/azext_aks_preview/_consts.py |
Adds AMP-related constants. |
Review details
Suppressed comments (1)
src/aks-preview/azext_aks_preview/addonconfiguration.py:76
- This substring check also matches addon names such as
monitoring-previewornotmonitoring. Because the warning is emitted before addon validation, those inputs can produce a misleading legacy-monitoring warning; split the comma-separated addon list and compare trimmed tokens exactly.
if "monitoring" not in (addons or ""):
return
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| c.argument("enable_prometheus_metrics_scraping", action="store_true", is_preview=True) | ||
| c.argument("disable_prometheus_metrics_scraping", action="store_true", is_preview=True) |
| ampls_resource_id=None, | ||
| enable_high_log_scale_mode=False, | ||
| ): | ||
| warn_on_legacy_monitoring_auth(enable_msi_auth_for_monitoring, addons) |
| unittest.main() | ||
|
|
||
|
|
||
| class ContainerInsightsSettingsNamespace(SimpleNamespace): |
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
| monitoring_profile.config.get("enableRetinaNetworkFlags", "") | ||
| ).lower() == "true" | ||
| if cnl_already_enabled: | ||
| if _is_container_network_logs_enabled_on_mc(mc, self.context.get_addon_consts()): |
There was a problem hiding this comment.
[P2] Honor a simultaneous container-network-logs disable
On a cluster with CNL mirrored into the legacy addon, az aks update -g RG -n CLUSTER --disable-container-network-logs --enable-high-log-scale-mode false now fails, although it worked before this PR. The Azure Monitor profile has been set to Disabled, but the helper falls back to the unchanged legacy enableRetinaNetworkFlags=true, so this guard still treats CNL as enabled. Please honor the requested disable rather than the stale legacy value.
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
| not self.context.raw_param.get("disable_azure_monitor_logs") | ||
| ): | ||
| self.context.set_intermediate( | ||
| "monitoring_addon_postprocessing_required", True, overwrite_exists=True |
There was a problem hiding this comment.
[P1] Preserve unspecified DCR settings
A standalone --data-collection-settings valid.json update now removes existing syslog unless --enable-syslog is repeated. Conversely, --enable-syslog resets existing high-scale/CNL streams, custom settings and the ingestion DCE. This path calls a full-replacement helper with omitted options as None. Please merge the existing DCR and change only explicitly supplied settings.
| # 'az aks enable-addons -a monitoring'. Silently re-running would otherwise recreate the | ||
| # default workspace and re-provision DCR/DCRA artifacts for a cluster that is already set | ||
| # up, which hides configuration mistakes such as a mistyped --workspace-resource-id. | ||
| if _is_monitoring_enabled_on_mc(mc, addon_consts): |
There was a problem hiding this comment.
[P1] Avoid double onboarding with the companion core update
With Azure/azure-cli#34077 at 450ebf0e installed, --enable-azure-monitor-logs on an unmonitored cluster runs setup via the extension's update_addon_profiles, then again through the new core callback. This guard rejects the second call as already enabled after DCR/DCRA provisioning, so no cluster PUT occurs. Please dispatch onboarding once across supported core versions.
🤖 PR Validation —⚠️ Review suggested
Summary
Onboards Container Insights through the first-class
azureMonitorProfile.containerInsightsAPI instead of the legacyomsagentaddon profile, and hardens--enable-azure-monitor-logs/--disable-azure-monitor-logsso they behave like the addon commands they replace.The RP keeps
azureMonitorProfile.containerInsightsand theomsagentaddon profile in sync, so both onboarding styles converge on the same agent and the same data collection rule. The gap was on the client side: the new flags skipped several guardrails thataz aks enable-addons -a monitoringhas always enforced, and mis-sequenced the DCR/DCRA provisioning.Details
Guardrails added to
--enable-azure-monitor-logsArgumentUsageErrorpointing at--disable-azure-monitor-logs, matchingenable-addons -a monitoringuseAADAuthfalse/absent)The legacy-auth check runs before the already-enabled check so the more actionable migration message wins.
--disable-azure-monitor-logsnow fully tears downPreviously it only flipped
containerInsights.enabledtofalse, leaving the data collection rule association attached and stale settings behind, so a later re-enable inherited them. It now removes the DCRA and resets the Container Insights fields — syslog port, Prometheus metrics scraping and container network logs — back to their defaults.It also asks for confirmation when OpenTelemetry logs and traces are enabled, since those depend on Container Insights and are disabled along with it.
DCR/DCRA sequencing fixes
enable-addons -a monitoring. On create they necessarily stay post-PUT: a DCRA is an extension resource nested under the cluster resource ID, and ARM rejects it withResourceNotFounduntil the cluster exists.Other changes
az aks create/az aks update:--syslog-port,--enable-prometheus-metrics-scraping,--disable-prometheus-metrics-scraping. These are valid only alongside--enable-azure-monitor-logsor on an already-onboarded cluster.--enable-msi-auth-for-monitoringis deprecated onaks enable-addons/aks addon enable/aks addon update, and warns when used to opt into shared-key auth. It is rejected outright when combined with--enable-azure-monitor-logs. Behaviour of the flag itself is unchanged.--disable-azure-monitor-metricsnow leaves the cluster unchanged and exits 0 instead of failing. It was the only confirmation prompt in the decorator raisingCLIErrorrather thanDecoratorEarlyExitException.Testing
Unit — 882 tests in
src/aks-preview/azext_aks_preview/tests/latest/pass, covering every guardrail, the reset-to-defaults path, both confirmation prompts including the decline paths, and the DCR-before-PUT ordering.Live — verified end to end on an AKS cluster:
--enable-azure-monitor-logscreates both the DCR and the DCRA, andama-logspods reach a healthy steady state.--disable-azure-monitor-logsremoves the DCRA and resets the profile fields.--enable-addons monitoringto confirm the ordering fix removed the startup delay.Static analysis —
pylint10.00/10 andflake8clean against the repository'spylintrcand.flake8.This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az aks create --enable-azure-monitor-logsaz aks update --enable-azure-monitor-logsaz aks update --disable-azure-monitor-logsaz aks update --disable-azure-monitor-metricsaz aks enable-addons -a monitoring,az aks addon enable,az aks addon update(deprecation warning for--enable-msi-auth-for-monitoringonly)General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install azdevrequired)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.