CNTRLPLANE-3515: test(e2e): add day-2 label/taint no-rollout verification#8595
CNTRLPLANE-3515: test(e2e): add day-2 label/taint no-rollout verification#8595mgencur wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request references CNTRLPLANE-3515 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates the nodepool_day2_tags e2e test: it adds the k8s sets import, captures the MachineDeployment.Generation before applying changes, applies EC2 resource tag updates plus node label and taint changes, polls NodePool status while tracking updating-related condition types (using a set and a rollingUpdateTriggered flag), asserts rollingUpdateTriggered is false, and re-fetches the MachineDeployment to verify its Generation is unchanged. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8595 +/- ##
==========================================
+ Coverage 37.44% 40.68% +3.24%
==========================================
Files 751 755 +4
Lines 91969 93363 +1394
==========================================
+ Hits 34435 37985 +3550
+ Misses 54894 52645 -2249
- Partials 2640 2733 +93 see 79 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/nodepool_day2_tags_test.go`:
- Around line 79-88: The test currently replaces NodePool.Spec.NodeLabels and
NodePool.Spec.Taints outright; instead modify the test to merge the day-2 values
into existing fields: ensure NodePool.Spec.NodeLabels is non-nil (create if nil)
and set the "e2e.day2.validation" key to "true" without dropping other keys, and
for NodePool.Spec.Taints append the new hyperv1.Taint (Key:"e2e-day2",
Value:"test", Effect:corev1.TaintEffectPreferNoSchedule) only if an equivalent
taint (same Key, Value, Effect) does not already exist, so existing taints are
preserved rather than overwritten.
- Around line 153-154: The MachineDeployment generation assertion is hardcoded
to 1; instead capture the pre-update generation (e.g., store md.Generation in a
variable like initialGeneration or preUpdateGen before performing the day-2
tag/label/taint updates) and then assert that md.Generation is still equal to
that stored value after the update; update the test in
test/e2e/nodepool_day2_tags_test.go to use the stored pre-update generation for
the final assertion against md.Generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9ed09c00-4205-40e5-b9d2-1c5734b44f66
📒 Files selected for processing (1)
test/e2e/nodepool_day2_tags_test.go
mgencur
force-pushed
the
node_labels_and_taints
branch
from
9ce5563 to
4f8b5f2
Compare
Extend NodePoolDay2TagsTest to patch nodeLabels and taints day-2 and verify no rolling upgrade is triggered via NodePool conditions and MachineDeployment generation. Ref: CNTRLPLANE-3515 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mgencur
force-pushed
the
node_labels_and_taints
branch
from
4f8b5f2 to
b5d3554
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mgencur, muraee The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/retest |
|
@mgencur: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
Now I have all the evidence I need. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Konflux pipeline failed in the Root CauseThe root cause is a transient image pull failure in the Konflux CI infrastructure, completely unrelated to the PR changes:
Recommendations
Evidence
|
5 participants
What this PR does / why we need it:
Extend NodePoolDay2TagsTest to patch nodeLabels and taints day-2 and verify no rolling upgrade is triggered via NodePool conditions and MachineDeployment generation.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3515
Special notes for your reviewer:
The original test also scaled up the NodePool and verified that the new nodes actually have the labels/taints. But I think it's not necessary to test it. The existing NodePool upgrade tests already set labels/taints at the beginning and verify these are properly applied to nodes after initial start. These tests together with the new one from this PR cover the whole original test
Checklist:
Summary by CodeRabbit