OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes" #1774

saifshaikh48 · 2023-09-08T16:57:57Z

This PR re-orders a deconfiguration step removing files and HNS networks
to be after the instance has finished rebooting. This way WMCO is guaranteed
to only interact with the instance when it is reachable via SSH.

Previously, we were hitting timing issues where, after WICD cleanup is ran,
WMCO's configmap and node controllers would race, resulting in WMCO issuing
SSH commands while the node is still rebooting. These would fail and cause
deconfiguration to reconcile again, leading to timeouts in CI for our deletion suite.

openshift-ci-robot · 2023-09-08T16:58:02Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-18554, which is invalid:

expected the bug to target the "4.15.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR re-orders a deconfiguration step removing files and HNS networks
to be after the instance has finished rebooting. This way WMCO is guaranteed
to only interact with the instance when it is reachable via SSH.

Previously, we were hitting timing issues where, after WICD cleanup is ran,
WMCO's configmap and node controllers would race, resulting in WMCO issuing
SSH commands while the node is still rebooting. These would fail and cause
deconfiguration to reconcile again, leading to timeouts in CI for our deletion suite.

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/test-infra repository.

openshift-ci · 2023-09-08T16:58:08Z

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

saifshaikh48 · 2023-09-08T16:58:47Z

/hold

blocked by #1770

saifshaikh48 · 2023-09-08T16:59:04Z

/test vsphere-proxy-e2e-operator

saifshaikh48 · 2023-09-08T20:17:23Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2023-09-08T20:17:24Z

@saifshaikh48: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.14

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/test-infra repository.

mtnbikenc · 2023-09-28T14:46:27Z

/jira refresh

openshift-ci-robot · 2023-09-28T14:46:34Z

@mtnbikenc: This pull request references Jira Issue OCPBUGS-18554, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

In response to this:

/jira refresh

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/test-infra repository.

saifshaikh48 · 2023-10-04T16:39:24Z

/unhold

#1770 merged

openshift-ci-robot · 2023-10-04T16:39:32Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-18554, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

In response to this:

This PR re-orders a deconfiguration step removing files and HNS networks
to be after the instance has finished rebooting. This way WMCO is guaranteed
to only interact with the instance when it is reachable via SSH.

Previously, we were hitting timing issues where, after WICD cleanup is ran,
WMCO's configmap and node controllers would race, resulting in WMCO issuing
SSH commands while the node is still rebooting. These would fail and cause
deconfiguration to reconcile again, leading to timeouts in CI for our deletion suite.

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/test-infra repository.

saifshaikh48 · 2023-10-04T17:28:48Z

/test vsphere-proxy-e2e-operator
/test aws-upgrade-e2e

sebsoto · 2023-10-04T17:27:53Z

pkg/windows/windows.go

-	// Deconfigure removes all files and networks created by WMCO and runs the WICD cleanup command.
-	Deconfigure(string, string) error
+	// Deconfigure removes all files and networks created by WMCO
+	Deconfigure() error


This isn't really Deconfigure at this point. Its no longer undoing changes done by ConfigureWICD
It may be more apt to name it RemoveFilesAndNetworks

I agree with this given that we no longer have Windows.Configure(). In some ways you are doing the opposite of windows.Bootstrap().

sebsoto · 2023-10-04T17:32:05Z

pkg/nodeconfig/nodeconfig.go

+	if err := nc.Windows.RunWICDCleanup(nc.wmcoNamespace, wicdKC); err != nil {
+		return fmt.Errorf("unable to cleanup the Windows instance: %w", err)
+	}
+
+	// Wait for reboot annotation removal. This prevents deleting the node until the node no longer needs reboot.
+	if err := metadata.WaitForRebootAnnotationRemoval(context.TODO(), nc.client, nc.node.Name); err != nil {
+		return err
+	}
+	if err := nc.Windows.Deconfigure(); err != nil {
 		return fmt.Errorf("error deconfiguring instance: %w", err)
 	}


Theres an ordering requirement thats not clear here.
If its solely between RunWICDCleanup and WaitForRebootAnnotationRemoval it would be best to move them to a separate documented function.

At this point even nc.generateWICDKubeconfig() needs to move into the separate documented function.

Makes sense, thanks for pointing this out

aravindhp · 2023-10-04T19:27:35Z

pkg/windows/windows.go

-	// Deconfigure removes all files and networks created by WMCO and runs the WICD cleanup command.
-	Deconfigure(string, string) error
+	// Deconfigure removes all files and networks created by WMCO
+	Deconfigure() error


I agree with this given that we no longer have Windows.Configure(). In some ways you are doing the opposite of windows.Bootstrap().

aravindhp · 2023-10-04T19:28:41Z

pkg/nodeconfig/nodeconfig.go

+	if err := nc.Windows.RunWICDCleanup(nc.wmcoNamespace, wicdKC); err != nil {
+		return fmt.Errorf("unable to cleanup the Windows instance: %w", err)
+	}
+
+	// Wait for reboot annotation removal. This prevents deleting the node until the node no longer needs reboot.
+	if err := metadata.WaitForRebootAnnotationRemoval(context.TODO(), nc.client, nc.node.Name); err != nil {
+		return err
+	}
+	if err := nc.Windows.Deconfigure(); err != nil {
 		return fmt.Errorf("error deconfiguring instance: %w", err)
 	}


At this point even nc.generateWICDKubeconfig() needs to move into the separate documented function.

This commit re-orders a deconfiguration step to remove files and HNS networks after the instance has finished rebooting. This way WMCO is guaranteed to only interact with the instance when it is reachable via SSH. Previously, we were hitting timing issues where, after WICD cleanup is ran, WMCO's configmap and node controllers would race, resulting in WMCO issuing SSH commands while the node is still rebooting, which would fail. With this change, a log message was moved to better signal the start of node deconfiguration.

openshift-ci · 2023-10-05T17:57:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, saifshaikh48

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

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 · 2023-10-05T17:57:16Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-18554, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR re-orders a deconfiguration step removing files and HNS networks
to be after the instance has finished rebooting. This way WMCO is guaranteed
to only interact with the instance when it is reachable via SSH.

Previously, we were hitting timing issues where, after WICD cleanup is ran,
WMCO's configmap and node controllers would race, resulting in WMCO issuing
SSH commands while the node is still rebooting. These would fail and cause
deconfiguration to reconcile again, leading to timeouts in CI for our deletion suite.

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/test-infra repository.

alinaryan

Does this need any testing? Otherwise lgtm

saifshaikh48 · 2023-10-05T18:43:06Z

@alinaryan the testing I did was

making sure the proxy job passes e2e
running the deletion tests a bunch of times to make sure the BYOH removal flake wasn't seen anymore

saifshaikh48 · 2023-10-05T18:46:18Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2023-10-05T18:46:19Z

@saifshaikh48: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.14

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/test-infra repository.

alinaryan · 2023-10-05T19:20:05Z

/lgtm

sebsoto · 2023-10-06T15:04:23Z

/override ci/prow/vsphere-e2e-upgrade

openshift-ci · 2023-10-06T15:07:12Z

@sebsoto: Overrode contexts on behalf of sebsoto: ci/prow/vsphere-e2e-upgrade

In response to this:

/override ci/prow/vsphere-e2e-upgrade

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/test-infra repository.

openshift-ci · 2023-10-06T15:07:17Z

@saifshaikh48: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-10-06T15:09:55Z

@saifshaikh48: Jira Issue OCPBUGS-18554: All pull requests linked via external trackers have merged:

openshift/windows-machine-config-operator#1774

Jira Issue OCPBUGS-18554 has been moved to the MODIFIED state.

In response to this:

This PR re-orders a deconfiguration step removing files and HNS networks
to be after the instance has finished rebooting. This way WMCO is guaranteed
to only interact with the instance when it is reachable via SSH.

Previously, we were hitting timing issues where, after WICD cleanup is ran,
WMCO's configmap and node controllers would race, resulting in WMCO issuing
SSH commands while the node is still rebooting. These would fail and cause
deconfiguration to reconcile again, leading to timeouts in CI for our deletion suite.

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/test-infra repository.

openshift-cherrypick-robot · 2023-10-06T15:10:45Z

@saifshaikh48: new pull request created: #1874

In response to this:

/cherry-pick release-4.14

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/test-infra repository.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 8, 2023

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 Sep 8, 2023

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

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 28, 2023

openshift-ci bot requested a review from rrasouli September 28, 2023 14:46

saifshaikh48 mentioned this pull request Oct 4, 2023

[release-4.14] OCPBUGS-20067: Fix "WMCO does not wait for instance to reboot properly" #1862

Merged

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

saifshaikh48 force-pushed the fix-proxy-byoh-deletion-flake branch from 242be9a to be323d0 Compare October 4, 2023 17:10

saifshaikh48 changed the title ~~[WIP] OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes"~~ OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes" Oct 4, 2023

saifshaikh48 force-pushed the fix-proxy-byoh-deletion-flake branch 2 times, most recently from b5bc8f2 to adfd87b Compare October 4, 2023 17:27

sebsoto requested changes Oct 4, 2023

View reviewed changes

aravindhp suggested changes Oct 4, 2023

View reviewed changes

saifshaikh48 force-pushed the fix-proxy-byoh-deletion-flake branch from adfd87b to 445c14c Compare October 5, 2023 16:05

aravindhp approved these changes Oct 5, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2023

alinaryan reviewed Oct 5, 2023

View reviewed changes

openshift-ci bot assigned alinaryan Oct 5, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2023

saifshaikh48 marked this pull request as ready for review October 5, 2023 22:18

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 Oct 5, 2023

openshift-ci bot requested review from aravindhp and sebsoto October 5, 2023 22:19

openshift-ci bot merged commit 50132d3 into openshift:master Oct 6, 2023
16 checks passed

openshift-cherrypick-robot mentioned this pull request Oct 6, 2023

[release-4.14] OCPBUGS-20191: Fix "error removing HNS network when cleaning up BYOH proxy nodes" #1874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes" #1774

OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes" #1774

saifshaikh48 commented Sep 8, 2023

openshift-ci-robot commented Sep 8, 2023

openshift-ci bot commented Sep 8, 2023

saifshaikh48 commented Sep 8, 2023

saifshaikh48 commented Sep 8, 2023

saifshaikh48 commented Sep 8, 2023

openshift-cherrypick-robot commented Sep 8, 2023

mtnbikenc commented Sep 28, 2023

openshift-ci-robot commented Sep 28, 2023

saifshaikh48 commented Oct 4, 2023

openshift-ci-robot commented Oct 4, 2023

saifshaikh48 commented Oct 4, 2023

sebsoto Oct 4, 2023

aravindhp Oct 4, 2023

sebsoto Oct 4, 2023

aravindhp Oct 4, 2023

saifshaikh48 Oct 5, 2023

aravindhp Oct 4, 2023

aravindhp Oct 4, 2023

openshift-ci bot commented Oct 5, 2023

openshift-ci-robot commented Oct 5, 2023

alinaryan left a comment

saifshaikh48 commented Oct 5, 2023

saifshaikh48 commented Oct 5, 2023

openshift-cherrypick-robot commented Oct 5, 2023

alinaryan commented Oct 5, 2023

sebsoto commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023

openshift-ci-robot commented Oct 6, 2023

openshift-cherrypick-robot commented Oct 6, 2023

OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes" #1774

OCPBUGS-18554: Fix "error removing HNS network when cleaning up BYOH proxy nodes" #1774

Conversation

saifshaikh48 commented Sep 8, 2023

openshift-ci-robot commented Sep 8, 2023

openshift-ci bot commented Sep 8, 2023

saifshaikh48 commented Sep 8, 2023

saifshaikh48 commented Sep 8, 2023

saifshaikh48 commented Sep 8, 2023

openshift-cherrypick-robot commented Sep 8, 2023

mtnbikenc commented Sep 28, 2023

openshift-ci-robot commented Sep 28, 2023

saifshaikh48 commented Oct 4, 2023

openshift-ci-robot commented Oct 4, 2023

saifshaikh48 commented Oct 4, 2023

sebsoto Oct 4, 2023

Choose a reason for hiding this comment

aravindhp Oct 4, 2023

Choose a reason for hiding this comment

sebsoto Oct 4, 2023

Choose a reason for hiding this comment

aravindhp Oct 4, 2023

Choose a reason for hiding this comment

saifshaikh48 Oct 5, 2023

Choose a reason for hiding this comment

aravindhp Oct 4, 2023

Choose a reason for hiding this comment

aravindhp Oct 4, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Oct 5, 2023

openshift-ci-robot commented Oct 5, 2023

alinaryan left a comment

Choose a reason for hiding this comment

saifshaikh48 commented Oct 5, 2023

saifshaikh48 commented Oct 5, 2023

openshift-cherrypick-robot commented Oct 5, 2023

alinaryan commented Oct 5, 2023

sebsoto commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023

openshift-ci-robot commented Oct 6, 2023

openshift-cherrypick-robot commented Oct 6, 2023