MGMT-15559: Change detached annotation condition in non-converged flow #5445

CrystalChun · 2023-08-25T20:51:03Z

MGMT-15559
Day 2 workers create BMH and Machine CRs on the spoke cluster when the host starts installing. The non-converged flow initially added the detached annotation for the BMH when the host starts installing too. This causes the BMH to stop being reconciled so the BMH and Machine CRs aren't created in the spoke cluster.

This change adds the detached annotation when the host reaches rebooting, joined, or failed instead of installing so that it doesn't conflict with adding the BMH/Machine to the spoke cluster.

List all the issues related to this PR

MGMT-15639

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

/cc @ori-amizur

openshift-ci-robot · 2023-08-25T20:51:06Z

openshift-ci · 2023-08-25T20:51:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CrystalChun

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

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

codecov · 2023-08-25T21:12:52Z

Codecov Report

Merging #5445 (180f859) into master (7e71500) will increase coverage by 0.65%.
Report is 10 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5445      +/-   ##
==========================================
+ Coverage   67.65%   68.30%   +0.65%     
==========================================
  Files         232      232              
  Lines       33988    35081    +1093     
==========================================
+ Hits        22994    23963     +969     
- Misses       8948     9036      +88     
- Partials     2046     2082      +36

Files Changed	Coverage Δ
...nal/controller/controllers/bmh_agent_controller.go	`74.91% <100.00%> (+0.14%)`	⬆️

... and 7 files with indirect coverage changes

ori-amizur

Is it possible to add tests to verify that the problem is fixed?

ori-amizur · 2023-08-28T08:18:44Z

internal/controller/controllers/bmh_agent_controller.go

@@ -553,27 +554,6 @@ func (r *BMACReconciler) ensureBMHDetached(log logrus.FieldLogger, bmh *bmh_v1al
 	return reconcileComplete{dirty: true, stop: true}
 }

-// The detached annotation is added if the installation of the agent associated with


Actually I think it is better to keep they were before this fix and just rename and change the function addBMHDetachedAnnotationIfAgentHasStartedInstallation.

Ok sounds good! I reverted it back but I'm not sure what the name of the function should be 😅 any ideas?

addBMHDetachedAnnotationIfHostIsRebooting ?

CrystalChun · 2023-08-31T19:30:39Z

Is it possible to add tests to verify that the problem is fixed?

Yes! Probably an e2e test that ensures that the day2 workers are installed and their BMHs exist in the release repo https://github.com/openshift/release/blob/master/ci-operator/step-registry/assisted/baremetal/operator/ztp/assisted-baremetal-operator-ztp-workflow.yaml#L9
Wdyt?

CrystalChun · 2023-09-06T13:23:27Z

/retitle MGMT-15559: Change detached annotation condition in non-converged flow

openshift-ci-robot · 2023-09-06T13:23:34Z

openshift-ci-robot · 2023-09-06T13:23:56Z

CrystalChun · 2023-09-06T14:27:21Z

/cc @filanov

CrystalChun · 2023-09-06T17:42:13Z

/retest-required

https://issues.redhat.com/browse/MGMT-15559 Day 2 workers create BMH and Machine CRs on the spoke cluster when the host starts installing. The non-converged flow initially added the detached annotation for the BMH when the host starts installing too. This causes the BMH to stop being reconciled so the BMH and Machine CRs aren't created in the spoke cluster. This change adds the detached annotation when the host reaches rebooting, joined, or failed instead of installing so that it doesn't conflict with adding the BMH/Machine to the spoke cluster.

CrystalChun · 2023-09-12T15:20:53Z

/retest

ori-amizur · 2023-09-19T11:32:14Z

/lgtm

openshift-ci · 2023-09-19T14:02:47Z

@CrystalChun: 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.

CrystalChun · 2023-09-19T15:13:04Z

/cherry-pick release-ocm-2.9

openshift-cherrypick-robot · 2023-09-19T15:13:52Z

@CrystalChun: new pull request created: #5502

In response to this:

/cherry-pick release-ocm-2.9

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.

CrystalChun · 2023-09-19T16:14:52Z

/cherry-pick release-ocm-2.8

CrystalChun · 2023-09-19T16:14:59Z

/cherry-pick release-ocm-2.7

openshift-cherrypick-robot · 2023-09-19T16:16:10Z

@CrystalChun: cannot fork openshift/assisted-service: could not fetch all existing repos: Get "http://ghproxy/user/33322735/repos?per_page=100&page=5": dial tcp 172.30.229.2:80: i/o timeout

In response to this:

/cherry-pick release-ocm-2.8

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.

CrystalChun · 2023-09-19T16:29:16Z

/cherry-pick release-ocm-2.8

openshift-cherrypick-robot · 2023-09-19T16:29:57Z

@CrystalChun: new pull request created: #5503

In response to this:

/cherry-pick release-ocm-2.8

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.

CrystalChun · 2023-09-20T21:55:35Z

/cherry-pick release-ocm-2.7

openshift-cherrypick-robot · 2023-09-20T21:56:22Z

@CrystalChun: #5445 failed to apply on top of branch "release-ocm-2.7":

Applying: MGMT-15559: Change detached annotation condition in non-converged flow
Using index info to reconstruct a base tree...
M	internal/controller/controllers/bmh_agent_controller.go
M	internal/controller/controllers/bmh_agent_controller_test.go
Falling back to patching base and 3-way merge...
Auto-merging internal/controller/controllers/bmh_agent_controller_test.go
CONFLICT (content): Merge conflict in internal/controller/controllers/bmh_agent_controller_test.go
Auto-merging internal/controller/controllers/bmh_agent_controller.go
CONFLICT (content): Merge conflict in internal/controller/controllers/bmh_agent_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 MGMT-15559: Change detached annotation condition in non-converged flow
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-ocm-2.7

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#5445) https://issues.redhat.com/browse/MGMT-15559 Day 2 workers create BMH and Machine CRs on the spoke cluster when the host starts installing. The non-converged flow initially added the detached annotation for the BMH when the host starts installing too. This causes the BMH to stop being reconciled so the BMH and Machine CRs aren't created in the spoke cluster. This change adds the detached annotation when the host reaches rebooting, joined, or failed instead of installing so that it doesn't conflict with adding the BMH/Machine to the spoke cluster.

#5445) (#5507) https://issues.redhat.com/browse/MGMT-15559 Day 2 workers create BMH and Machine CRs on the spoke cluster when the host starts installing. The non-converged flow initially added the detached annotation for the BMH when the host starts installing too. This causes the BMH to stop being reconciled so the BMH and Machine CRs aren't created in the spoke cluster. This change adds the detached annotation when the host reaches rebooting, joined, or failed instead of installing so that it doesn't conflict with adding the BMH/Machine to the spoke cluster.

openshift#5445) https://issues.redhat.com/browse/MGMT-15559 Day 2 workers create BMH and Machine CRs on the spoke cluster when the host starts installing. The non-converged flow initially added the detached annotation for the BMH when the host starts installing too. This causes the BMH to stop being reconciled so the BMH and Machine CRs aren't created in the spoke cluster. This change adds the detached annotation when the host reaches rebooting, joined, or failed instead of installing so that it doesn't conflict with adding the BMH/Machine to the spoke cluster.

openshift-ci bot requested a review from ori-amizur August 25, 2023 20:51

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

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 25, 2023

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

ori-amizur reviewed Aug 28, 2023

View reviewed changes

CrystalChun force-pushed the MGMT-15639 branch from b8abf10 to 0c6515d Compare August 31, 2023 19:28

CrystalChun requested a review from ori-amizur September 6, 2023 13:22

openshift-ci bot changed the title ~~MGMT-15639: Change detached annotation condition in non-converged flow~~ MGMT-15559: Change detached annotation condition in non-converged flow Sep 6, 2023

CrystalChun force-pushed the MGMT-15639 branch from 0c6515d to 0dccdb8 Compare September 6, 2023 14:23

openshift-ci bot requested a review from filanov September 6, 2023 14:27

CrystalChun force-pushed the MGMT-15639 branch from 0dccdb8 to 180f859 Compare September 11, 2023 19:59

openshift-ci bot assigned ori-amizur Sep 19, 2023

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

openshift-merge-robot merged commit cabc63d into openshift:master Sep 19, 2023
15 checks passed

openshift-cherrypick-robot mentioned this pull request Sep 19, 2023

[release-ocm-2.9] MGMT-15559: Change detached annotation condition in non-converged flow #5502

Merged

openshift-cherrypick-robot mentioned this pull request Sep 19, 2023

[release-ocm-2.8] MGMT-15559: Change detached annotation condition in non-converged flow #5503

Merged

CrystalChun mentioned this pull request Sep 21, 2023

[release-ocm-2.7] MGMT-15559: Change detached annotation condition in non-converged flow #5507

Merged

20 tasks

CrystalChun deleted the MGMT-15639 branch January 29, 2024 23:46

MGMT-15559: Change detached annotation condition in non-converged flow #5445

MGMT-15559: Change detached annotation condition in non-converged flow #5445

Conversation

CrystalChun commented Aug 25, 2023 • edited

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Aug 25, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Aug 25, 2023

codecov bot commented Aug 25, 2023 • edited

Codecov Report

ori-amizur left a comment

Choose a reason for hiding this comment

ori-amizur Aug 28, 2023 • edited

Choose a reason for hiding this comment

CrystalChun Aug 31, 2023

Choose a reason for hiding this comment

ori-amizur Sep 11, 2023

Choose a reason for hiding this comment

CrystalChun commented Aug 31, 2023

CrystalChun commented Sep 6, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Sep 6, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Sep 6, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

CrystalChun commented Sep 6, 2023

CrystalChun commented Sep 6, 2023

CrystalChun commented Sep 12, 2023

ori-amizur commented Sep 19, 2023

openshift-ci bot commented Sep 19, 2023

CrystalChun commented Sep 19, 2023

openshift-cherrypick-robot commented Sep 19, 2023

CrystalChun commented Sep 19, 2023

CrystalChun commented Sep 19, 2023

openshift-cherrypick-robot commented Sep 19, 2023

CrystalChun commented Sep 19, 2023

openshift-cherrypick-robot commented Sep 19, 2023

CrystalChun commented Sep 20, 2023

openshift-cherrypick-robot commented Sep 20, 2023

CrystalChun commented Aug 25, 2023 •

edited

openshift-ci-robot commented Aug 25, 2023 •

edited by openshift-ci bot

codecov bot commented Aug 25, 2023 •

edited

ori-amizur Aug 28, 2023 •

edited

CrystalChun commented Sep 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 6, 2023 •

edited by openshift-ci bot