Conversation
xgugeng
requested review from
alyssa1303,
anson627,
sumanthreddy29,
vittoriasalim,
wonderyl and
xinWeiWei24
as code owners
xgugeng
commented
Jan 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the cloud_info collection by adding container registry metadata, specifically targeting Azure Container Registry (ACR) for future image pull benchmarking. The changes thread a new registry parameter through the pipeline architecture and update Azure's cloud info collection to gather detailed registry properties.
Key changes include:
- Adding registry parameter throughout the pipeline template hierarchy
- Implementing ACR metadata collection in Azure cloud info script
- Updating Python modules and tests to handle the registry parameter
- Making registry configuration dynamic in deployment templates
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| jobs/competitive-test.yml | Adds registry parameter with empty string default and threads it through execute and publish steps |
| steps/execute-tests.yml | Adds registry parameter (missing default value) and passes it to topology templates |
| steps/publish-results.yml | Adds registry parameter with empty string default and passes it to cloud info collection |
| steps/topology/cri-resource-consume/execute-clusterloader2.yml | Adds registry parameter and passes it to engine execution |
| steps/topology/cri-resource-consume/collect-clusterloader2.yml | Adds registry parameter and passes it to engine collection |
| steps/engine/clusterloader2/cri/execute.yml | Adds registry parameter and includes it in clusterloader2 script arguments |
| steps/engine/clusterloader2/cri/collect.yml | Adds registry parameter and passes it to cloud-specific collection |
| steps/cloud/azure/collect-cloud-info.yml | Implements ACR metadata collection using Azure CLI queries |
| steps/cloud/gcp/collect-cloud-info.yml | Adds registry parameter placeholder (no implementation) |
| steps/cloud/aws/collect-cloud-info.yml | Adds registry parameter placeholder (no implementation) |
| modules/python/clusterloader2/cri/cri.py | Adds registry argument to override_config_clusterloader2 function and writes to config |
| modules/python/tests/test_cri.py | Updates test cases to include registry parameter in function calls |
| modules/python/clusterloader2/cri/config/deployment_template.yaml | Replaces hardcoded registry name with dynamic Registry parameter |
| modules/python/clusterloader2/cri/config/config.yaml | Adds registry parameter with akscritelescope as default |
| pipelines/system/new-pipeline-test.yml | Converts template placeholders to actual test configuration with registry values |
modules/python/clusterloader2/cri/config/deployment_template.yaml
Outdated
Show resolved
Hide resolved
modules/python/clusterloader2/cri/config/deployment_template.yaml
Outdated
Show resolved
Hide resolved
modules/python/clusterloader2/cri/config/deployment_template.yaml
Outdated
Show resolved
Hide resolved
modules/python/clusterloader2/cri/config/deployment_template.yaml
Outdated
Show resolved
Hide resolved
sajayantony
reviewed
Jan 6, 2026
vittoriasalim
requested changes
Jan 8, 2026
#989) This pull request updates the way the GKE cluster name is generated in the Terraform configuration to ensure it fits within GCP's naming constraints and is more predictable. Resource naming improvement: * Changed the `name` attribute of the `google_container_cluster.gke` resource to use `trimright(substr(..., 0, 40), "-")` instead of a regex-based approach, ensuring the cluster name is always at most 40 characters and does not end with a hyphen.…tructure
This pull request makes a targeted change to the logic for creating the `azurerm_role_assignment.network_contributor` resource in the AKS CLI Terraform module. The update ensures that the role assignment is only created when both a managed identity name and a subnet ID are provided, preventing potential errors during deployment. **Resource creation logic improvement:** * Updated the `count` expression for the `azurerm_role_assignment.network_contributor` resource in `main.tf` to require both `managed_identity_name` and `aks_subnet_id` to be non-null before creating the resource.
This pull request makes a small update to the job configuration by adding support for specifying a custom job template path. This change allows for more flexibility in job submissions by letting users override the default job template. * Added a `--job_template_path` parameter to the job controller step in `steps/engine/clusterloader2/job_controller/collect.yml`, defaulting to `base/job_template.yaml` if not specified.
cl2 benchmark failure due to pod startup latency failure --------- Co-authored-by: vittoria salim <vslaim@microsoft.com>
vittoriasalim
approved these changes
Jan 9, 2026
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
5 participants
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.
This pull request enhances
cloud_infoby adding container registry information. It introduces a newregistryparameter to the relevant pipeline templates and steps and updates Azure cloud info collection to gather detailed registry metadata. This change enables future image pull benchmarking for Azure Container Registry (ACR) and other container registries across the market.For simplicity, the target ACR and test AKS must reside within the same subscription. If not, the exception handling will take care of it and return a null value.
The properties collected for the registry are as follows:
Pipeline run: ADO link