OCPCLOUD-2646: MachineSetSync controller: tests #230

theobarberbany · 2024-11-05T13:16:35Z

This PR adds tests outside of the happy path for the MachineSetSync controller going in the direction from capi -> mapi

#221 should merge before this.

pkg/controllers/machinesetsync/machineset_sync_controller.go

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

JoelSpeed · 2024-11-07T11:18:56Z

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

+			// I think there may be another case here, where we have had it previously set - synchronized and then it gets unset.
+			// We may want to ensure that we don't change resourceVersion, and that the condition does not change?
+			// Changes to false with a failure reason?
+			//
+			// I'm not sure if we have an API validation that stops this.
+			// TODO: clarify / work out


I believe we have validation to say that it cannot be cleared once populated, but lets check that

I don't think we have a validation currently, here's where I've looked:

https://github.com/openshift/api/blob/master/machine/v1beta1/types_machineset.go#L158-L167

looks like we've got one that forces moving through Migrating on the status.

https://github.com/openshift/api/blob/master/machine/v1beta1/types_machineset.go#L64-L77

on the spec we say it must be one of CAPI or MAPI.

I'm not sure I see anything stopping a particularly determined user from manually editing the status? Or am I missing something

Perhaps we should add a validation to make sure the field cannot be removed once set 🤔

So adding something like:

// +kubebuilder:validation:XValidation:rule="self != '' || oldSelf == ''",message="Once authoritativeAPI is set, it cannot be unset."

to the API?

You're very close to what we would need. But... if someone completely removes a field, then the validations do not run. So you need a validation similar to this on a higher level, nearest, absolutely required field, so that even if the field itself is attempted to be removed, the validation will still execute

I see so setting this at the top of MachineSetStatus
and:

// +kubebuilder:validation:XValidation:rule="self.authoritativeAPI != '' || oldSelf.authoritativeAPI == ''",message="The authoritativeAPI field cannot be unset once it has been set."

?

And if this is the right approach, will this also pass validation in case of creation without authoritativeAPI set?

Yes, something like that should work. Since it includes oldSelf, it's considered a transition rule, and therefore doesn't execute on create. So a status without a valid value on create will be accepted still, which is what we need

See: openshift/api#2091

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

JoelSpeed · 2024-11-07T11:19:55Z

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

+			// Do we have another case here where we consistently want to not see a MAPI MachineSet created?
+			// Not sure if this is overkill
+


I'd say consistently no new MAPI MachineSets seems reasonable yes, in the future we may decide to change that behaviour so we should test it, as we have decided it for now

most of a test for this in WIP commit.

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

openshift-ci-robot · 2024-11-07T15:11:00Z

@theobarberbany: This pull request references OCPCLOUD-2646 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.18.0" version, but no target version was set.

In response to this:

This PR adds tests outside of the happy path for the MachineSetSync controller going in the direction from capi -> mapi

#221 should merge before 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.

JoelSpeed · 2024-11-08T12:37:16Z

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

+			// It("should not create a MAPI MachineSet", func() {
+			// 	Consistently(k.ObjectList(&machinev1beta1.MachineSetList{}), timeout).ShouldNot(
+			// 		ContainElement(WithTransform(
+			// 			func(ms machinev1beta1.MachineSet) string {
+			// 				return ms.GetName()
+			// 			}, Equal(capiMachineSet.GetName()))))
+			// })


I think something along the lines of

Suggested change

// It("should not create a MAPI MachineSet", func() {

// Consistently(k.ObjectList(&machinev1beta1.MachineSetList{}), timeout).ShouldNot(

// ContainElement(WithTransform(

// func(ms machinev1beta1.MachineSet) string {

// return ms.GetName()

// }, Equal(capiMachineSet.GetName()))))

// })

It("should not create a MAPI MachineSet", func() {

Consistently(k.ObjectList(&machinev1beta1.MachineSetList{}), timeout).ShouldNot(

ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachineSet.GetName()))),

),

)

})

Ended up with:

It("should not create a MAPI MachineSet", func() { Consistently(k.ObjectList(&machinev1beta1.MachineSetList{}), timeout).ShouldNot(HaveField("Items", ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachineSet.GetName()))), )) })

damdo · 2024-11-08T16:43:05Z

@JoelSpeed I pushed, in a new commit, the changes we agreed on and slightly reorged the contexts to deduplicate some setups like the MachineSet creation. LMK if they look ok and I'll cleanup the history and get ready for labelling.

Co-authored-by: Damiano Donati <[email protected]>

JoelSpeed · 2024-11-08T18:28:39Z

/approve
/lgtm

openshift-ci · 2024-11-08T18:29:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-11-08T20:28:02Z

/retest-required

