OCPCLOUD-2992: machine update #400
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Machine state helpers e2e/machine_migration_helpers.go |
Adds MachineState struct and helpers: captureMachineStateBeforeUpdate, verifyMachineStateChanged, and verifyMachineStateUnchanged to snapshot and assert MAPI/CAPI generation and synchronized-time transitions. |
MAPI update tests (authoritative) e2e/machine_migration_mapi_authoritative_test.go |
Adds an "Update MAPI Machine" test suite exercising add/modify/delete of labels and annotations, synchronization checks to CAPI, uses MachineState helpers, adds time, Gomega, and komega imports. The suite appears duplicated within the file. |
CAPI update tests (authoritative) e2e/machine_migration_capi_authoritative_test.go |
Adds an "Update CAPI Machine" test suite exercising add/modify/delete of labels and annotations on CAPI and verifying sync to MAPI; adds time, Gomega, and komega imports. The suite appears duplicated within the file. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Test as E2E Test
participant Helper as MachineState Helper
participant MAPI as MAPI API
participant CAPI as CAPI API
rect rgba(200,220,255,0.12)
Test->>Helper: captureMachineStateBeforeUpdate(machine)
Helper->>MAPI: read machine (generation, status)
Helper->>CAPI: read mirror machine (generation, syncedTime)
Helper-->>Test: previous MachineState
end
rect rgba(200,255,200,0.12)
Test->>MAPI: apply update (labels/annotations)
alt authoritative=MAPI
MAPI-->>Test: update ACK
else authoritative=CAPI
Test->>CAPI: apply update (labels/annotations)
CAPI-->>Test: update ACK
end
end
rect rgba(255,240,200,0.12)
Test->>Helper: wait & poll for synchronization
Helper->>CAPI: poll mirror for expected metadata
Helper->>MAPI: poll for expected metadata
Helper-->>Test: observed synced state
end
rect rgba(230,210,255,0.08)
Test->>Helper: verifyMachineStateChanged/Unchanged(prevState, authority, op)
Helper->>MAPI: fetch post-op status
Helper->>CAPI: fetch post-op status
Helper-->>Test: verdict (generations/time changed or unchanged)
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Check duplicated test blocks in both
e2e/machine_migration_mapi_authoritative_test.goande2e/machine_migration_capi_authoritative_test.go(intent vs accidental). - Review generation/time comparison and nil-handling in
MachineStatehelpers. - Verify polling/timing choices to reduce flakiness and ensure sufficient time gaps for timestamp comparisons.
- Confirm consistent use of Gomega/komega imports and project test conventions.
Poem
🐇 I tally generations, hop through time and tags,
I nudge a label, I nibble at flags.
CAPI and MAPI twirl in my sight,
I stamp their sync in dawn's soft light. ✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title 'OCPCLOUD-2992: machine update' refers to the main change (machine update tests) but is extremely vague and generic. It uses non-descriptive language that doesn't convey meaningful information about what machine update tests were added or their scope. | Make the title more specific and descriptive. For example: 'Add e2e tests for machine label/annotation updates and synchronization' or 'Add CAPI/MAPI machine update and state verification tests'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
e2e/machine_migration_capi_authoritative_test.go(3 hunks)e2e/machine_migration_helpers.go(1 hunks)e2e/machine_migration_mapi_authoritative_test.go(2 hunks)
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 golangci-lint (2.5.0)
Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/machine_migration_helpers.go (3)
328-333: Prefer value + presence flag over pointer for SynchronizedTimeUsing a pointer here (and later taking an address of a range copy) is fragile. Store the time value and a boolean presence flag instead; it’s simpler and avoids accidental aliasing.
type MachineState struct { MAPIGeneration int64 CAPIGeneration int64 - SynchronizedTime *metav1.Time + SynchronizedTime metav1.Time + HasSynchronizedTime bool }Follow-ups (outside this hunk):
- In captureMachineStateBeforeUpdate: set both SynchronizedTime and HasSynchronizedTime when the condition is found.
- In verify helpers: gate temporal assertions on HasSynchronizedTime and use SynchronizedTime directly.
339-356: Don’t take the address of a for-range copy; capture the value insteadThis loop takes the address of condition.LastTransitionTime where condition is a copy. Prefer copying the value into state (or use the boolean flag from the previous comment).
- for _, condition := range currentMAPIMachine.Status.Conditions { + for _, condition := range currentMAPIMachine.Status.Conditions { if condition.Type == SynchronizedCondition { - state.SynchronizedTime = &condition.LastTransitionTime + // value copy; no pointer to range variable + state.SynchronizedTime = condition.LastTransitionTime + state.HasSynchronizedTime = true break } }
408-419: DRY the “find synchronized time” scan into a tiny helperBoth verify functions repeat the same loop. Extract a small helper to read the synchronized condition’s LastTransitionTime and reuse it. Keeps tests terse and less error-prone.
// outside: helpers.go func getSynchronizedLastTransitionTime(m *mapiv1beta1.Machine) (*metav1.Time, bool) { for _, c := range m.Status.Conditions { if c.Type == SynchronizedCondition { t := c.LastTransitionTime return &t, true } } return nil, false }Then replace the loops with getSynchronizedLastTransitionTime(mapiMachine).
e2e/machine_migration_capi_authoritative_test.go (1)
325-341: Stabilize capture: wait for Synchronized condition before recording previous stateIf the synchronized condition isn’t set yet, previousState.SynchronizedTime may be nil and downstream assertions may skip/flake. Ensure it’s True before capture.
It("should add labels/annotations on CAPI machine", func() { - // Capture state before the update - previousState = captureMachineStateBeforeUpdate(cl, newMapiMachine.Name) + // Ensure sync condition is present, then capture state + verifyMAPIMachineSynchronizedCondition(newMapiMachine, mapiv1beta1.MachineAuthorityClusterAPI) + previousState = captureMachineStateBeforeUpdate(cl, newMapiMachine.Name) Eventually(func() error {Please run this suite once locally to confirm reduced flakes in the first “Update CAPI Machine” block.
e2e/machine_migration_mapi_authoritative_test.go (1)
229-248: Optional: add the same precondition before capture for consistencyAlign with the CAPI suite by waiting for the Synchronized condition before capturing; reduces any timing window variability.
It("should add labels/annotations on MAPI machine", func() { - // Capture state before the update + // Optional: ensure sync condition is present, then capture state + verifyMAPIMachineSynchronizedCondition(newMapiMachine, mapiv1beta1.MachineAuthorityMachineAPI) previousState = captureMachineStateBeforeUpdate(cl, newMapiMachine.Name)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
e2e/machine_migration_capi_authoritative_test.go(2 hunks)e2e/machine_migration_helpers.go(1 hunks)e2e/machine_migration_mapi_authoritative_test.go(2 hunks)
openshift-ci-robot
added
the
jira/valid-reference
|
@huali9: This pull request references OCPCLOUD-2992 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 story to target the "4.21.0" version, but no target version was set. In 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. |
|
@huali9 can we use Komega for Eventually? |
huali9
force-pushed
the
machineupdate2
branch
from
a4dd8b0 to
36d3beb
Compare
Sure, updated, PTAL again @sunzhaohua2 |
|
@huali9: This pull request references OCPCLOUD-2992 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 story to target the "4.21.0" version, but no target version was set. In 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. |
|
/assign @nrb @theobarberbany @chrischdi |
e2e/machine_migration_helpers.go
Outdated
| // before an update operation. The captured state is later used by verifyMachineStateChanged or verifyMachineStateUnchanged | ||
| // to verify whether the state has changed or remained the same after the operation. | ||
| func captureMachineStateBeforeUpdate(cl client.Client, machineName string) *MachineState { | ||
| currentMAPIMachine, err := mapiframework.GetMachine(cl, machineName) |
There was a problem hiding this comment.
Should this be wrapped in an eventually?
| //It didn't sync to CAPI for deleting labels/annotations | ||
| //https://issues.redhat.com/browse/OCPBUGS-61596 |
There was a problem hiding this comment.
| //It didn't sync to CAPI for deleting labels/annotations | |
| //https://issues.redhat.com/browse/OCPBUGS-61596 | |
| // It didn't sync to CAPI for deleting labels/annotations | |
| // https://issues.redhat.com/browse/OCPBUGS-61596 |
|
/lgtm Only a nit |
|
Scheduling tests matching the |
huali9
force-pushed
the
machineupdate2
branch
from
36d3beb to
5e0d75b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@huali9: This pull request references OCPCLOUD-2992 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 story to target the "4.21.0" version, but no target version was set. In 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/machine_migration_helpers.go (1)
379-390: Nil-deref risk remains unaddressed.This issue was previously flagged: if
previousState.SynchronizedTimeis nil (condition wasn't present when captured), lines 388-389 will panic. Note thatverifyMachineStateUnchangedat line 412 correctly guards against this with an explicit nil check.Apply this diff to guard against nil:
// Verify synchronized time is updated var currentSynchronizedTime *metav1.Time for _, condition := range mapiMachine.Status.Conditions { if condition.Type == SynchronizedCondition { currentSynchronizedTime = &condition.LastTransitionTime break } } Expect(currentSynchronizedTime).ToNot(BeNil(), "Synchronized condition should exist") - By(fmt.Sprintf("Verifying synchronized time changed after %s (previous: %v, current: %v)", operation, previousState.SynchronizedTime.Time, currentSynchronizedTime.Time)) - Expect(currentSynchronizedTime.Time).To(BeTemporally(">", previousState.SynchronizedTime.Time), fmt.Sprintf("Synchronized time should change when syncing %s metadata to spec", operation)) + if previousState.SynchronizedTime != nil { + By(fmt.Sprintf("Verifying synchronized time changed after %s (previous: %v, current: %v)", operation, previousState.SynchronizedTime.Time, currentSynchronizedTime.Time)) + Expect(currentSynchronizedTime.Time).To(BeTemporally(">", previousState.SynchronizedTime.Time), fmt.Sprintf("Synchronized time should change when syncing %s metadata to spec", operation)) + } else { + By(fmt.Sprintf("Previous synchronized time missing; asserting current time is set after %s", operation)) + Expect(currentSynchronizedTime.Time.IsZero()).To(BeFalse(), "Synchronized time should be set after synchronization") + }
🧹 Nitpick comments (1)
e2e/machine_migration_capi_authoritative_test.go (1)
327-344: Consider eliminating explicit sleep delays.Lines 332, 379, and 420 use
time.Sleep(1 * time.Second)to ensure timestamps are distinguishable. While this works, it adds latency to test execution. Consider whether theEventuallyassertions that follow are sufficient to observe the time progression, or if the sleep is truly necessary for timestamp resolution on the test infrastructure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
e2e/machine_migration_capi_authoritative_test.go(3 hunks)e2e/machine_migration_helpers.go(1 hunks)e2e/machine_migration_mapi_authoritative_test.go(2 hunks)
🔇 Additional comments (8)
e2e/machine_migration_helpers.go (3)
328-333: LGTM! Clean state tracking structure.The
MachineStatestruct appropriately captures the generation metadata and synchronized time needed for pre/post-update validation.
335-362: LGTM! Reliable state capture logic.The function correctly captures both MAPI and CAPI generations and safely handles the case where the SynchronizedTime condition may not exist.
396-428: LGTM! Proper nil handling.This function correctly guards against nil
previousState.SynchronizedTimeat line 412 before accessing it, demonstrating the proper pattern that should be applied toverifyMachineStateChanged.e2e/machine_migration_capi_authoritative_test.go (2)
346-372: LGTM! Thorough synchronization verification.The tests properly verify that CAPI metadata changes are synchronized to both MAPI metadata and spec, and correctly validate generation bumps and time progression.
374-459: LGTM! Comprehensive update operation coverage.The test suite thoroughly covers the complete lifecycle of labels/annotations (add, modify, delete) and consistently verifies synchronization behavior across all scenarios.
e2e/machine_migration_mapi_authoritative_test.go (3)
231-268: LGTM! Proper MAPI authoritative behavior verification.The test correctly verifies that metadata-only changes on MAPI machines synchronize to CAPI but don't trigger generation bumps or synchronized time changes when MAPI is authoritative. The same
time.Sleepconsideration from the CAPI tests applies here (lines 236, 275, 308).
270-301: LGTM! Consistent modification test.The modify operation test correctly follows the same pattern as the add test, verifying synchronization while ensuring no unintended state changes occur.
303-339: LGTM! Proper handling of known issue.The deletion test appropriately uses
PItfor the sync verification that's blocked by OCPBUGS-61596, while still verifying that the state remains unchanged as expected. The issue reference at lines 316-317 clearly documents why the test is pending.
|
/test e2e-aws-capi-techpreview |
huali9
force-pushed
the
machineupdate2
branch
from
5e0d75b to
c7ff9b1
Compare
|
@huali9: This pull request references OCPCLOUD-2992 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 story to target the "4.21.0" version, but no target version was set. In 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. |
| // Add a small delay to ensure timestamps are distinguishable | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
I'm curious, why are timestamps important here?
There was a problem hiding this comment.
Thank you @chrischdi for your review. I am writing the e2e following the "Update MAPI Machine" section of the e2e doc , and I think the "Synchronized conditions" mentioned there refers to checking the lastTransitionTime of Synchronized conditions, but I am not sure if my understanding is correct.
There was a problem hiding this comment.
No strong opinion: I'd ensure this is part of a unit test, that the func we use updates the lastTransitionTime correctly, and I'd only sanity check here without the sleep.
But ok if others like the idea to have the sleep and explicit check here in e2e.
There was a problem hiding this comment.
Good point about unit test! The reason I added the sleep here in the e2e test is specifically due to the second-level precision of our timestamps. When updates happen very quickly (within the same second), the lastTransitionTime doesn't change, which was causing CI failures in our previous test runs. The sleep ensures we cross the second boundary so we can actually observe the timestamp difference.
Thanks for being open to it! I'll keep the sleep to ensure CI stability. Let's see if others have thoughts - happy either way.
There was a problem hiding this comment.
CI stability 100% has priority :-) 👍
|
/test okd-scos-images |
|
/test e2e-aws-capi-techpreview |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest-required |
|
/retest |
|
/test all |
|
/test e2e-aws-ovn |
|
/test e2e-azure-ovn-techpreview |
|
/test e2e-openstack-ovn-techpreview |
|
@huali9: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@damdo Could you please help to approve this pr? Thanks! |
|
/assign @damdo |
7 participants
machine update e2e, I tested in my local and works @sunzhaohua2 @miyadav @damdo @theobarberbany PTAL, thanks!
Summary by CodeRabbit