Add support to MHC created CR #111

clobrano · 2023-12-16T14:35:12Z

No description provided.

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

openshift-ci · 2023-12-16T14:35:16Z

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 · 2023-12-16T14:35:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano

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

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

clobrano · 2023-12-17T07:32:12Z

/test?

openshift-ci · 2023-12-17T07:32:16Z

@clobrano: The following commands are available to trigger required jobs:

/test 4.12-ci-bundle-machine-deletion-remediation-bundle
/test 4.12-images
/test 4.12-openshift-e2e
/test 4.12-unit
/test 4.13-ci-bundle-machine-deletion-remediation-bundle
/test 4.13-images
/test 4.13-openshift-e2e
/test 4.13-unit
/test 4.14-ci-bundle-machine-deletion-remediation-bundle
/test 4.14-images
/test 4.14-openshift-e2e
/test 4.14-unit
/test 4.15-ci-bundle-machine-deletion-remediation-bundle
/test 4.15-images
/test 4.15-openshift-e2e
/test 4.15-unit

Use /test all to run all jobs.

In response to this:

/test?

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.

clobrano · 2023-12-17T07:33:03Z

/test 4.15-openshift-e2e

razo7

Still a WIP but I saw some stuff that can be addressed now.

controllers/machinedeletionremediation_controller.go

controllers/machinedeletionremediation_controller_test.go

controllers/machinedeletionremediation_controller.go

In case the MDR CR is created by MHC - get the remediation target Machine from CR's ownerReference of Kind "Machine" - Ignore if the target Machine has no Nodes Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

clobrano · 2023-12-19T16:09:28Z

/test 4.14-openshift-e2e

mshitrit · 2023-12-20T09:32:11Z

controllers/machinedeletionremediation_controller.go