Remaining retests: 0 against base HEAD 3953a2a and 2 for PR HEAD 393a17c in total

damdo · 2024-11-08T21:50:53Z

/skip

openshift-ci · 2024-11-09T00:45:27Z

@theobarberbany: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/regression-clusterinfra-cucushift-rehearse-capi-aws-ipi	`393a17c`	link	false	`/test regression-clusterinfra-cucushift-rehearse-capi-aws-ipi`
ci/prow/e2e-azure-ovn-techpreview	`393a17c`	link	false	`/test e2e-azure-ovn-techpreview`

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.

openshift-bot · 2024-11-09T02:10:48Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-capi-operator
This PR has been included in build ose-cluster-capi-operator-container-v4.18.0-202411090139.p0.gec9120b.assembly.stream.el9.
All builds following this will include this PR.

theobarberbany marked this pull request as draft November 5, 2024 13:16

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2024

openshift-ci bot requested review from elmiko and nrb November 5, 2024 13:17

theobarberbany force-pushed the ms-capi2mapi-test branch 4 times, most recently from 2d774b6 to 6e83d24 Compare November 6, 2024 17:05

JoelSpeed reviewed Nov 7, 2024

View reviewed changes

theobarberbany force-pushed the ms-capi2mapi-test branch from 6e83d24 to e42627c Compare November 7, 2024 12:15

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024

theobarberbany force-pushed the ms-capi2mapi-test branch from e42627c to 80fef1a Compare November 7, 2024 13:17

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024

theobarberbany changed the title ~~MachineSetSync controller: tests~~ OCPCLOUD-2646: MachineSetSync controller: tests Nov 7, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 7, 2024

theobarberbany force-pushed the ms-capi2mapi-test branch 5 times, most recently from 92de073 to 08e22ce Compare November 7, 2024 18:27

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024

Update capi actuator pkg testutils

d41d246

theobarberbany force-pushed the ms-capi2mapi-test branch from 08e22ce to 27d80e3 Compare November 7, 2024 21:07

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024

theobarberbany force-pushed the ms-capi2mapi-test branch 2 times, most recently from e2f7b4f to 0cc991e Compare November 7, 2024 22:21

theobarberbany marked this pull request as ready for review November 7, 2024 22:33

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2024

openshift-ci bot requested a review from damdo November 7, 2024 22:34

openshift-ci bot requested a review from RadekManak November 7, 2024 22:34

theobarberbany force-pushed the ms-capi2mapi-test branch from ad9ca6f to aaba899 Compare November 7, 2024 22:34

JoelSpeed reviewed Nov 8, 2024

View reviewed changes

Updates tests for MachineSetSync controller

393a17c

Co-authored-by: Damiano Donati <[email protected]>

damdo force-pushed the ms-capi2mapi-test branch from 9c517ae to 393a17c Compare November 8, 2024 18:07

openshift-ci bot assigned JoelSpeed Nov 8, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2024

openshift-merge-bot bot merged commit ec9120b into openshift:main Nov 9, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPCLOUD-2646: MachineSetSync controller: tests #230

OCPCLOUD-2646: MachineSetSync controller: tests #230

theobarberbany commented Nov 5, 2024 •

edited

Loading

JoelSpeed Nov 7, 2024

theobarberbany Nov 7, 2024

JoelSpeed Nov 8, 2024

damdo Nov 8, 2024

JoelSpeed Nov 8, 2024

damdo Nov 8, 2024 •

edited

Loading

JoelSpeed Nov 8, 2024

damdo Nov 8, 2024

JoelSpeed Nov 7, 2024

theobarberbany Nov 7, 2024

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

JoelSpeed Nov 8, 2024

damdo Nov 8, 2024

damdo commented Nov 8, 2024

JoelSpeed commented Nov 8, 2024

openshift-ci bot commented Nov 8, 2024

openshift-ci-robot commented Nov 8, 2024

damdo commented Nov 8, 2024

openshift-ci bot commented Nov 9, 2024

openshift-bot commented Nov 9, 2024

		// Do we have another case here where we consistently want to not see a MAPI MachineSet created?
		// Not sure if this is overkill

OCPCLOUD-2646: MachineSetSync controller: tests #230

OCPCLOUD-2646: MachineSetSync controller: tests #230

Conversation

theobarberbany commented Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damdo Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 7, 2024 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damdo commented Nov 8, 2024

JoelSpeed commented Nov 8, 2024

openshift-ci bot commented Nov 8, 2024

openshift-ci-robot commented Nov 8, 2024

damdo commented Nov 8, 2024

openshift-ci bot commented Nov 9, 2024

openshift-bot commented Nov 9, 2024

theobarberbany commented Nov 5, 2024 •

edited

Loading

damdo Nov 8, 2024 •

edited

Loading

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading