MCO-1152: MCO-1146: Add e2e tests for NodeDisruptionPolicy #4365

djoshy · 2024-05-14T19:58:02Z

This PR adds:

some e2e tests for NodeDisruptionPolicy which landed in MCO-1009: MCO-1008: MCO-905: Implement NodeDisruptionPolicy API #4267
add a generation check before the MCD consumes NodeDisruptionPolicy Status
the MCD will now degrade on NodeDisruptionPolicy errors

openshift-ci-robot · 2024-05-14T19:58:05Z

@djoshy: This pull request references MCO-1152 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.16.0" version, but no target version was set.

Details

In response to this:

- What I did

- How to verify it

- Description for the changelog

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 · 2024-05-14T19:58:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

djoshy · 2024-05-14T20:00:43Z

/test e2e-gcp-op-techpreview

djoshy · 2024-05-15T15:02:21Z

/test e2e-gcp-op-techpreview

djoshy · 2024-05-15T15:18:36Z

/test e2e-gcp-op-techpreview

openshift-ci-robot · 2024-05-15T17:30:53Z

@djoshy: This pull request references MCO-1152 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.16.0" version, but no target version was set.

Details

In response to this:

This PR:

adds some e2e tests for NodeDisruptionPolicy which landed in MCO-1009: MCO-1008: MCO-905: Implement NodeDisruptionPolicy API #4267

an API bump, which lets us use the Generation of the MachineConfiguration before consuming it

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-robot · 2024-05-15T17:31:06Z

@djoshy: This pull request references MCO-1152 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.16.0" version, but no target version was set.

Details

In response to this:

This PR adds:

some e2e tests for NodeDisruptionPolicy which landed in MCO-1009: MCO-1008: MCO-905: Implement NodeDisruptionPolicy API #4267

an API bump, which lets us use the Generation of the MachineConfiguration before consuming it

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-robot · 2024-05-20T18:05:34Z

@djoshy: This pull request references MCO-1152 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.17.0" version, but no target version was set.

Details

In response to this:

This PR adds:

some e2e tests for NodeDisruptionPolicy which landed in MCO-1009: MCO-1008: MCO-905: Implement NodeDisruptionPolicy API #4267

add a generation check before the MCD consumes a NodeDisruptionPolicy

the MCD will now degrade on NodeDisruptionPolicy errors

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-robot · 2024-05-20T18:09:37Z

@djoshy: This pull request references MCO-1152 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.17.0" version, but no target version was set.

Details

In response to this:

This PR adds:

some e2e tests for NodeDisruptionPolicy which landed in MCO-1009: MCO-1008: MCO-905: Implement NodeDisruptionPolicy API #4267

add a generation check before the MCD consumes NodeDisruptionPolicy Status

the MCD will now degrade on NodeDisruptionPolicy errors

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.

djoshy · 2024-05-21T13:47:35Z

/retest-required

djoshy · 2024-05-21T18:30:31Z

/test unit

djoshy · 2024-05-23T12:55:35Z

/retest-required

djoshy · 2024-05-23T15:16:05Z

/test e2e-gcp-op-techpreview

djoshy · 2024-05-30T13:30:13Z

/test all

cheesesashimi

Overall this looks great! I just have a few small suggestions.

cheesesashimi · 2024-05-30T14:33:43Z

test/e2e-shared-tests/mcd_config_drift.go

 	require.Nil(t, err)

-	assertLogsContain(t, cs, mcdPod, &node, logEntry)
+	helpers.AssertMCDLogsContain(t, cs, mcdPod, &node, logEntry)


praise: Nice!

cheesesashimi · 2024-05-30T14:42:11Z

test/e2e-techpreview/nodedisrupt_test.go

