OCPBUGS-44244: use node node in deleted machine phases #29269

kannon92 · 2024-11-05T21:56:59Z

This PR makes sure to add a node name to the DeletedMachine monitor.

Once that is added, we also added some unit tests to this code to make sure that we are correctly detecting UnexpectedNodeNotReady / UnexpectedUnreachable.

The main edge case we had to work around was when Machines get deleted and that node goes not ready. This is correct behavior but our test was flagging this as this code path does not go through a Machine Config change.

openshift-ci-robot · 2024-11-05T21:57:28Z

@kannon92: This pull request references Jira Issue OCPBUGS-44244, 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.18.0) matches configured target version for branch (4.18.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

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

In response to 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.

kannon92 · 2024-11-05T21:57:30Z

/jira refresh

openshift-ci-robot · 2024-11-05T21:57:34Z

@kannon92: This pull request references Jira Issue OCPBUGS-44244, which is valid.

3 validation(s) were run on this bug

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

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

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 openshift-eng/jira-lifecycle-plugin repository.

kannon92 · 2024-11-05T23:39:13Z

/hold

I need to write some unit tests.

dgoodwin · 2024-11-06T12:47:07Z

pkg/monitortests/machines/watchmachines/machines.go

+			}
+
+			if newHasNodeRef {
+				nodeName = machine.Status.NodeRef.Name


So getting a correct node name into the interval annotation will allow your test logic to correctly correlate this deleting machine with a node not ready, and ignore it going forward? i.e. this will cause LESS failures without having to make the test flake.

Suggest explaining some of that in the commit message + PR when you finish this up and have added the unit tests.

I don't have a lot of real estate to work with for commit message. I added this to the description.

openshift-ci-robot · 2024-11-06T20:07:30Z

@kannon92: An error was encountered querying GitHub for users with public email (schoudha@redhat.com) for bug OCPBUGS-44244 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details.

Full error message.


Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: i/o timeout

Please contact an administrator to resolve this issue, then request a bug refresh with /jira refresh.

In response to this:

This PR makes sure to add a node name to the DeletedMachine monitor.

Once that is added, we also added some unit tests to this code to make sure that we are correctly detecting UnexpectedNodeNotReady / UnexpectedUnreachable.

The main edge case we had to work around was when Machines get deleted and that node goes not ready. This is correct behavior but our test was flagging this as this code path does not go through a Machine Config change.

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.

kannon92 · 2024-11-06T20:07:36Z

cc @rphillips @deads2k @dgoodwin

This is ready. I want to run the tests off of this PR so please don't merge until we confirm this works.

rphillips · 2024-11-06T20:45:33Z

Nice. This looks good... Let's see how the tests go.

kannon92 · 2024-11-06T23:22:03Z

/hold cancel

Something is up with these tests..

kannon92 · 2024-11-06T23:22:19Z

/jira refresh

openshift-ci-robot · 2024-11-06T23:22:22Z

@kannon92: This pull request references Jira Issue OCPBUGS-44244, which is valid.

3 validation(s) were run on this bug

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

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-11-07T00:21:26Z

@kannon92: This pull request references Jira Issue OCPBUGS-44244. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state.

In response to this:

This PR makes sure to add a node name to the DeletedMachine monitor.

Once that is added, we also added some unit tests to this code to make sure that we are correctly detecting UnexpectedNodeNotReady / UnexpectedUnreachable.

The main edge case we had to work around was when Machines get deleted and that node goes not ready. This is correct behavior but our test was flagging this as this code path does not go through a Machine Config change.

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-11-07T00:21:28Z

@kannon92: This pull request references Jira Issue OCPBUGS-44244, 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.18.0) matches configured target version for branch (4.18.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

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

In response to this:

This PR makes sure to add a node name to the DeletedMachine monitor.

Once that is added, we also added some unit tests to this code to make sure that we are correctly detecting UnexpectedNodeNotReady / UnexpectedUnreachable.

The main edge case we had to work around was when Machines get deleted and that node goes not ready. This is correct behavior but our test was flagging this as this code path does not go through a Machine Config change.

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-trt-bot · 2024-11-07T06:09:02Z

Job Failure Risk Analysis for sha: 8b023ce

Job Name	Failure Risk
pull-ci-openshift-origin-master-e2e-aws-ovn-single-node-serial	IncompleteTests Tests for this run (26) are below the historical average (1642): IncompleteTests (not enough tests ran to make a reasonable risk analysis; this could be due to infra, installation, or upgrade problems)

pacevedom · 2024-11-07T15:25:45Z

/test e2e-aws-ovn-microshift

kannon92 · 2024-11-07T15:28:22Z

Found a little issue and kicked off a new round of testing.

I did not realize that the MachinePhase interval was constructed. So I added it to the machineDeletedInAPI but it wasn't in the MachinePhase interval. This should be fixed.

Once a test finishes, I will verify the artifacts to see if this is correct.

kannon92 · 2024-11-08T01:46:12Z

/retest

/hold cancel

Confirmed that the machines have the right node names in the MachinePhase Intervals.

openshift-trt-bot · 2024-11-08T05:14:56Z

Job Failure Risk Analysis for sha: dd5eb16

Job Name	Failure Risk
pull-ci-openshift-origin-master-e2e-aws-ovn-single-node-upgrade	Low [sig-node] static pods should start after being created This test has passed 71.43% of 154 runs on release 4.18 [Architecture:amd64 FeatureSet:default Installer:ipi Network:ovn NetworkStack:ipv4 Platform:aws SecurityMode:default Topology:single Upgrade:micro] in the last week. Open Bugs Static pod controller pods sometimes fail to start [etcd] --- [sig-node] static pods should start after being created This test has passed 71.43% of 154 runs on release 4.18 [Architecture:amd64 FeatureSet:default Installer:ipi Network:ovn NetworkStack:ipv4 Platform:aws SecurityMode:default Topology:single Upgrade:micro] in the last week. Open Bugs Static pod controller pods sometimes fail to start [etcd]

kannon92 · 2024-11-08T14:31:52Z

@deads2k @dgoodwin @rphillips
This PR is ready to merge. PTAL.

dgoodwin

Looks good to me just a couple nits.

dgoodwin · 2024-11-08T17:25:18Z

pkg/monitortests/node/watchnodes/node_test.go

+			unexpectedReason: monitorapi.NodeUnexpectedReadyReason,
+		},
+		{
+			// OCPBUG-44244: if machine monitor does not have node name than we will get no failures


This test looks to expect a failure. Just a comment that needs updating?

yea. updated.

dgoodwin · 2024-11-08T17:26:14Z

pkg/monitortests/node/watchnodes/node_test.go

+			unexpectedReason: monitorapi.NodeUnexpectedUnreachableReason,
+		},
+		{
+			// OCPBUG-44244: if machine monitor does not have node name than we will get no failures


expects a failure as well

dgoodwin · 2024-11-08T17:37:55Z

/lgtm

openshift-ci-robot · 2024-11-08T19:28:00Z

/retest-required

Remaining retests: 0 against base HEAD 9cffbdd and 2 for PR HEAD 1bf8c85 in total

openshift-ci-robot · 2024-11-09T01:28:05Z

/retest-required

Remaining retests: 0 against base HEAD 23ca269 and 1 for PR HEAD 1bf8c85 in total

openshift-ci-robot · 2024-11-09T15:27:53Z

/retest-required

Remaining retests: 0 against base HEAD 6807d0c and 0 for PR HEAD 1bf8c85 in total

openshift-ci-robot · 2024-11-09T18:28:02Z

/hold

Revision 1bf8c85 was retested 3 times: holding

openshift-trt-bot · 2024-11-09T19:28:17Z

Job Failure Risk Analysis for sha: 1bf8c85

Job Name	Failure Risk
pull-ci-openshift-origin-master-e2e-aws-ovn-edge-zones	IncompleteTests Tests for this run (102) are below the historical average (3107): IncompleteTests (not enough tests ran to make a reasonable risk analysis; this could be due to infra, installation, or upgrade problems)

kannon92 · 2024-11-12T15:14:53Z

/hold cancel

dgoodwin · 2024-11-12T15:19:52Z

/lgtm

openshift-ci · 2024-11-12T15:20:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgoodwin, kannon92

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

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

openshift-ci · 2024-11-12T19:00:36Z

@kannon92: 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-aws-ovn-upgrade	`de21794`	link	false	`/test e2e-aws-ovn-upgrade`

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 · 2024-11-12T19:04:48Z

@kannon92: Jira Issue OCPBUGS-44244: All pull requests linked via external trackers have merged:

openshift/origin#29269

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

In response to this:

This PR makes sure to add a node name to the DeletedMachine monitor.

Once that is added, we also added some unit tests to this code to make sure that we are correctly detecting UnexpectedNodeNotReady / UnexpectedUnreachable.

The main edge case we had to work around was when Machines get deleted and that node goes not ready. This is correct behavior but our test was flagging this as this code path does not go through a Machine Config change.

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-bot · 2024-11-12T20:52:23Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-tests
This PR has been included in build openshift-enterprise-tests-container-v4.18.0-202411122008.p0.g5e59368.assembly.stream.el9.
All builds following this will include this PR.

kannon92 · 2024-11-19T12:28:07Z

/cherry-pick release-4.17

openshift-cherrypick-robot · 2024-11-19T12:30:37Z

@kannon92: #29269 failed to apply on top of branch "release-4.17":

Applying: Fix UnexpectedNodeNotReady and UnexpectedNodeUnreachable
Using index info to reconstruct a base tree...
M	pkg/monitortests/node/kubeletlogcollector/node.go
M	pkg/monitortests/node/kubeletlogcollector/node_test.go
M	pkg/monitortests/node/watchnodes/monitortest.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/monitortests/node/watchnodes/monitortest.go
CONFLICT (content): Merge conflict in pkg/monitortests/node/watchnodes/monitortest.go
Auto-merging pkg/monitortests/node/kubeletlogcollector/node_test.go
CONFLICT (content): Merge conflict in pkg/monitortests/node/kubeletlogcollector/node_test.go
Auto-merging pkg/monitortests/node/kubeletlogcollector/node.go
CONFLICT (content): Merge conflict in pkg/monitortests/node/kubeletlogcollector/node.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix UnexpectedNodeNotReady and UnexpectedNodeUnreachable

In response to this:

/cherry-pick release-4.17

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.

kannon92 changed the title ~~use node node in deleted machine phases~~ OCPBUGS-44244: use node node in deleted machine phases Nov 5, 2024

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

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Nov 5, 2024

openshift-ci bot requested review from p0lyn0mial and sjenning November 5, 2024 21:57

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

dgoodwin reviewed Nov 6, 2024

View reviewed changes

kannon92 force-pushed the fix-machine-phase-deletion branch from f44e549 to 13c26b8 Compare November 6, 2024 20:04

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

kannon92 closed this Nov 7, 2024

kannon92 reopened this Nov 7, 2024

kannon92 force-pushed the fix-machine-phase-deletion branch from 13c26b8 to 8b023ce Compare November 7, 2024 01:55

kannon92 force-pushed the fix-machine-phase-deletion branch from 44e19ca to dd5eb16 Compare November 7, 2024 19:10

kannon92 requested a review from dgoodwin November 8, 2024 14:31

dgoodwin reviewed Nov 8, 2024

View reviewed changes

openshift-ci bot assigned dgoodwin Nov 8, 2024

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 8, 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 Nov 9, 2024

Fix UnexpectedNodeNotReady and UnexpectedNodeUnreachable

de21794

kannon92 force-pushed the fix-machine-phase-deletion branch from 1bf8c85 to de21794 Compare November 12, 2024 15:14

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 12, 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 Nov 12, 2024

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

openshift-merge-bot bot merged commit 5e59368 into openshift:master Nov 12, 2024
26 of 27 checks passed

kannon92 mentioned this pull request Nov 19, 2024

[release-4.17] OCPBUGS-44190: Fix UnexpectedNodeNotReady and UnexpectedNodeUnreachable #29306

Open

OCPBUGS-44244: use node node in deleted machine phases #29269

OCPBUGS-44244: use node node in deleted machine phases #29269

Conversation

kannon92 commented Nov 5, 2024 • edited Loading

openshift-ci-robot commented Nov 5, 2024

kannon92 commented Nov 5, 2024

openshift-ci-robot commented Nov 5, 2024

kannon92 commented Nov 5, 2024

dgoodwin Nov 6, 2024

Choose a reason for hiding this comment

kannon92 Nov 6, 2024

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 6, 2024

kannon92 commented Nov 6, 2024

rphillips commented Nov 6, 2024

kannon92 commented Nov 6, 2024

kannon92 commented Nov 6, 2024

openshift-ci-robot commented Nov 6, 2024

openshift-ci-robot commented Nov 7, 2024

openshift-ci-robot commented Nov 7, 2024

openshift-trt-bot commented Nov 7, 2024

pacevedom commented Nov 7, 2024

kannon92 commented Nov 7, 2024

kannon92 commented Nov 8, 2024

openshift-trt-bot commented Nov 8, 2024

kannon92 commented Nov 8, 2024

dgoodwin left a comment

Choose a reason for hiding this comment

dgoodwin Nov 8, 2024

Choose a reason for hiding this comment

kannon92 Nov 8, 2024

Choose a reason for hiding this comment

dgoodwin Nov 8, 2024

Choose a reason for hiding this comment

kannon92 Nov 8, 2024

Choose a reason for hiding this comment

dgoodwin commented Nov 8, 2024

openshift-ci-robot commented Nov 8, 2024

openshift-ci-robot commented Nov 9, 2024

openshift-ci-robot commented Nov 9, 2024

openshift-ci-robot commented Nov 9, 2024

openshift-trt-bot commented Nov 9, 2024

kannon92 commented Nov 12, 2024

dgoodwin commented Nov 12, 2024

openshift-ci bot commented Nov 12, 2024

openshift-ci bot commented Nov 12, 2024

openshift-ci-robot commented Nov 12, 2024

openshift-bot commented Nov 12, 2024

kannon92 commented Nov 19, 2024

openshift-cherrypick-robot commented Nov 19, 2024

kannon92 commented Nov 5, 2024 •

edited

Loading