+	// turns it means that the Machine must exist in the cluster, otherwise an error is returned.
+	mustExist := machineName == ""
+
+	if machineName == "" && len(remediation.GetOwnerReferences()) > 0 {


machineName == "" can be replaced with mustExist

technically yes, semantically not so much IMHO...

I think this pattern is confusing.

someBool := someEvaluation if someEvaluation { // Do something }

When I read this sort of code I immediately wonder why wasn't boolean used in the if clause since the exact same evaluation was done one line before.
I suggest either add a comment on mustExist that explains that, or use a different name than mustExist that will explain it.

or use a different name

+1
maybe something like isUnhandledMachine, which explains the situation, and implies indirectly that the machine has to exist

I added a specific comment to mustExist in the change below, together with a rework of the logic. @mshitrit what do you think?

16570a4 l

I think it's better with the comment rather than without, but it still doesn't explain why we are not using the boolean in the if clause.
Let me suggest the following refactoring:

//this method will contain all the logic of getting the machineName (annotation/ownerRef or remediation) machineName, machineNs, err := getMachineData(remediation, MachineNameNsAnnotation) ... if err := r.Get(ctx, key, machine); err != nil { .. if isMachineNameExistInAnnotation(remediation, MachineNameNsAnnotation){ ...

I think it'll make the code much clearer, one downside is that we might need to get the data from the annotation twice- but IMO it is worth the trade-off.

In any case feel free to use different method name, or disregard completely - I don't think this is major enough to block on.

I understand your point. Maybe we can get the same result renaming mustExist as you suggested in isUnhandledMachine (as suggested by Marc)?

I propose this because I think that isMachineNameExistInAnnotation would still need an explanation as well as mustExist does. Changing the name I could write something like (kind of pseudo code)

name, ns := getDataFromAnnotation() isUnhandledMachine := name == "" if isUnhandledMachine { // get the Machine from the cluster }

wdyt?

In general I think it's a good idea to use isUnhandledMachine in the if clause instead of evaluating name == "" again, which I find confusing.
I also think that isUnhandledMachine is a better name.

I do think this is still a bit confusing because isUnhandledMachine value may not reflect the actual logic.
IIUC We only decide that the machine is unhandled after we also check ownerRef & remediation.
So to put it in other words isUnhandledMachine value may equal true when logically it is actually need to be false.

Another option is just to rename isMachineNameExistInAnnotation method to isUnhandledMachinein my original suggestion.

In any case I do think your suggestion is better than the current status and in any case I consider this a NIT so do feel free to implement it as you decide best 😄

Let's merge this now then, I'll follow up with a specific PR :)

controllers/machinedeletionremediation_controller.go

clobrano · 2023-12-20T10:22:17Z

Converting the PR to draft to prevent all tests to run

slintes

I don't think it makes a difference in the code, but just as a reminder: CR created by NHC can also have Machine owners (needed for Metal3 remediation)

slintes · 2023-12-20T12:27:19Z

controllers/machinedeletionremediation_controller.go

@@ -625,3 +667,18 @@ func (r *MachineDeletionRemediationReconciler) getMachineOwnerSpecReplicas(ctx c
 	}
 	return int(replicas), nil
 }
+
+func (r *MachineDeletionRemediationReconciler) getMachineKindAndNamespace(ctx context.Context) (kind, namespace string, err error) {


seems to be unused

slintes · 2023-12-20T12:30:06Z

controllers/machinedeletionremediation_controller.go

@@ -576,6 +588,36 @@ func (r *MachineDeletionRemediationReconciler) getMachineOwnerNodes(ctx context.
 	return machineOwnerNodes, nil
 }

+func (r *MachineDeletionRemediationReconciler) getMachineNameNsFromCR(ctx context.Context, remediation *v1alpha1.MachineDeletionRemediation) (machineName, machineNs string, err error) {


maybe rename to ...FromName, for similar to ...FromOwnerReference?

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

For coherence with getMachineNameNsFromOwnerReference, rename getMachineNameNsFromCR into getMachineNameNsFromRemediationName. Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

controllers/machinedeletionremediation_controller.go

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

clobrano · 2023-12-21T08:59:03Z

/test 4.14-openshift-e2e

mshitrit · 2023-12-25T07:24:31Z

/lgtm
/hold
I left a suggestion for some refactoring but not blocking on it.

clobrano · 2024-01-02T10:30:53Z

/retest

clobrano · 2024-01-02T13:16:21Z

/unhold
I will address the rest of the remarks in a following PR

clobrano added 3 commits November 24, 2023 10:35

Use shorter variable name for MDR remediation

9a22e84

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

Replace deprecated k8s.io/utils/pointer with k8s.io/utils/ptr

3e3bd8f

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

Add functions to create CR with specific owners NHC and MHC

cc237e4

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

openshift-ci bot added the do-not-merge/work-in-progress label Dec 16, 2023

openshift-ci bot added the approved label Dec 16, 2023

clobrano changed the title ~~Add support to MHC created CR~~ [WIP] Add support to MHC created CR Dec 16, 2023

razo7 reviewed Dec 17, 2023

View reviewed changes

razo7 reviewed Dec 19, 2023

View reviewed changes

controllers/machinedeletionremediation_controller.go Outdated Show resolved Hide resolved

Add support for MHC created remediations

ecf2c95

In case the MDR CR is created by MHC - get the remediation target Machine from CR's ownerReference of Kind "Machine" - Ignore if the target Machine has no Nodes Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

clobrano force-pushed the enhance-support-for-mhc/0 branch from c151227 to ecf2c95 Compare December 19, 2023 14:58

clobrano marked this pull request as ready for review December 20, 2023 08:53

clobrano changed the title ~~[WIP] Add support to MHC created CR~~ Add support to MHC created CR Dec 20, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 20, 2023

openshift-ci bot requested review from beekhof and slintes December 20, 2023 08:53

mshitrit reviewed Dec 20, 2023

View reviewed changes

controllers/machinedeletionremediation_controller.go Outdated Show resolved Hide resolved

controllers/machinedeletionremediation_controller.go Show resolved Hide resolved

clobrano marked this pull request as draft December 20, 2023 10:22

openshift-ci bot added the do-not-merge/work-in-progress label Dec 20, 2023

slintes requested changes Dec 20, 2023

View reviewed changes

openshift-ci bot assigned slintes Dec 20, 2023

clobrano added 2 commits December 20, 2023 13:57

Removed unnecessary global const

7d4ae61

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

Rename function to get Machine Name Ns from remediation

baf3f7e

For coherence with getMachineNameNsFromOwnerReference, rename getMachineNameNsFromCR into getMachineNameNsFromRemediationName. Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

mshitrit reviewed Dec 20, 2023

View reviewed changes

controllers/machinedeletionremediation_controller.go Outdated Show resolved Hide resolved

clobrano added 2 commits December 20, 2023 14:00

Remove unused function

e6a7e62

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

Improve readability of the logic to get Machine NamespacedName

16570a4

Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>

openshift-ci bot assigned mshitrit Dec 25, 2023

openshift-ci bot added do-not-merge/hold lgtm labels Dec 25, 2023

clobrano marked this pull request as ready for review January 2, 2024 09:01

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 2, 2024

openshift-ci bot requested review from mshitrit and slintes January 2, 2024 09:02

openshift-ci bot removed the do-not-merge/hold label Jan 2, 2024

openshift-merge-bot bot merged commit 1bac83e into medik8s:main Jan 2, 2024
19 checks passed

clobrano mentioned this pull request Jan 3, 2024

Refactor getMachine to clarify machine-not-found error conditions #113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to MHC created CR #111

Add support to MHC created CR #111

clobrano commented Dec 16, 2023

openshift-ci bot commented Dec 16, 2023

openshift-ci bot commented Dec 16, 2023

clobrano commented Dec 17, 2023

openshift-ci bot commented Dec 17, 2023

clobrano commented Dec 17, 2023

razo7 left a comment

clobrano commented Dec 19, 2023

mshitrit Dec 20, 2023

slintes Dec 20, 2023

mshitrit Dec 21, 2023

slintes Dec 21, 2023

clobrano Dec 21, 2023

mshitrit Dec 25, 2023 •

edited

Loading

clobrano Dec 28, 2023

mshitrit Dec 31, 2023 •

edited

Loading

clobrano Jan 2, 2024

clobrano commented Dec 20, 2023

slintes left a comment

slintes Dec 20, 2023

slintes Dec 20, 2023

clobrano commented Dec 21, 2023

mshitrit commented Dec 25, 2023

clobrano commented Jan 2, 2024

clobrano commented Jan 2, 2024

Add support to MHC created CR #111

Add support to MHC created CR #111

Conversation

clobrano commented Dec 16, 2023

openshift-ci bot commented Dec 16, 2023

openshift-ci bot commented Dec 16, 2023

clobrano commented Dec 17, 2023

openshift-ci bot commented Dec 17, 2023

clobrano commented Dec 17, 2023

razo7 left a comment

Choose a reason for hiding this comment

clobrano commented Dec 19, 2023

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

mshitrit Dec 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshitrit Dec 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clobrano commented Dec 20, 2023

slintes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clobrano commented Dec 21, 2023

mshitrit commented Dec 25, 2023

clobrano commented Jan 2, 2024

clobrano commented Jan 2, 2024

mshitrit Dec 25, 2023 •

edited

Loading

mshitrit Dec 31, 2023 •

edited

Loading