-
Notifications
You must be signed in to change notification settings - Fork 248
ci: azure-ipam patch upgrade testing #3678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces patch upgrade testing for Azure Container Networking by adding upgrade-specific pipeline parameters and stages to mirror the existing Cilium patch upgrade flow.
- Added new parameters (upgradeScenario, upgradeAzureIpam) and conditions to support upgrade testing.
- Updated multiple pipeline and load test templates to conditionally publish logs and deploy upgrade-specific jobs.
- Extended the cilium overlay load test template with upgrade logic for IPAM and CNS versions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
.pipelines/templates/log-template.yaml | Added conditional artifact publishing based on logType. |
.pipelines/cni/pipeline.yaml | Introduced new upgradeScenario parameter and upgrade job stages. |
.pipelines/cni/load-test-templates/restart-node-template.yaml | Updated logType condition to support upgrade scenarios. |
.pipelines/cni/load-test-templates/restart-cns-template.yaml | Updated logType condition to support upgrade scenarios. |
.pipelines/cni/load-test-templates/pod-deployment-template.yaml | Added conditional logType assignment for pod deployment testing. |
.pipelines/cni/cilium/cilium-overlay-load-test-template.yaml | Extended upgrade logic with conditional deployment steps for IPAM/CNS. |
@@ -4,6 +4,7 @@ parameters: | |||
cni: "" | |||
jobName: "restart_nodes" | |||
region: "" | |||
upgradeScenario: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter 'upgradeScenario' is defined as an empty string; consider using a boolean type to improve clarity and consistency with the pipeline parameters defined elsewhere.
Copilot uses AI. Check for mistakes.
@@ -5,6 +5,7 @@ parameters: | |||
nodeCount: 10 | |||
os: "" | |||
jobName: "restart_cns" | |||
upgradeScenario: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter 'upgradeScenario' is declared as an empty string, while its usage implies a boolean; switching to a boolean type will reduce ambiguity in condition comparisons.
Copilot uses AI. Check for mistakes.
@@ -6,6 +6,7 @@ parameters: | |||
nodeCount: 10 | |||
cni: "" | |||
jobName: "deploy_pods" | |||
upgradeScenario: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider declaring 'upgradeScenario' as a boolean instead of an empty string to harmonize with its boolean comparisons and enhance code readability.
Copilot uses AI. Check for mistakes.
@@ -11,37 +11,40 @@ parameters: | |||
hubbleEnabled: false | |||
dualstackVersion: "" | |||
cni: "cilium" | |||
upgradeScenario: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'upgradeScenario' parameter is initialized as an empty string, but is used in boolean checks; aligning its type as a boolean can reduce potential confusion in conditional logic.
Copilot uses AI. Check for mistakes.
@@ -11,37 +11,40 @@ parameters: | |||
hubbleEnabled: false | |||
dualstackVersion: "" | |||
cni: "cilium" | |||
upgradeScenario: "" | |||
upgradeAzureIpam: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, 'upgradeAzureIpam' is defined as an empty string yet evaluated in conditions; consider using a boolean type or clearly defined string values to better express its intended purpose.
Copilot uses AI. Check for mistakes.
Reason for Change:
First pr for patch upgrade testing in CNI release pipeline. The CNI release patch upgrade tests will follow the same flow that we use for cilium patch upgrade tests.
Create cluster > deploy current images > e2e > deploy upgrade images > e2e > delete cluster
Adding TEST_AZURE_IPAM_VERSION and TEST_CNS_VERSION and new conditions to check these vars in the upgrade scenario.
(CNS patch upgrade work still needs to be completed)
further documentation/instruction for use will be added when pipeline work is complete
Issue Fixed:
Requirements:
Notes: