🌱 Remove and reinstall BMO in source cluster before and after pivot #838

kashifest · 2023-01-24T11:08:14Z

BMO is caching the old node states after pivoting back and that might be the cause to the flake where the bmh is not deleted in centos this PR tries to solve the issue by reinstating bmo after moving back to source cluster

kashifest · 2023-01-24T11:09:27Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-25T07:41:18Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-25T07:53:28Z

/retest

kashifest · 2023-01-25T07:53:53Z

/test-ubuntu-e2e-main
/test-upgrade-e2e-main

furkatgofurov7 · 2023-01-25T08:59:35Z

/retest

furkatgofurov7 · 2023-01-25T09:06:20Z

/test generate

kashifest · 2023-01-25T10:35:47Z

/retest

kashifest · 2023-01-25T10:36:01Z

/test all

kashifest · 2023-01-25T10:39:11Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-25T17:16:36Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-26T08:33:03Z

/keep-test-ubuntu-integration-e2e-main

kashifest · 2023-01-26T18:08:31Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-26T19:07:10Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-26T19:11:17Z

/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-27T07:03:59Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-27T08:57:36Z

/test-ubuntu-e2e-main
/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-27T09:55:15Z

/test-centos-e2e-main

kashifest · 2023-01-27T11:24:16Z

/test-centos-e2e-main
/test-upgrade-e2e-main

kashifest · 2023-01-27T13:55:00Z

/test-upgrade-e2e-main

kashifest · 2023-01-29T15:40:53Z

/test-upgrade-e2e-main

kashifest · 2023-01-30T08:42:04Z

/test-upgrade-e2e-main

kashifest · 2023-01-30T12:53:54Z

/test-upgrade-e2e-main

kashifest · 2023-01-30T16:54:01Z

/test-ubuntu-integration-main

lentzi90 · 2023-01-31T12:44:41Z

test/e2e/pivoting.go

@@ -33,6 +33,7 @@ const (
 	ironicBasicAuth            = "IRONIC_BASIC_AUTH"
 	Kind                       = "kind"
 	NamePrefix                 = "NAMEPREFIX"
+	NameSuffix                 = "NAMESUFFIX"


This is not used.

Suggested change

NameSuffix = "NAMESUFFIX"

It is used, check RemoveIronicBMOInput below

That is different. It is defined in the struct here

cluster-api-provider-metal3/test/e2e/pivoting.go

Line 295 in 762326f

NameSuffix string

This is a constant that is never referenced. The other constants above are used to get the value from the e2e config as in

cluster-api-provider-metal3/test/e2e/pivoting.go

Line 95 in 762326f

NamePrefix: input.E2EConfig.GetVariable(NamePrefix),

But this NameSuffix does not exist in the e2e config so we never take it from there.

true, good catch! I will remove it, with other change

lentzi90 · 2023-01-31T12:46:46Z

test/e2e/pivoting.go

+	iNamespace := &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: input.E2EConfig.GetVariable(ironicNamespace),
 		},
 	}
-	_, err := targetClusterClientSet.CoreV1().Namespaces().Create(ctx, ironicNamespace, metav1.CreateOptions{})
+	_, err := targetClusterClientSet.CoreV1().Namespaces().Create(ctx, iNamespace, metav1.CreateOptions{})


Why change the variable name? I think it was easier to understand before. iNamespace is harder to understand. Is it "in namespace" or "ironic namespace" or "input namespace"? 🤷

If you see closely, it is creating a Namespace out of the ironicNameSpace value and previously it was assigning it to ironicNameSpace var only. That was ok since ironicNameSpace was not needed as a string later. Now that we need it, simply converting it to string didn't work. So a rename was needed. And it is used in line 102 and 107 only.

Oh I see! What if we rename ironicNamespace -> ironicNamespaceName or ironicNamespaceVar (since it is the variable where we get the name from) and then this can be ironicNamespace?

lentzi90 · 2023-01-31T12:59:26Z

test/e2e/pivoting.go

 }

