Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
microsoft-github-policy-service
Bot
requested review from
Qi Pan (Pan-Qi),
Yu Chen (jsntcy),
ZelinWang (wangzelin007) and
Yong Zhang (yonzhan)
September 14, 2026 19:18
Member
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Collaborator
|
Please fix CI issues |
Resolve conflict in acs/tests/latest/test_validators.py: both branches appended new test classes at the end of the file. Kept both sides — TestAzureMonitorLogsValidators, TestContainerInsightsSettingsValidators and TestOpenTelemetryValidators from this branch, and TestNatGatewayV2Validators from dev. The additions are independent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0090dbce-3f16-44e6-b016-b7e92f47f656
suyadav1
changed the title
Member
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in generated artifacts, CLI validation, deprecation wiring, and monitoring/OTLP lifecycle handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR onboards AKS Container Insights through the Azure Monitor profile and GA-enables OTLP receivers for metrics, logs, and traces.
Changes:
- Adds CLI options, validation, help, and release notes.
- Updates monitoring profile and DCR/DCRA lifecycle handling.
- Adds test coverage and includes unrelated diagnostic artifacts requiring removal.
File summaries
| File | Summary |
|---|---|
src/azure-cli/HISTORY.rst |
Documents new monitoring and OTLP behavior. |
src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_validators.py |
Tests new validators. |
src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_managed_cluster_decorator.py |
Tests monitoring and OTLP lifecycle behavior. |
src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py |
Tests custom monitoring behavior. |
src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_aks_commands.py |
Updates command-level monitoring tests. |
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py |
Implements profile and DCR lifecycle changes; unresolved issues affect settings-size validation, OTLP port updates, duplicate provisioning, port-only inputs, and DCR postprocessing. |
src/azure-cli/azure/cli/command_modules/acs/custom.py |
Extends AKS command handlers and deprecation behavior. |
src/azure-cli/azure/cli/command_modules/acs/addonconfiguration.py |
Updates DCR/DCRA provisioning helpers. |
src/azure-cli/azure/cli/command_modules/acs/_validators.py |
Adds monitoring and OTLP validation. |
src/azure-cli/azure/cli/command_modules/acs/_params.py |
Registers options; unresolved validator wiring, port handling, deprecation redirect, and disable-flow issues remain. |
src/azure-cli/azure/cli/command_modules/acs/_help.py |
Adds command help and examples. |
src/azure-cli/azure/cli/command_modules/acs/_consts.py |
Adds monitoring constants and defaults. |
report.20260914.015459.849235.0.001.json |
Unrelated Node.js OOM diagnostic containing workstation and environment metadata; remove it. |
report.20260913.222148.802059.0.001.json |
Unrelated Node.js OOM diagnostic containing workstation and environment metadata; remove it. |
report.20260913.220253.791060.0.001.json |
Unrelated Node.js OOM diagnostic containing workstation and environment metadata; remove it. |
linter_exclusions.yml |
Excludes long new option names. |
Review details
Suppressed comments (10)
src/azure-cli/azure/cli/command_modules/acs/_params.py:923
- The aggregate OTLP validator is only attached to
enable_opentelemetry_metrics. A logs/traces-only update therefore skipsvalidate_opentelemetry_ports, allowing duplicate receiver ports that the validator is intended to reject. Wire the same update validator to the logs/traces enable flag.
action='store_true',
src/azure-cli/azure/cli/command_modules/acs/_params.py:1055
- This is the
aks enable-addonsargument context, but that command does not register--enable-azure-monitor-logs; only create and update do. The current deprecation redirect therefore tells users to retry an unrecognized option. Redirect to the fullaz aks update --enable-azure-monitor-logscommand, or add the replacement option to this command.
"disable_gateway_api",
src/azure-cli/azure/cli/command_modules/acs/_params.py:545
- The port arguments have no validator, and the create decorator only reads them inside the corresponding
get_enable_opentelemetry_*()branches. Thereforeaz aks create --enable-azure-monitor-logs --opentelemetry-metrics-port-http 4318is accepted but creates no metrics receiver; the getter's intended “port without enable” error is never reached. Validate port-only input or invoke the setup when a port is supplied.
c.argument(
src/azure-cli/azure/cli/command_modules/acs/_params.py:550
- The logs/traces port arguments are also only consumed when
--enable-opentelemetry-logs-tracesis set, but the port options themselves are not validated. A create command that supplies only--opentelemetry-logs-traces-port-httpis accepted and silently drops the requested value instead of producing the getter's documented validation error. Reject standalone ports or apply the configuration when they are supplied.
c.argument(
src/azure-cli/azure/cli/command_modules/acs/_params.py:913
- These flags have no update-time validator. Combining them with
--disable-azure-monitor-logsis therefore accepted:_disable_azure_monitor_logsresets Container Insights, thenupdate_azure_monitor_logs_settingsreapplies the supplied syslog/scraping values, so the disable operation leaves non-default settings and contradicts the validator/help contract. Attach the update validator before the command runs.
action='store_true',
help=(
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:10760
- The context port getter explicitly permits changing metrics ports on update when the receiver is already enabled, but this branch calls the setup method only when the enable flag is present.
az aks update --opentelemetry-metrics-port-http/--grpcon an enabled receiver is consequently accepted and ignored.
def update_azure_monitor_profile(self, mc: ManagedCluster) -> ManagedCluster:
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:9933
- When
--enable-azure-monitor-logsis combined with container network logs or high log scale mode, this intermediate is set beforeupdate_azure_monitor_logsprovisions the DCR/DCRA.check_is_postprocessing_required()then invokes postprocessing, wherecreate_dcr=Trueprovisions the same artifacts again after the PUT. This duplicates DCR/DCRA operations for fresh AMP onboarding and can race with the pre-PUT provisioning; distinguish fresh pre-provisioning from existing-monitoring updates or clear/consume this intermediate after the pre-PUT path.
)
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:8591
- Port arguments are only consumed inside
_setup_opentelemetry_metrics, but this call is gated solely by the enable flag. A create command that supplies--opentelemetry-metrics-port-http/--grpcwithout--enable-opentelemetry-metricstherefore succeeds and silently ignores the ports instead of invoking the existing getter validation and reporting the invalid combination.
otel_logs_cls = self.models.ManagedClusterAzureMonitorProfileAppMonitoringOpenTelemetryLogsAndTraces
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:8595
- The logs/traces port arguments have the same silent no-op path on create: without the enable flag,
_setup_opentelemetry_logs_tracesis never called, so invalid or standalone ports are neither rejected nor written. Include port presence in this decision so the existing getter validation runs.
if logs_port_http is not None:
otlp_logs_config.http_port = logs_port_http
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:11113
- This independent settings path only writes
containerInsights.syslog_portand Prometheus scraping; it never schedules the DCR update needed for--enable-syslog,--data-collection-settings, or--ampls-resource-id. Those values are consumed by the DCR helper at line 11937 onward, but update postprocessing is gated bymonitoring_addon_postprocessing_required, which this path does not set, so the documentedaz aks update --enable-syslog --syslog-port ...leaves syslog uncollected (and AMP-only clusters skip it entirely). Mark monitoring postprocessing as required for these DCR-affecting options, or provision/update the DCR directly here.
is_private_cluster=False,
ampls_resource_id=None,
enable_high_log_scale_mode=False,
)
- Files reviewed: 16/16 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2
to
+8
| { | ||
| "header": { | ||
| "reportVersion": 5, | ||
| "event": "Allocation failed - JavaScript heap out of memory", | ||
| "trigger": "OOMError", | ||
| "filename": "report.20260913.220253.791060.0.001.json", | ||
| "dumpEventTime": "2026-09-13T22:02:53Z", |
Member
There was a problem hiding this comment.
suyadav1 please take a look at Copilot's cleanup request above; all three generated crash reports are still included in the current revision.
| "LESSCLOSE": "/usr/bin/lesspipe %s %s", | ||
| "LESSOPEN": "| /usr/bin/lesspipe %s", | ||
| "LOGNAME": "sky", | ||
| "LOG_ANALYTICS_WORKSPACE_RESOURCE_ID": "/subscriptions/26ad903f-2330-429d-8389-864ac35c4350/resourcegroups/defaultresourcegroup-eus2/providers/microsoft.operationalinsights/workspaces/defaultworkspace-26ad903f-2330-429d-8389-864ac35c4350-eus2", |
| "LESSCLOSE": "/usr/bin/lesspipe %s %s", | ||
| "LESSOPEN": "| /usr/bin/lesspipe %s", | ||
| "LOGNAME": "sky", | ||
| "LOG_ANALYTICS_WORKSPACE_RESOURCE_ID": "/subscriptions/26ad903f-2330-429d-8389-864ac35c4350/resourcegroups/defaultresourcegroup-eus2/providers/microsoft.operationalinsights/workspaces/defaultworkspace-26ad903f-2330-429d-8389-864ac35c4350-eus2", |
Comment on lines
+536
to
+537
| c.argument('enable_prometheus_metrics_scraping', action='store_true') | ||
| c.argument('disable_prometheus_metrics_scraping', action='store_true') |
| c.argument('disable_opentelemetry_metrics', action='store_true') | ||
| c.argument('opentelemetry_metrics_port_http', type=int) | ||
| c.argument('opentelemetry_metrics_port_grpc', type=int) | ||
| c.argument('enable_opentelemetry_logs_traces', action='store_true') |
| data_collection_settings = self.context.get_data_collection_settings() | ||
| # Oversized settings are dropped rather than sent, to avoid the DCR call failing with | ||
| # "Request Header Fields Too Large". | ||
| if data_collection_settings and len(str(data_collection_settings)) > 10000: |
suyadav1
changed the title
Author
|
/azp run |
|
Commenter does not have sufficient privileges for PR 34077 in repo Azure/azure-cli |
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
FumingZhang
reviewed
Sep 17, 2026
| # update azure monitor metrics profile | ||
| mc = self.update_azure_monitor_profile(mc) | ||
| # update azure monitor logs (container insights) enablement | ||
| mc = self.update_azure_monitor_logs(mc) |
Member
There was a problem hiding this comment.
[P1] Confirm both disables before deleting monitoring resources
On a cluster with managed metrics and OTLP logs/traces enabled, combining --disable-azure-monitor-metrics --disable-azure-monitor-logs cleans up metrics resources before asking about logs (after accepting the metrics prompt, if present). Declining the logs prompt then skips the cluster PUT, leaving metrics enabled but without its collection resources. Please collect both confirmations before either cleanup.
| self.models.ManagedClusterAzureMonitorProfileAppMonitoringAutoInstrumentation(enabled=False) | ||
| ) | ||
|
|
||
| if self.context.get_enable_opentelemetry_metrics(): |
Member
There was a problem hiding this comment.
[P2] Apply OTLP port-only updates
On an already-enabled receiver, az aks update -g RG -n CLUSTER --opentelemetry-metrics-port-http 5500 passes validation but leaves the old port unchanged because the setter is gated on the enable flag. The same applies to all four OTLP port options. Please apply explicitly supplied ports to the existing receiver even when no enable flag is passed.
| if syslog_port is None and disable_prometheus_scraping is None: | ||
| return mc | ||
|
|
||
| _apply_container_insights_settings( |
Member
There was a problem hiding this comment.
[P2] Reconfigure the DCR when enabling syslog on an existing cluster
For a monitoring-enabled cluster whose DCR does not collect syslog, az aks update -g RG -n CLUSTER --enable-syslog --syslog-port 28330 only writes the listening port. It never marks monitoring postprocessing as required, so the DCR is unchanged and syslog ingestion does not start. Please trigger DCR reconfiguration when collection settings are supplied.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 PR Validation — ❌ Action needed
❌AzureCLI-BreakingChangeTest
Please submit your Breaking Change Pre-announcement ASAP if you haven't already. Please note:
Related command
az aks create,az aks update,az aks enable-addonsDescription
Onboards Container Insights through the Azure Monitor profile (
azureMonitorProfile.containerInsights) instead of the legacyomsagentaddon, and GAs the OpenTelemetry (OTLP) receivers for metrics, logs and traces.New options on
az aks create/az aks update:--enable-azure-monitor-logs/--disable-azure-monitor-logs— onboard/offboard Container Insights, always with managed identity authentication.--enable-msi-auth-for-monitoringis deprecated in favour of it.--enable-opentelemetry-metrics/--enable-opentelemetry-logs-traces(plus the matching--disable-*and--*-port-http/--*-port-grpcoptions) — configure the OTLP receivers.--syslog-port,--enable-prometheus-metrics-scraping/--disable-prometheus-metrics-scraping— tune the Container Insights configuration. Container network logs also move onto the Azure Monitor profile.az aks updategains the workspace/DCR options it previously lacked:--workspace-resource-id,--data-collection-settings,--ampls-resource-id,--enable-syslog.Guard rails and fixes:
--enable-azure-monitor-logsis rejected on service principal clusters (the Azure Monitor profile is managed identity only), on clusters already onboarded with legacy shared key authentication, and when Azure Monitor logs is already enabled — matchingaz aks enable-addons -a monitoring.--disable-azure-monitor-logsnow removes the data collection rule association and resets the Container Insights settings to their defaults, so a later re-enable no longer silently inherits stale values. The workspace id is deliberately left on the profile: blanking it makes ARM reject every subsequent cluster write withLinkedInvalidPropertyId.az aks update, and creates them before the cluster PUT so the agent never starts without a data collection rule attached.--yesis passed.Testing Guide
Unit tests:
azdev test acsHistory Notes
[ACS]
az aks create,az aks update: Add--enable-azure-monitor-logsto onboard Container Insights through the Azure Monitor profile using managed identity authentication[ACS]
az aks update: Add--disable-azure-monitor-logsto offboard Container Insights[ACS]
az aks create,az aks update: Add--syslog-port,--enable-prometheus-metrics-scrapingand--disable-prometheus-metrics-scrapingto tune the Azure Monitor Container Insights configuration[ACS]
az aks create,az aks update: Add--enable-opentelemetry-metrics,--disable-opentelemetry-metrics,--opentelemetry-metrics-port-httpand--opentelemetry-metrics-port-grpcfor the OpenTelemetry metrics receiver[ACS]
az aks create,az aks update: Add--enable-opentelemetry-logs-traces,--disable-opentelemetry-logs-traces,--opentelemetry-logs-traces-port-httpand--opentelemetry-logs-traces-port-grpcfor the OpenTelemetry logs and traces receiver[ACS]
az aks create,az aks update,az aks enable-addons: Deprecate--enable-msi-auth-for-monitoringin favor of--enable-azure-monitor-logsThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.