Skip to content

OCPCLOUD-2992: machine update #400

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPCLOUD-2992: machine update #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions e2e/machine_migration_capi_authoritative_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package e2e

import (
"fmt"
"time"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
mapiv1beta1 "github.com/openshift/api/machine/v1beta1"
mapiframework "github.com/openshift/cluster-api-actuator-pkg/pkg/framework"
Expand All @@ -12,6 +14,7 @@ import (

awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
)

var _ = Describe("[sig-cluster-lifecycle][OCPFeatureGate:MachineAPIMigration] Machine Migration CAPI Authoritative Tests", Ordered, func() {
Expand Down Expand Up @@ -298,4 +301,161 @@ var _ = Describe("[sig-cluster-lifecycle][OCPFeatureGate:MachineAPIMigration] Ma
})
})*/
})

var _ = Describe("Update CAPI Machine", Ordered, func() {
var capiUpdateTestName = "machine-update-capi-test"
var newMapiMachine *mapiv1beta1.Machine
var newCapiMachine *clusterv1.Machine
var previousState *MachineState
Context("with spec.authoritativeAPI: ClusterAPI (both MAPI and CAPI machine exist)", func() {
BeforeAll(func() {
newMapiMachine = createMAPIMachineWithAuthority(ctx, cl, capiUpdateTestName, mapiv1beta1.MachineAuthorityClusterAPI)
newCapiMachine = capiframework.GetMachine(cl, newMapiMachine.Name, capiframework.CAPINamespace)
verifyMachineRunning(cl, newCapiMachine)

DeferCleanup(func() {
By("Cleaning up machine resources")
cleanupMachineResources(
ctx,
cl,
[]*clusterv1.Machine{newCapiMachine},
[]*mapiv1beta1.Machine{newMapiMachine},
)
})
})

It("should add labels/annotations on CAPI machine", func() {
// Capture state before the update
previousState = captureMachineStateBeforeUpdate(cl, newMapiMachine)

// Add a small delay to ensure timestamps are distinguishable
time.Sleep(1 * time.Second)
Comment on lines +331 to +332
Copy link
Contributor

@chrischdi chrischdi Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, why are timestamps important here?

Copy link
Contributor Author

@huali9 huali9 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@chrischdi chrischdi Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@huali9 huali9 Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@chrischdi chrischdi Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI stability 100% has priority :-) 👍


Eventually(komega.Update(newCapiMachine, func() {
if newCapiMachine.Labels == nil {
newCapiMachine.Labels = make(map[string]string)
}
if newCapiMachine.Annotations == nil {
newCapiMachine.Annotations = make(map[string]string)
}
newCapiMachine.Labels["test-capi-label"] = "test-capi-label-value"
newCapiMachine.Annotations["test-capi-annotation"] = "test-capi-annotation-value"
}), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to add CAPI Machine labels/annotations")
})

It("should synchronize added labels/annotations to both metadata and spec on MAPI", func() {
Eventually(komega.Object(newMapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(
SatisfyAll(
// Check metadata
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Labels["test-capi-label"]
}, Equal("test-capi-label-value")),
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Annotations["test-capi-annotation"]
}, Equal("test-capi-annotation-value")),
// Check spec template metadata
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Spec.ObjectMeta.Labels["test-capi-label"]
}, Equal("test-capi-label-value")),
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Spec.ObjectMeta.Annotations["test-capi-annotation"]
}, Equal("test-capi-annotation-value")),
),
"Added labels should be copied to both metadata and spec on MAPI Machine",
)
})