-func removeIronic(ctx context.Context, inputGetter func() RemoveIronicInput) {
+func removeIronicBMO(ctx context.Context, inputGetter func() RemoveIronicBMOInput) {


This function is starting to feel a bit messy. It is almost a generic function for deleting a Deployment, but it also tries to handle the case when Ironic is running outside the cluster. If we combine the remove BMO part with IsDeployment=false, we get complete nonsense. Also, if we fail to delete the Deployment we get an error that it failed to delete Ironic (even if we tried to delete BMO).

I think a cleaner solution would be to separate out the case when Ironic is running outside the cluster, e.g. removeLocalIronic. Then this function can be a generic deleteDeployment function. No need for the prefix/suffix either, just take the name as input.

What do you think?

combining ironic local and deployment deletion in one function was there before. I am not adding that functionality here. I am adding BMO removal only. And since we know its always a deployment we always hardcode the isDeployment to true. Expecting common sense from developer here. Since a developer can also try to delete ironic deployment while it was deployed locally. One good point raised by you is the error message at line 304. We can add the deployment or component name to be deleted which I missed out .

combining ironic local and deployment deletion in one function was there before

I know, but adding BMO to this makes it harder to understand and easier to make mistakes. Splitting it into two functions was just a suggestion for how to make it cleaner, we can go the other way also. Then I would suggest to make it "smarter", similar to installIronicBMO. Add to the input a variable that tells if it is BMO or Ironic we are dealing with, then use it to set the other parameters that are already known, e.g. prefix/suffix. We also know that BMO is never deployed locally in this context so check and error out if we get this combination.

I am in favor of keeping the function core as it is now as both ironic and bmo have the same lifecycle and do the refactoring later on?

that might be this work will not solve the centos issue to delete bmh and then I would prefer to revert this changes

I'm not understanding the issue. Why make a messy implementation that we have to fix in the future when we can make a nice implementation? Both of them can easily be reverted but only one have to be improved.

Just think about how the function is used. I want to remove BMO, ok removeIronicBMO sounds good. What does it take as input? Name suffix, what is this? Why should I have to know this to remove BMO? I just want to say if I want to remove BMO or Ironic.

I stand my ground. This must be simplified. Either split it into separate functions or make it "smarter" so it is harder to shoot yourself in the foot.

Just think about how the function is used. I want to remove BMO, ok removeIronicBMO sounds good. What does it take as input? Name suffix, what is this? Why should I have to know this to remove BMO? I just want to say if I want to remove BMO or Ironic.

I didn't get your point here, this is not asked from user. Theres bunch of other similar suffix, prefix parameter in the same file here. You as a user dont need to know, you as a developer will always code the name to delete no matter how simplified the function is. Suffix is a part of it.

I can agree on one point , this might be a bit difficult to understand in future since ironic is deployed locally and in deployment while BMO is only deployed as a deployment. So for BMO isDeployment var doesn’t make any sense and leaves room for mistakes. Splitting it in two functions increases readability and understanding of the code.

There is always a user. The user of the function is the developer. And no, the developer does not need to know the suffix and should not need to think about this. It is always -controller-manager for BMO and -ironic for Ironic. This can be taken care of by the function.

Alternatively, if you want to keep the flexibility of setting the exact name, then just make a generic function that can delete any deployment by giving a name. I'm also fine with that. But mixing it in one function is not nice.

There is always a user. The user of the function is the developer. And no, the developer does not need to know the suffix and should not need to think about this. It is always -controller-manager for BMO and -ironic for Ironic. This can be taken care of by the function.

Hummm, I don’t think I agree. let’s agree to disagree here. Atleast we agree we need separate functions :)

tuminoid · 2023-04-14T09:03:23Z

Please add proper description and commit message.

mboukhalfa · 2023-04-24T14:17:40Z

/test-centos-e2e-feature-main

mboukhalfa · 2023-04-24T16:03:50Z

/test-centos-e2e-feature-main

mboukhalfa · 2023-04-24T16:20:02Z

/test-centos-e2e-feature-main

mboukhalfa · 2023-04-24T17:11:35Z

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

kashifest · 2023-04-28T08:17:47Z

I know its going to fail but just running it so that we can see it as a test in github.
/test-ubuntu-integration-release-1-4

kashifest · 2023-04-28T09:23:52Z

I know its going to fail but just running it so that we can see it as a test in github. /test-ubuntu-integration-release-1-4

Funny enough, it passed

mboukhalfa · 2023-05-09T06:50:54Z

/test-centos-e2e-integration-main

mboukhalfa · 2023-05-09T16:58:44Z

/test-centos-e2e-integration-main

mboukhalfa · 2023-06-06T14:03:14Z

/test-centos-e2e-integration-main

mboukhalfa · 2023-06-30T13:07:04Z

/keep-test-centos-e2e-integration-main

mboukhalfa · 2023-06-30T14:40:23Z

/keep-test-centos-e2e-integration-main

mboukhalfa · 2023-06-30T14:51:38Z

/test-centos-e2e-integration-main

mboukhalfa · 2023-06-30T16:03:19Z

/test-ubuntu-integration-main

mboukhalfa · 2023-06-30T16:07:24Z

/assign @furkatgofurov7

metal3-io-bot · 2023-07-03T07:38:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7

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

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

Sunnatillo

/lgtm

metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 24, 2023

kashifest force-pushed the add-remove-bmo-pivot branch from a862993 to d689a8a Compare January 25, 2023 07:40

kashifest force-pushed the add-remove-bmo-pivot branch from d689a8a to def4b61 Compare January 26, 2023 19:06

kashifest force-pushed the add-remove-bmo-pivot branch from def4b61 to 14684d4 Compare January 27, 2023 07:03

kashifest force-pushed the add-remove-bmo-pivot branch from 14684d4 to 762326f Compare January 27, 2023 08:57

lentzi90 suggested changes Jan 31, 2023

View reviewed changes

kashifest force-pushed the add-remove-bmo-pivot branch from 762326f to 9053948 Compare February 1, 2023 07:51

mboukhalfa force-pushed the add-remove-bmo-pivot branch from 157b6cc to cca7237 Compare April 14, 2023 08:56

mboukhalfa force-pushed the add-remove-bmo-pivot branch from cca7237 to 64c52df Compare April 24, 2023 14:16

mboukhalfa force-pushed the add-remove-bmo-pivot branch from ed00a04 to 9cf319b Compare May 9, 2023 16:56

mboukhalfa force-pushed the add-remove-bmo-pivot branch from 9cf319b to 7e04470 Compare June 30, 2023 14:40

remove bmo after repivote

f968a98

mboukhalfa force-pushed the add-remove-bmo-pivot branch from 7e04470 to f968a98 Compare June 30, 2023 14:46

metal3-io-bot assigned furkatgofurov7 Jun 30, 2023

mboukhalfa requested a review from lentzi90 June 30, 2023 16:07

furkatgofurov7 approved these changes Jul 3, 2023

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2023

Sunnatillo reviewed Jul 3, 2023

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2023

metal3-io-bot merged commit 4ba5131 into metal3-io:main Jul 3, 2023
3 checks passed

kashifest deleted the add-remove-bmo-pivot branch August 29, 2023 07:56

🌱 Remove and reinstall BMO in source cluster before and after pivot #838

🌱 Remove and reinstall BMO in source cluster before and after pivot #838

Conversation

kashifest commented Jan 24, 2023 • edited by mboukhalfa Loading

kashifest commented Jan 24, 2023

kashifest commented Jan 25, 2023

kashifest commented Jan 25, 2023

kashifest commented Jan 25, 2023

furkatgofurov7 commented Jan 25, 2023

furkatgofurov7 commented Jan 25, 2023

kashifest commented Jan 25, 2023

kashifest commented Jan 25, 2023

kashifest commented Jan 25, 2023

kashifest commented Jan 25, 2023

kashifest commented Jan 26, 2023

kashifest commented Jan 26, 2023

kashifest commented Jan 26, 2023

kashifest commented Jan 26, 2023

kashifest commented Jan 27, 2023

kashifest commented Jan 27, 2023

kashifest commented Jan 27, 2023

kashifest commented Jan 27, 2023

kashifest commented Jan 27, 2023

kashifest commented Jan 29, 2023

kashifest commented Jan 30, 2023

kashifest commented Jan 30, 2023

kashifest commented Jan 30, 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

kashifest Jan 31, 2023 • edited Loading

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

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

kashifest Apr 14, 2023 • edited Loading

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

tuminoid commented Apr 14, 2023

mboukhalfa commented Apr 24, 2023

mboukhalfa commented Apr 24, 2023

mboukhalfa commented Apr 24, 2023

mboukhalfa commented Apr 24, 2023

kashifest commented Apr 28, 2023

kashifest commented Apr 28, 2023

mboukhalfa commented May 9, 2023

mboukhalfa commented May 9, 2023

mboukhalfa commented Jun 6, 2023

mboukhalfa commented Jun 30, 2023

mboukhalfa commented Jun 30, 2023

mboukhalfa commented Jun 30, 2023

mboukhalfa commented Jun 30, 2023

mboukhalfa commented Jun 30, 2023

metal3-io-bot commented Jul 3, 2023

Sunnatillo left a comment

Choose a reason for hiding this comment

kashifest commented Jan 24, 2023 •

edited by mboukhalfa

Loading

kashifest Jan 31, 2023 •

edited

Loading

kashifest Apr 14, 2023 •

edited

Loading