🌱 pull [email protected] #1521

tormath1 · 2023-04-05T15:35:34Z

What this PR does / why we need it:

This PR pulls [email protected] (including 1.3.2, 1.3.3, 1.3.4, 1.3.5, 1.3.6 and 1.4.0) - this follow a mention from the office hours

Special notes for your reviewer:

Not so much to report regarding the upgrade as the commits speak by themselves.
BIG QUESTION MARK on the following commit: test/e2e/suites: update remediation flavor
sigs.k8s.io/cluster-api/api/v1alpha{3,4} are deprecated - we currently ignore the warning before agreeing on what to do
Got a weird issue with the CI: cloud-init was failing (but don't know why), cloud-init validation was complaining about permissions not being a string so I fixed this in a commit.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

netlify · 2023-04-05T15:35:40Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`3822684`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64369f85943efd0008aaa725
😎 Deploy Preview	https://deploy-preview-1521--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mdbooth · 2023-04-06T16:05:07Z

/assign

jichenjc · 2023-04-11T03:22:36Z

hack/ci/cloud-init/common.yaml.tpl

@@ -20,13 +20,13 @@ packages:
 package_upgrade: true
 write_files:
 - path: /etc/sysctl.d/devstack.conf
-  permissions: 0644
+  permissions: "0644"


not sure whether it's because latest cloud-init has this kind of restriction?
https://github.com/canonical/cloud-init/pull/1360/files#diff-c18cfd88ec0bd5f1077576fee00a0b2e87cf0b30aa547ad1eaacfd5742e4a757R1870

It might be yes, I'm just wondering why we'd would hitting this only now since new version of cloud-init is released since 1 month (https://launchpad.net/ubuntu/focal/+source/cloud-init).
At least it makes it consistent with the rest of the codebase.

jichenjc · 2023-04-11T03:24:46Z

BIG QUESTION MARK on the following commit: test/e2e/suites: update remediation flavor

seems one additional test added here... so what's the big question?

tormath1 · 2023-04-11T07:02:51Z

BIG QUESTION MARK on the following commit: test/e2e/suites: update remediation flavor

seems one additional test added here... so what's the big question?

@jichenjc I'm not yet super comfortable with the codebase so it's more to ask extra attention on this commit during the review :)

mdbooth

/lgtm

I'd like to wait for @lentzi90's take on the test changes before approving, but hopefully this is good to go. Thanks!

mdbooth · 2023-04-11T11:10:44Z

api/v1alpha6/openstackmachine_types.go

@@ -177,7 +177,7 @@ func (r *OpenStackMachine) SetConditions(conditions clusterv1.Conditions) {
 // SetFailure sets the OpenStackMachine status failure reason and failure message.
 func (r *OpenStackMachine) SetFailure(failureReason errors.MachineStatusError, failureMessage error) {
 	r.Status.FailureReason = &failureReason
-	r.Status.FailureMessage = pointer.StringPtr(failureMessage.Error())
+	r.Status.FailureMessage = pointer.String(failureMessage.Error())


Reviewer note:
pointer.StringPtr === pointer.String

StringPtr is deprecated

Actually all the pointer.<Type>Ptr have been renamed to pointer.<Type> - I think it's to follow the non-redundancy pattern (https://google.github.io/styleguide/go/decisions#package-vs-exported-symbol-name)

mdbooth · 2023-04-11T11:14:11Z

controllers/openstackmachine_controller.go

@@ -590,7 +590,7 @@ func (r *OpenStackMachineReconciler) requeueOpenStackMachinesForUnpausedCluster(
 }

 func (r *OpenStackMachineReconciler) requestsForCluster(ctx context.Context, log logr.Logger, namespace, name string) []ctrl.Request {
-	labels := map[string]string{clusterv1.ClusterLabelName: name}
+	labels := map[string]string{clusterv1.ClusterNameLabel: name}


Reviewer note:
Constant was renamed. Value remains the same.

mdbooth · 2023-04-11T11:16:02Z

controllers/openstackmachine_controller_test.go

@@ -137,7 +137,7 @@ func Test_machineToInstanceSpec(t *testing.T) {
 			machine: func() *clusterv1.Machine {
 				m := getDefaultMachine()
 				m.Labels = map[string]string{
-					clusterv1.MachineControlPlaneLabelName: "true",
+					clusterv1.MachineControlPlaneLabel: "true",


Reviewer note:
Constant was renamed. Value the same.

hack/tools/go.mod

mdbooth · 2023-04-11T11:21:02Z

test/e2e/shared/common.go

@@ -113,7 +113,7 @@ func dumpMachines(ctx context.Context, e2eCtx *E2EContext, namespace *corev1.Nam
 		return
 	}

-	machineNames := sets.NewString()
+	machineNames := sets.New[string]()


mdbooth · 2023-04-11T12:13:55Z

test/e2e/suites/e2e/mhc_remediations_test.go

+var _ = Describe("When testing unhealthy machines remediation", func() {
+	Describe("[mhc-remediations]", func() {


@lentzi90 This change passes my smell test: KCPFlavor and MDFlavor have been removed in favour of just Flavor, but I'd appreciate if you could take a closer look.

Thanks for bringing this to my attention!
It seems like they split the test into two actually. There is now the MachineDeploymentRemediationSpec that we see here, but also KCPRemediationSpec as seen here.
@tormath1 could you please update to use the KCPRemediationSpec here in place of the second MachineDeploymentRemediationSpec? If it is in separate files or not does not matter to me but it would be nice to run both. 🙂

Thanks for the extra check. The content of this commit is not enough then: 9d56439 ?
We test both KCPRemediationSpec and MachineDeploymentRemediationSpec

Umm I may be missing something, but I don't see KCPRemediationSpec? I see two times MachineDeploymentRemediationSpec with different flavors.

Ah perfect thanks - I thought we were speaking about the flavor name :)

lentzi90 · 2023-04-12T09:43:19Z

/test pull-cluster-api-provider-openstack-e2e-full-test

lentzi90

/lgtm

mdbooth · 2023-04-12T09:49:27Z

Can you squash the commits before removing the hold?

/approve

k8s-ci-robot · 2023-04-12T09:49:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth, tormath1

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

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

lentzi90 · 2023-04-12T11:46:57Z

Ow the KCPRemediationSpec is a bit more involved. I didn't realize they changed so much. It is now a much more thorough test but it means we need some changes to make it work. Previously these remediation tests were almost the same, just one for the control plane and one for the MachineDeployment. But now the KCP test is also checking remediation of the very first node that fails to initialize.

They have put some comments in the code for what is needed: https://github.com/kubernetes-sigs/cluster-api/blob/9ad6d519caed6755da688225172d8999be6c7015/test/e2e/kcp_remediations.go#L52-L77
If I understand it correctly we would need the following:

Define wait intervals for wait-cluster, wait-machines, check-machines-stable and wait-machine-provisioned (if they don't already exist) here.
Ensure that the flavor we use have 3 control plane nodes and 0 workers. I think this is already the case.
The control plane must be configured with preKubeadmCommands that queries for a well-known ConfigMap on the management cluster, holding up bootstrap until a signal is passed via the config map. I'm not even sure if this is possible in our CI since it requires the workload cluster to access the management cluster...
The MachineHealthCheck itself that we already have (may need some tweak though, I haven't checked the exact details of it).

Point 3 is the big one. It may be a bit too much to add in this PR. I would be fine with dropping the KCP test for now since it is basically a completely new test after the changes they made. I will anyway investigate if we can add it later when I have time.

Signed-off-by: Mathieu Tortuyaux <[email protected]>

tormath1 · 2023-04-12T12:11:37Z

@mdbooth @lentzi90 thanks for your reviews!

commits have been squashed
dropped the KCP remediation test in favor of Add KCP remediation test #1523
added a comment regarding the golang lint staticcheck

Changes are available here: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/compare/bfcd6d46d5a60a82222b0a87d7c204f0ee64073b..38226846051a7b38e29a9ad4bfeb0539667b1e7a

lentzi90

/lgtm

lentzi90 · 2023-04-12T13:55:00Z

/test pull-cluster-api-provider-openstack-e2e-full-test

tormath1 · 2023-04-12T16:04:59Z

/unhold

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 5, 2023

k8s-ci-robot requested review from seanschneeweiss and tobiasgiese April 5, 2023 15:35

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 5, 2023

tormath1 changed the title ~~🌱 pull [email protected]~~ [wip] 🌱 pull [email protected] Apr 5, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2023

tormath1 force-pushed the tormath1/capi-upgrade branch 2 times, most recently from d3d9c12 to 6f225d9 Compare April 6, 2023 11:55

tormath1 changed the title ~~[wip] 🌱 pull [email protected]~~ 🌱 pull [email protected] Apr 6, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2023

k8s-ci-robot assigned mdbooth Apr 6, 2023

jichenjc reviewed Apr 11, 2023

View reviewed changes

mdbooth reviewed Apr 11, 2023

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 11, 2023

tormath1 force-pushed the tormath1/capi-upgrade branch from 6f225d9 to bfcd6d4 Compare April 12, 2023 09:10

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 12, 2023

lentzi90 reviewed Apr 12, 2023

View reviewed changes

k8s-ci-robot assigned lentzi90 Apr 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2023

tormath1 mentioned this pull request Apr 12, 2023

Add KCP remediation test #1523

Closed

mod: go get -u sigs.k8s.io/[email protected]

3822684

Signed-off-by: Mathieu Tortuyaux <[email protected]>

tormath1 force-pushed the tormath1/capi-upgrade branch from bfcd6d4 to 3822684 Compare April 12, 2023 12:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023

lentzi90 reviewed Apr 12, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2023

k8s-ci-robot merged commit 097d20a into kubernetes-sigs:main Apr 12, 2023

tormath1 deleted the tormath1/capi-upgrade branch April 12, 2023 16:06

		var _ = Describe("When testing unhealthy machines remediation", func() {
		Describe("[mhc-remediations]", func() {

🌱 pull [email protected] #1521

🌱 pull [email protected] #1521

Uh oh!

Conversation

tormath1 commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Uh oh!

mdbooth commented Apr 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jichenjc commented Apr 11, 2023

Uh oh!

tormath1 commented Apr 11, 2023

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lentzi90 commented Apr 12, 2023

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Apr 12, 2023

Uh oh!

k8s-ci-robot commented Apr 12, 2023

Uh oh!

lentzi90 commented Apr 12, 2023

Uh oh!

tormath1 commented Apr 12, 2023

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

lentzi90 commented Apr 12, 2023

Uh oh!

tormath1 commented Apr 12, 2023

Uh oh!

Uh oh!

tormath1 commented Apr 5, 2023 •

edited

Loading

netlify bot commented Apr 5, 2023 •

edited

Loading