It("should verify MAPI generation changed and synchronized time changed after syncing labels to spec", func() {
Eventually(komega.Get(newMapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to get updated MAPI Machine")

verifyMachineStateChanged(cl, newMapiMachine, previousState, mapiv1beta1.MachineAuthorityClusterAPI, "adding labels/annotations")
})

It("should modify existing labels/annotations on CAPI machine", func() {
// Capture state before the update
previousState = captureMachineStateBeforeUpdate(cl, newMapiMachine)

// Add a small delay to ensure timestamps are distinguishable
time.Sleep(1 * time.Second)

Eventually(komega.Update(newCapiMachine, func() {
newCapiMachine.Labels["test-capi-label"] = "modified-capi-label-value"
newCapiMachine.Annotations["test-capi-annotation"] = "modified-capi-annotation-value"
}), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to modify CAPI Machine labels/annotations")
})

It("should synchronize modified labels/annotations to both metadata and spec on MAPI", func() {
Eventually(komega.Object(newMapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(
SatisfyAll(
// Check metadata
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Labels["test-capi-label"]
}, Equal("modified-capi-label-value")),
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Annotations["test-capi-annotation"]
}, Equal("modified-capi-annotation-value")),
// Check spec template metadata
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Spec.ObjectMeta.Labels["test-capi-label"]
}, Equal("modified-capi-label-value")),
WithTransform(func(m *mapiv1beta1.Machine) string {
return m.Spec.ObjectMeta.Annotations["test-capi-annotation"]
}, Equal("modified-capi-annotation-value")),
),
"Modified labels/annotations should be synchronized to both metadata and spec on MAPI Machine",
)
})

It("should verify MAPI generation changed and synchronized time changed after syncing modified labels to spec", func() {
Eventually(komega.Get(newMapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to get updated MAPI Machine")

verifyMachineStateChanged(cl, newMapiMachine, previousState, mapiv1beta1.MachineAuthorityClusterAPI, "modifying labels/annotations")
})

It("should delete labels/annotations on CAPI machine", func() {
// Capture state before the update
previousState = captureMachineStateBeforeUpdate(cl, newMapiMachine)

// Add a small delay to ensure timestamps are distinguishable
time.Sleep(1 * time.Second)

Eventually(komega.Update(newCapiMachine, func() {
delete(newCapiMachine.Labels, "test-capi-label")
delete(newCapiMachine.Annotations, "test-capi-annotation")
}), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to delete CAPI Machine labels/annotations")
})

It("should synchronize label/annotation deletion to both metadata and spec on MAPI", func() {
Eventually(komega.Object(newMapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(
SatisfyAll(
// Check metadata deletion
WithTransform(func(m *mapiv1beta1.Machine) bool {
_, exists := m.Labels["test-capi-label"]
return exists
}, BeFalse()),
WithTransform(func(m *mapiv1beta1.Machine) bool {
_, exists := m.Annotations["test-capi-annotation"]
return exists
}, BeFalse()),
// Verify deleted labels are also removed from spec
WithTransform(func(m *mapiv1beta1.Machine) bool {
_, exists := m.Spec.ObjectMeta.Labels["test-capi-label"]
return exists
}, BeFalse()),
WithTransform(func(m *mapiv1beta1.Machine) bool {
_, exists := m.Spec.ObjectMeta.Annotations["test-capi-annotation"]
return exists
}, BeFalse()),
),
"Deleted labels/annotations should be synchronized to both metadata and spec on MAPI Machine",
)
})

It("should verify MAPI generation changed and synchronized time changed after syncing deleted labels to spec", func() {
Eventually(komega.Get(newMapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to get updated MAPI Machine")

verifyMachineStateChanged(cl, newMapiMachine, previousState, mapiv1beta1.MachineAuthorityClusterAPI, "deleting labels/annotations")
})
})
})
})
97 changes: 97 additions & 0 deletions e2e/machine_migration_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,3 +324,100 @@ func verifyMachineSynchronizedGeneration(cl client.Client, mapiMachine *mapiv1be
fmt.Sprintf("MAPI Machine SynchronizedGeneration should equal %s Machine Generation (%d)", authoritativeMachineType, expectedGeneration),
)
}

// MachineState holds the state of both MAPI and CAPI Machines before an update operation.
type MachineState struct {
MAPIGeneration int64
CAPIGeneration int64
SynchronizedTime *metav1.Time
}

// captureMachineStateBeforeUpdate captures the current state (generation and synchronized time) of both MAPI and CAPI Machines
// 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, currentMAPIMachine *mapiv1beta1.Machine) *MachineState {
Eventually(komega.Get(currentMAPIMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), "Failed to get MAPI Machine before capturing state")

