Skip to content
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

Jump to bottom

WINC-607: [e2e] Skip MachineSet update in deletion test #899

Closed
wants to merge 1 commit into from
Closed

WINC-607: [e2e] Skip MachineSet update in deletion test #899

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 15 additions & 14 deletions test/e2e/delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ func testWindowsNodeDeletion(t *testing.T) {
testCtx, err := NewTestContext()
require.NoError(t, err)

// set excepted node count to zero, since all Windows Nodes should be deleted in the test
expectedNodeCount := int32(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer this initialized closer to its usage (around line 132)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True and agree. I moved it up here to facilitate a more natural flow to the MachineSets logic.

  • Fetch the MachineSets
  • Filter the MachineSets
  • Check the MachineSet with the Windows Label

Happy to move it closer, in case we need to.

// Get all the Machines created by the e2e tests
// For platform=none, the resulting collection is empty
e2eMachineSets, err := testCtx.client.Machine.MachineSets(clusterinfo.MachineAPINamespace).List(context.TODO(),
meta.ListOptions{LabelSelector: clusterinfo.MachineE2ELabel + "=true"})
require.NoError(t, err, "error listing MachineSets")
Expand All @@ -126,20 +129,18 @@ func testWindowsNodeDeletion(t *testing.T) {
break
}
}

require.NotNil(t, windowsMachineSetWithLabel, "could not find MachineSet with Windows label")

// Scale the Windows MachineSet to 0
expectedNodeCount := int32(0)
windowsMachineSetWithLabel.Spec.Replicas = &expectedNodeCount
_, err = testCtx.client.Machine.MachineSets(clusterinfo.MachineAPINamespace).Update(context.TODO(),
windowsMachineSetWithLabel, meta.UpdateOptions{})
require.NoError(t, err, "error updating Windows MachineSet")

// we are waiting 10 minutes for all windows machines to get deleted.
err = testCtx.waitForWindowsNodes(expectedNodeCount, true, false, false)
require.NoError(t, err, "Windows node deletion failed")

// skip the scale down step if there is no MachineSet with Windows label
if windowsMachineSetWithLabel != nil {
// Scale the Windows MachineSet to 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment was more meaningful where we used to set expectedNodeCount := int32(0) next line. It would be good if you can move it back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

expectedNodeCount := int32(0) does not scale Windows MachineSet to 0, testCtx.client.Machine.MachineSets(clusterinfo.MachineAPINamespace).Update(...) does.

In addition, expectedNodeCount is used outside of the if block, so moved it up to avoid duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

but we are setting it in windowsMachineSetWithLabel.Spec.Replicas = &expectedNodeCount which is used in update call.
anyways this is very minor.

windowsMachineSetWithLabel.Spec.Replicas = &expectedNodeCount
_, err = testCtx.client.Machine.MachineSets(clusterinfo.MachineAPINamespace).Update(context.TODO(),
windowsMachineSetWithLabel, meta.UpdateOptions{})
require.NoError(t, err, "error updating Windows MachineSet")

// we are waiting 10 minutes for all windows machines to get deleted.
err = testCtx.waitForWindowsNodes(expectedNodeCount, true, false, false)
require.NoError(t, err, "Windows node deletion failed")
}
t.Run("BYOH node removal", testCtx.testBYOHRemoval)

// Cleanup all the MachineSets created by us.
Expand Down