+		var ac *mcoac.NodeDisruptionPolicySpecFileApplyConfiguration
+		fileName := "/etc/test-" + string(action.Type)
+		if action.Type == opv1.ReloadSpecAction {
+			ac = mcoac.NodeDisruptionPolicySpecFile().WithPath(fileName).WithActions(mcoac.NodeDisruptionPolicySpecAction().WithType(action.Type).WithReload(mcoac.ReloadService().WithServiceName(action.Reload.ServiceName)))


suggestion (non-blocking): For better readability, split this up a bit:

reload := mcoac.ReloadService().WithServiceName(action.Reload.ServiceName) actions := mcoac.NodeDisruptionPolicySpecAction().WithType(action.Type).WithReload(reload) ac = mcoac.NodeDisruptionPolicySpecFile().WithPath(fileName).WithActions(actions)

If this API won't let you do that or you get bizarre errors by doing that, you can break this across multiple lines like this instead:

ac = mcoac.NodeDisruptionPolicySpecFile().WithPath(fileName).WithActions( mcoac.NodeDisruptionPolicySpecAction().WithType(action.Type).WithReload( mcoac.ReloadService().WithServiceName(action.Reload.ServiceName)))

Will attempt to clean this up, thanks (:

I was able to break this down as suggested, and create a new helper - making it overall a lot cleaner and re-usable. Thanks so much for this suggestion!

test/e2e-techpreview/nodedisrupt_test.go

test/helpers/utils.go

cheesesashimi · 2024-05-30T14:48:02Z

test/helpers/utils.go

+	}
+}
+
+func GetFunctionName(i interface{}) string {


praise: Cool!

cheesesashimi · 2024-05-30T14:51:03Z

pkg/daemon/update.go

+		// Ensure status.ObservedGeneration matches the last generation of MachineConfiguration
+		if mcop.Generation != mcop.Status.ObservedGeneration {
+			klog.Errorf("calculating NodeDisruptionPolicies: NodeDisruptionPolicyStatus is not up to date.")
+			err = fmt.Errorf("NodeDisruptionPolicyStatus is not up to date")


~~suggestion: Should this error be returned? If not, add a comment explaining why.~~

thought: I just took a look at why this was written like this. Basically, we use the variable err for both the value returned by wait.PollUntilContextTimeout() in addition to what is inside the closure that wait.PollUntilContextTimeout() executes. Its a bit confusing as to whether we should stop polling whenever we encounter an error or keep going and deal with it error later. If we want to keep going and deal with the error later, using the Aggregate type coupled with an appropriate deduplication function could help readability.

(To be clear: I am not asking you to change this as part of this PR. It's just a thought I'm putting here for posterity.)

Sorry about the structure being confusing! My thought process to use it was the following:

If we encounter an error, we don't want to keep going in the polling function. We want to try again after an interval of time.

If we timeout on the polling function, we want the last error we encountered to be reported. Hence the use of the same variable inside and outside of the function.

I hope that clears up why I went with the last error vs aggregation. Any of the errors within the polling loop are fatal, with the earlier ones being slightly more fatal than the ones following.

Ahhh that makes sense, thanks for the clarification!

I assigned a new variable so it is easier to read what I'm trying to do (:

cheesesashimi · 2024-05-30T14:57:15Z

test/e2e-techpreview/nodedisrupt_test.go

+		{Type: opv1.RestartSpecAction, Restart: &opv1.RestartService{ServiceName: "crio.service"}},
+		{Type: opv1.ReloadSpecAction, Reload: &opv1.ReloadService{ServiceName: "crio.service"}}}
+
+	// Shuffle the three action sets so each testFunc is randomly assigned one of the above action sets.


praise: Nice! I wish we could run our e2e suite with -shuffle=on. I think we have e2e tests that need to execute in a certain order, but I'm unsure how that flag affects subtests like this.

cheesesashimi · 2024-05-30T15:06:35Z

test/e2e-techpreview/nodedisrupt_test.go

+		t.Run(helpers.GetFunctionName(testFunc), func(t *testing.T) {
+			// Only parallelize if there are enough nodes to run the tests individually
+			if len(nodes) >= len(testFuncs) {
+				t.Parallel()


praise: Nice! I want to be able to parallelize more in our e2e suite.

djoshy · 2024-06-03T13:51:18Z

/retest-required

djoshy · 2024-06-03T14:18:15Z

/retest-required

djoshy · 2024-06-03T17:51:31Z

/retest-required

djoshy · 2024-06-04T13:04:48Z

/retest-required

sinnykumari

changes looks good.
/approve
Will see if Zack have any final thoughts and would like to tag lgtm.

djoshy · 2024-06-06T13:14:33Z

/test all

cheesesashimi · 2024-06-07T18:34:48Z

/lgtm
/approve

openshift-ci · 2024-06-07T18:36:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheesesashimi, djoshy, sinnykumari

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cheesesashimi,djoshy,sinnykumari]

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

djoshy · 2024-06-07T18:45:08Z

/hold

just in case QE wants to take a look

djoshy · 2024-06-07T18:45:18Z

/test all

openshift-ci · 2024-06-07T22:27:33Z

@djoshy: The following test 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/e2e-vsphere-ovn-upi	`23a8c8d`	link	false	`/test e2e-vsphere-ovn-upi`

Full PR test history. Your PR dashboard.

Details

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.

djoshy · 2024-06-10T17:40:10Z

Discussed with @sergiordlr , as this is mainly e2es, pre merge QE is not required. Unholding.

/unhold
/retest-required

openshift-ci-robot · 2024-06-10T18:34:32Z

/retest-required

Remaining retests: 0 against base HEAD 3506e3e and 2 for PR HEAD 23a8c8d in total

openshift-bot · 2024-06-10T23:35:11Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-machine-config-operator-container-v4.17.0-202406102247.p0.ge7a0f9a.assembly.stream.el9 for distgit ose-machine-config-operator.
All builds following this will include this PR.

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

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 May 14, 2024

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

djoshy force-pushed the node-disrupt-e2e branch from 193a193 to bc86228 Compare May 15, 2024 15:02

djoshy force-pushed the node-disrupt-e2e branch from bc86228 to d7f367a Compare May 15, 2024 15:16

djoshy force-pushed the node-disrupt-e2e branch from d7f367a to a804831 Compare May 15, 2024 17:24

djoshy marked this pull request as ready for review May 15, 2024 17:24

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 May 15, 2024

openshift-ci bot requested review from cdoern and cheesesashimi May 15, 2024 17:26

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

djoshy force-pushed the node-disrupt-e2e branch from a804831 to 16a6d4f Compare May 20, 2024 13:21

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

djoshy force-pushed the node-disrupt-e2e branch from 16a6d4f to aa5388d Compare May 20, 2024 13:32

djoshy changed the title ~~MCO-1152: Add e2e tests for NodeDisruptionPolicy~~ MCO-1152: MCO-1146: Add e2e tests for NodeDisruptionPolicy May 20, 2024

djoshy force-pushed the node-disrupt-e2e branch from 53b5a3b to dc99777 Compare May 21, 2024 16:37

add check for MachineConfiguration generation

b90b11e

djoshy force-pushed the node-disrupt-e2e branch from dc99777 to 2cf9a1c Compare May 22, 2024 18:04

cheesesashimi suggested changes May 30, 2024

View reviewed changes

djoshy force-pushed the node-disrupt-e2e branch from 2cf9a1c to 33b1a8f Compare May 31, 2024 16:09

djoshy force-pushed the node-disrupt-e2e branch 2 times, most recently from 313c89b to 2f6cc09 Compare June 4, 2024 20:41

djoshy added 2 commits June 4, 2024 16:42

degrade daemon on NodeDisruptionPolicy errors

b65c633

add NodeDisruption e2e tests

23a8c8d

djoshy force-pushed the node-disrupt-e2e branch from 2f6cc09 to 23a8c8d Compare June 4, 2024 20:42

hexfusion mentioned this pull request Jun 5, 2024

OCPBUGS-33248: daemon: upgrade os image from local container storage if it exists #4347

Merged

sinnykumari reviewed Jun 6, 2024

View reviewed changes

openshift-ci bot assigned cheesesashimi Jun 7, 2024

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

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 7, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2024

openshift-merge-bot bot merged commit e7a0f9a into openshift:master Jun 10, 2024

djoshy deleted the node-disrupt-e2e branch June 27, 2024 13:22

MCO-1152: MCO-1146: Add e2e tests for NodeDisruptionPolicy #4365

MCO-1152: MCO-1146: Add e2e tests for NodeDisruptionPolicy #4365

Uh oh!

Conversation

djoshy commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 14, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented May 14, 2024

Uh oh!

djoshy commented May 14, 2024

Uh oh!

djoshy commented May 15, 2024

Uh oh!

djoshy commented May 15, 2024

Uh oh!

openshift-ci-robot commented May 15, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 15, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 20, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 20, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djoshy commented May 21, 2024

Uh oh!

djoshy commented May 21, 2024

Uh oh!

djoshy commented May 23, 2024

Uh oh!

djoshy commented May 23, 2024

Uh oh!

djoshy commented May 30, 2024

Uh oh!

cheesesashimi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djoshy May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheesesashimi May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djoshy commented Jun 3, 2024

Uh oh!

djoshy commented Jun 3, 2024

Uh oh!

djoshy commented Jun 3, 2024

Uh oh!

djoshy commented May 14, 2024 •

edited

Loading

openshift-ci-robot commented May 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 15, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 15, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 20, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 20, 2024 •

edited by openshift-ci bot

Loading

djoshy May 31, 2024 •

edited

Loading

cheesesashimi May 30, 2024 •

edited

Loading

openshift-ci bot commented Jun 7, 2024 •

edited

Loading