currentCAPIMachine := capiframework.GetMachine(cl, currentMAPIMachine.Name, capiframework.CAPINamespace)

state := &MachineState{
MAPIGeneration: currentMAPIMachine.Generation,
CAPIGeneration: currentCAPIMachine.Generation,
}

// Find and store the synchronized condition's last transition time
for _, condition := range currentMAPIMachine.Status.Conditions {
if condition.Type == SynchronizedCondition {
state.SynchronizedTime = &condition.LastTransitionTime
break
}
}

return state
}

// verifyMachineStateChanged verifies that:
// 1. CAPI Machine generation remains unchanged (metadata-only changes don't bump spec generation)
// 2. MAPI Machine generation has changed (spec.objectMeta was updated)
// 3. Synchronized time has changed (indicating a new synchronization occurred)
// This is used for CAPI authoritative scenarios where CAPI metadata changes are synced to MAPI spec.
func verifyMachineStateChanged(cl client.Client, mapiMachine *mapiv1beta1.Machine, previousState *MachineState, authority mapiv1beta1.MachineAuthority, operation string) {
// First verify CAPI Machine generation remains unchanged
currentCAPIMachine := capiframework.GetMachine(cl, mapiMachine.Name, capiframework.CAPINamespace)
By(fmt.Sprintf("Verifying CAPI Machine generation unchanged after %s (previous: %d, current: %d)", operation, previousState.CAPIGeneration, currentCAPIMachine.Generation))
Expect(currentCAPIMachine.Generation).To(Equal(previousState.CAPIGeneration), "CAPI Machine generation should not change for metadata-only updates")

// Then verify MAPI Machine generation has changed
By(fmt.Sprintf("Verifying MAPI Machine generation changed after %s (previous: %d, current: %d)", operation, previousState.MAPIGeneration, mapiMachine.Generation))
Expect(mapiMachine.Generation).To(BeNumerically(">", previousState.MAPIGeneration), "MAPI Machine generation should increment when spec.objectMeta is updated")

// 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))

// Verify status remains correct
verifyMAPIMachineSynchronizedCondition(mapiMachine, authority)
verifyMachineSynchronizedGeneration(cl, mapiMachine, authority)
}

// verifyMachineStateUnchanged verifies that:
// 1. CAPI Machine generation remains unchanged
// 2. MAPI Machine generation remains unchanged
// 3. Synchronized time remains unchanged
// This is used for MAPI authoritative scenarios where MAPI metadata-only changes should not trigger any updates.
func verifyMachineStateUnchanged(cl client.Client, mapiMachine *mapiv1beta1.Machine, previousState *MachineState, authority mapiv1beta1.MachineAuthority, operation string) {
// Verify CAPI Machine generation remains unchanged
currentCAPIMachine := capiframework.GetMachine(cl, mapiMachine.Name, capiframework.CAPINamespace)
By(fmt.Sprintf("Verifying CAPI Machine generation unchanged after %s (previous: %d, current: %d)", operation, previousState.CAPIGeneration, currentCAPIMachine.Generation))
Expect(currentCAPIMachine.Generation).To(Equal(previousState.CAPIGeneration), "CAPI Machine generation should not change for metadata-only updates")

// Verify MAPI Machine generation remains unchanged
By(fmt.Sprintf("Verifying MAPI Machine generation unchanged after %s (previous: %d, current: %d)", operation, previousState.MAPIGeneration, mapiMachine.Generation))
Expect(mapiMachine.Generation).To(Equal(previousState.MAPIGeneration), "MAPI Machine generation should not change for metadata-only updates")

// Verify synchronized time remains unchanged
if previousState.SynchronizedTime != nil {
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 unchanged after %s (previous: %v, current: %v)", operation, previousState.SynchronizedTime.Time, currentSynchronizedTime.Time))
Expect(currentSynchronizedTime.Time).To(Equal(previousState.SynchronizedTime.Time), "Synchronized time should not change for metadata-only updates")
}

// Verify status remains correct
verifyMAPIMachineSynchronizedCondition(mapiMachine, authority)
verifyMachineSynchronizedGeneration(cl, mapiMachine, authority)
}
Loading