Skip to content

Conversation

@salasberryfin
Copy link
Contributor

@salasberryfin salasberryfin commented Feb 25, 2025

/kind bug

What this PR does / why we need it:

To align GCPManagedControlPlane specification and status with CAPI >=v1.9 we need to introduce new control plane version fields and set existing ones for deprecation in coming releases. The list of changes, as agreed on Slack (thanks @damdo @richardcase @cpanato):

  • Introduce new Version fields in GCPManagedControlPlaneSpec and GCPManagedControlPlaneStatus as per the updated CAPI contract for control planes.
  • Mark ControlPlaneVersion and CurrentVersion as deprecated with a message saying to use Version instead.
  • Have a validation webhook that disallows setting Version and ControlPlaneVersion.
  • Update all templates and e2e tests to use Version.

Which issue(s) this PR fixes:
Fixes #1433

Special notes for your reviewer:

In the future, as the deprecated field is ultimately removed, we can add a conversion from ControlPlaneVersion to Version.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

action required
!! ACTION REQUIRED BEFORE UPGRADING !!
If you are using the GCPManagedControlPlane to provision GKE clusters and you do not have a spec.Version specified in such resource (or you are using spec.ControlPlaneVersion), you will need to either:
a) explicitly set such `spec.Version` field before upgrading CAPG (if you are already using spec.ControlPlaneVersion, please, use spec.Version instead)
or b) disable the MachineSetPreflightChecks in your cluster either:
b1) by setting this core CAPI feature gate to `false`
b2) or by disabling it via the relevant annotation on all the machineSets belonging to said cluster (follow this guide on how to do this: https://cluster-api.sigs.k8s.io/tasks/experimental-features/machineset-preflight-checks).
This is necessary as core CAPI 1.9 introduces a feature gate change, setting MachineSetPreflightChecks=true, which in turn relies on the presence of spec.Version and status.Version on the GCPManagedControlPlane object.
These fields will be deprecated in a future release.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 25, 2025
@k8s-ci-robot k8s-ci-robot requested review from damdo and dims February 25, 2025 10:09
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 25, 2025
@netlify
Copy link

netlify bot commented Feb 25, 2025

Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

Name Link
🔨 Latest commit af6413d
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-gcp/deploys/67c815d20809c5000860a17f
😎 Deploy Preview https://deploy-preview-1434--kubernetes-sigs-cluster-api-gcp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch from bf2c4da to 153f13f Compare February 25, 2025 10:16
Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @salasberryfin
Thanks a lot for working on implementing this!

Mostly LGTM, left a couple of things we need to change though.

@damdo
Copy link
Member

damdo commented Feb 25, 2025

@salasberryfin Also we will likely need to add an action required in the release-note for this PR,
similar to the one I added for CAPA here: kubernetes-sigs/cluster-api-provider-aws#5209 (comment)
(you should be able to mostly copy-paste it).

cc. @richardcase

@damdo
Copy link
Member

damdo commented Feb 26, 2025

@salasberryfin also we might need to silence the linter for errors like : cloud/services/container/clusters/reconcile.go:515:5: SA1019: spec.ControlPlaneVersion is deprecated: This field will soon be removed and you are expected to use Version instead. (staticcheck), as we still want to set/get those fields internally even if they are deprecated, until we remove them.

Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @salasberryfin 🎉

Just a couple of minor comments.

@salasberryfin
Copy link
Contributor Author

Thanks for the reviews @damdo @richardcase. Sorry I couldn't address your comments before, I'll be doing so today!

@damdo
Copy link
Member

damdo commented Feb 28, 2025

No worries, thanks @salasberryfin

@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch 2 times, most recently from ec845dc to 80a52a3 Compare March 3, 2025 10:52
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 3, 2025
@salasberryfin
Copy link
Contributor Author

I think the latest commit addresses all of the comments @damdo @richardcase.

@cpanato
Copy link
Member

cpanato commented Mar 3, 2025

@salasberryfin there are a few lints to tackle, thanks for working on this

@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch from 80a52a3 to d5880ec Compare March 3, 2025 11:00
@salasberryfin
Copy link
Contributor Author

Thanks @cpanato. The exclude rule I added covered some of the cases where the linter found errors but missed some of them. Should be fixed now.

ControlPlaneVersion: &vV1_27_1,
EnableAutopilot: true,
ClusterName: "cluster1_autopilot",
Version: &vV1_27_1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be this updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update this in a separate PR, if that's okay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 3, 2025
@salasberryfin
Copy link
Contributor Author

salasberryfin commented Mar 3, 2025

Also added the action required to the release note. Thanks @damdo!

Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2025
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

/approve

/hold for @richardcase @damdo

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demicev, cpanato, salasberryfin

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2025
Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@salasberryfin Mostly LGTM, I left one blocking question though. LMK thanks!

@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch from d5880ec to 7ec8133 Compare March 5, 2025 08:55
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2025
@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch from 7ec8133 to 744baff Compare March 5, 2025 09:00
@salasberryfin
Copy link
Contributor Author

Thanks for the reviews. The webhook now returns a warning if only spec.controlPlaneVersion is used and an error if both spec.controlPlaneVersion and spec.Version are set. New tests have been added to verify the validation webhook on these scenarios.

@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch 2 times, most recently from cda1f3c to bb361dd Compare March 5, 2025 09:06
@salasberryfin salasberryfin force-pushed the managed-control-plane-version-api-change branch from bb361dd to af6413d Compare March 5, 2025 09:13
@damdo
Copy link
Member

damdo commented Mar 5, 2025

Hm looks like the E2Es failed https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-gcp/1434/pull-cluster-api-provider-gcp-e2e-test/1897214121795915776

Let's retest for good measure in case it is a flake, otherwise we might need to look into it.

/retest

@salasberryfin
Copy link
Contributor Author

Thanks @damdo, looks like it was a flake.

Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks @salasberryfin for working on this

/unhold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 5, 2025
@k8s-ci-robot k8s-ci-robot merged commit 2f1320c into kubernetes-sigs:main Mar 5, 2025
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.1.0 milestone Mar 5, 2025
@richardcase
Copy link
Member

Great work @salasberryfin 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align managed control plane to latest CAPI changes

6 participants