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.

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

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: djoshy

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~~ [djoshy]

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

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.

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.

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.

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.

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

@@ -388,7 +388,7 @@ func assertNodeAndMCPIsDegraded(t *testing.T, cs *framework.ClientSet, node core
 	mcdPod, err := helpers.MCDForNode(cs, &node)
 	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!

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

openshift-ci · 2024-06-03T18:08:03Z

@djoshy: 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/e2e-azure-ovn-upgrade-out-of-change	`33b1a8f`	link	false	`/test e2e-azure-ovn-upgrade-out-of-change`
ci/prow/e2e-hypershift	`33b1a8f`	link	true	`/test e2e-hypershift`

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-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 added 2 commits May 31, 2024 11:05

degrade daemon on NodeDisruptionPolicy errors

2250bff

add NodeDisruption e2e tests

33b1a8f

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

djoshy commented May 14, 2024 •

edited

openshift-ci-robot commented May 14, 2024 •

edited by openshift-ci bot

openshift-ci bot commented May 14, 2024

openshift-ci bot commented May 14, 2024

djoshy commented May 14, 2024

djoshy commented May 15, 2024

djoshy commented May 15, 2024

openshift-ci-robot commented May 15, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 15, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 20, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 20, 2024 •

edited by openshift-ci bot

djoshy commented May 21, 2024

djoshy commented May 21, 2024

djoshy commented May 23, 2024

djoshy commented May 23, 2024

djoshy commented May 30, 2024

cheesesashimi left a comment

cheesesashimi May 30, 2024

cheesesashimi May 30, 2024

djoshy May 30, 2024

djoshy May 31, 2024 •

edited

cheesesashimi May 30, 2024

cheesesashimi May 30, 2024

cheesesashimi May 30, 2024 •

edited

djoshy May 30, 2024

cheesesashimi Jun 3, 2024

cheesesashimi May 30, 2024

cheesesashimi May 30, 2024

djoshy commented Jun 3, 2024

djoshy commented Jun 3, 2024

djoshy commented Jun 3, 2024

openshift-ci bot commented Jun 3, 2024

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

Are you sure you want to change the base?

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

Conversation

djoshy commented May 14, 2024 • edited

openshift-ci-robot commented May 14, 2024 • edited by openshift-ci bot

openshift-ci bot commented May 14, 2024

openshift-ci bot commented May 14, 2024

djoshy commented May 14, 2024

djoshy commented May 15, 2024

djoshy commented May 15, 2024

openshift-ci-robot commented May 15, 2024 • edited by openshift-ci bot

openshift-ci-robot commented May 15, 2024 • edited by openshift-ci bot

openshift-ci-robot commented May 20, 2024 • edited by openshift-ci bot

openshift-ci-robot commented May 20, 2024 • edited by openshift-ci bot

djoshy commented May 21, 2024

djoshy commented May 21, 2024

djoshy commented May 23, 2024

djoshy commented May 23, 2024

djoshy commented May 30, 2024

cheesesashimi left a 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

djoshy May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheesesashimi May 30, 2024 • edited

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

djoshy commented Jun 3, 2024

djoshy commented Jun 3, 2024

djoshy commented Jun 3, 2024

openshift-ci bot commented Jun 3, 2024

djoshy commented May 14, 2024 •

edited

openshift-ci-robot commented May 14, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 15, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 15, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 20, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 20, 2024 •

edited by openshift-ci bot

djoshy May 31, 2024 •

edited

cheesesashimi May 30, 2024 •

edited