Support more configurations for GCPManagedMachinePool #1026

gzcharleszhang · 2023-09-22T18:35:37Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Currently, GCPManagedMachinePool only supports a limited set of configurations. This PR adds support for a wider set of configurations made available by the GCP SDK. Specifically, it adds support for the following fields:

- Config
  + MachineType
  + DiskSizeGb
  + ServiceAccount
  + ImageType
  + Labels
  + LocalSsdCount
  + Tags
  + DiskType
  + Taints
  + ResourceLabels
- Autoscaling
  + Enabled
  + LocationPolicy
  + TotalMinNodeCount
  + TotalMaxNodeCount
- Management
  + AutoUpgrade
  + AutoRepair
- MaxPodsConstraint
  + MaxPodsPerNode

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1025

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

GCPManagedMachinePool now supports more configurations made available by https://pkg.go.dev/cloud.google.com/go/container/apiv1/containerpb#NodePool
- Config
  + MachineType
  + DiskSizeGb
  + ServiceAccount
  + ImageType
  + Labels
  + LocalSsdCount
  + Tags
  + DiskType
  + Taints
  + ResourceLabels
- Autoscaling
  + Enabled
  + LocationPolicy
  + TotalMinNodeCount
  + TotalMaxNodeCount
- Management
  + AutoUpgrade
  + AutoRepair
- MaxPodsConstraint
  + MaxPodsPerNode

linux-foundation-easycla · 2023-09-22T18:35:40Z

The committers listed above are authorized under a signed CLA.

✅ login: gzcharleszhang / name: Charles Zhang (627fc41)

k8s-ci-robot · 2023-09-22T18:35:45Z

Welcome @gzcharleszhang!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-gcp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-gcp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-09-22T18:35:46Z

Hi @gzcharleszhang. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

cpanato

/ok-to-test

richardcase · 2023-10-26T11:14:59Z

Until #944 merges:

/hold

This is making changes to the same API kinds. @gzcharleszhang & @akshay196-rafay have you synced up?

gzcharleszhang · 2023-10-26T22:31:17Z

@richardcase I think I'll rebase after #944 merges. My PR will introduce a few more additional fields.

richardcase · 2023-10-27T11:08:13Z

@richardcase I think I'll rebase after #944 merges. My PR will introduce a few more additional fields.

Great, thanks @gzcharleszhang . I've approved #944, we just need another reviewer and someone to lgtm.

richardcase · 2023-11-21T20:28:30Z

@gzcharleszhang - now that #944 has merged would you be able to rebase and then we can aim to get this merged.

gzcharleszhang · 2023-12-06T15:23:40Z

@richardcase I have rebased with the latest changes, could you help take a look? Thanks!

richardcase · 2023-12-14T07:17:05Z

cloud/scope/managedmachinepool.go

+// NodePoolResourceLabels returns the resource labels of the node pool.
+func NodePoolResourceLabels(additionalLabels infrav1.Labels, clusterName string) infrav1.Labels {
+	resourceLabels := additionalLabels.DeepCopy()
+	if resourceLabels == nil {


Should we check for nil before you do the deepcopy?

richardcase · 2023-12-14T07:19:52Z

cloud/services/container/nodepools/reconcile.go

+// setReadyStatusFromConditions updates the GCPManagedMachinePool's ready status based on its conditions.
+func (s *Service) setReadyStatusFromConditions() {
+	machinePool := s.scope.GCPManagedMachinePool
+	if conditions.IsTrue(machinePool, clusterv1.ReadyCondition) || conditions.IsTrue(machinePool, infrav1exp.GKEMachinePoolUpdatingCondition) {


Where we set the ready condition to true we can also set the Status.Ready.

this is what this helper function does, if the ready condition is true then we set the ready status to true

🤣 I understand what the helper function is doing, my comment was more around not using this function and instead setting status ready at the same time you set the condition to true.

This function is defereed and also i'm not a fan on logic gitting off of conditions.

This is more a personal/stylistic preference so lets not block on this.

I see. The reason why I refactored it this way is because there were some places where we forgot to set the ready status when setting the ready condition. I thought this would make it more reliable so future contributors don't have to worry about setting both at the same time.

richardcase · 2023-12-14T07:21:45Z

cloud/services/container/nodepools/reconcile.go


 	setNodePoolAutoscalingRequest := containerpb.SetNodePoolAutoscalingRequest{
 		Name: s.scope.NodePoolFullName(),
 	}
-	if !reflect.DeepEqual(desiredAutoscaling, existingAutoscaling) {
+	if !reflect.DeepEqual(desiredAutoscaling, existingNodePool.Autoscaling) {


Can we change this to cmp.Equal

richardcase · 2023-12-15T08:56:17Z

GKE and machinepools are still experimental so:

/override pull-cluster-api-provider-gcp-apidiff

k8s-ci-robot · 2023-12-15T08:56:21Z

@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-gcp-apidiff

In response to this:

GKE and machinepools are still experimental so:

/override pull-cluster-api-provider-gcp-apidiff

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.

richardcase · 2023-12-15T08:56:30Z

/lgtm
/approved

richardcase · 2023-12-15T08:56:48Z

Thanks for this @gzcharleszhang 👍

richardcase · 2023-12-15T09:01:49Z

/unhold

richardcase · 2023-12-15T09:02:13Z

/approve

k8s-ci-robot · 2023-12-15T09:02:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gzcharleszhang, richardcase

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

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 22, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 22, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 22, 2023

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

k8s-ci-robot requested review from cpanato and richardcase September 22, 2023 18:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 22, 2023

gzcharleszhang closed this Sep 22, 2023

gzcharleszhang force-pushed the gcp-managed-machine-pool branch from e997c3b to 44bd891 Compare September 22, 2023 19:04

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 22, 2023

gzcharleszhang reopened this Sep 22, 2023

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 22, 2023

gzcharleszhang force-pushed the gcp-managed-machine-pool branch from f9cb96f to f866eb0 Compare September 22, 2023 20:37

cpanato reviewed Sep 23, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 23, 2023

gzcharleszhang force-pushed the gcp-managed-machine-pool branch 2 times, most recently from 3589b33 to adf7f2b Compare September 26, 2023 18:59

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2023

gzcharleszhang force-pushed the gcp-managed-machine-pool branch from adf7f2b to 3ca0f69 Compare December 6, 2023 13:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2023

gzcharleszhang force-pushed the gcp-managed-machine-pool branch 2 times, most recently from 93d7d5d to 776d542 Compare December 6, 2023 15:19

gzcharleszhang force-pushed the gcp-managed-machine-pool branch from 776d542 to 03a2e20 Compare December 6, 2023 15:49

richardcase reviewed Dec 14, 2023

View reviewed changes

richardcase mentioned this pull request Dec 14, 2023

Add nodeManagement field to GCPManagedMachinePool #1086

Closed

3 tasks

add more configurations to gcp managed machine pool

627fc41

gzcharleszhang force-pushed the gcp-managed-machine-pool branch from 03a2e20 to 627fc41 Compare December 14, 2023 23:10

gzcharleszhang requested a review from richardcase December 14, 2023 23:10

k8s-ci-robot assigned richardcase Dec 15, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 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 Dec 15, 2023

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

k8s-ci-robot merged commit cadb9b9 into kubernetes-sigs:main Dec 15, 2023
13 checks passed

k8s-ci-robot added this to the v1.1.0 milestone Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support more configurations for GCPManagedMachinePool #1026

Support more configurations for GCPManagedMachinePool #1026

gzcharleszhang commented Sep 22, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 22, 2023 •

edited

Loading

k8s-ci-robot commented Sep 22, 2023

k8s-ci-robot commented Sep 22, 2023

cpanato left a comment

richardcase commented Oct 26, 2023

gzcharleszhang commented Oct 26, 2023

richardcase commented Oct 27, 2023

richardcase commented Nov 21, 2023

gzcharleszhang commented Dec 6, 2023

richardcase Dec 14, 2023

richardcase Dec 14, 2023

gzcharleszhang Dec 14, 2023

richardcase Dec 15, 2023 •

edited

Loading

richardcase Dec 15, 2023

gzcharleszhang Dec 15, 2023

richardcase Dec 14, 2023

richardcase commented Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023

richardcase commented Dec 15, 2023

richardcase commented Dec 15, 2023

richardcase commented Dec 15, 2023

richardcase commented Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023

Support more configurations for GCPManagedMachinePool #1026

Support more configurations for GCPManagedMachinePool #1026

Conversation

gzcharleszhang commented Sep 22, 2023 • edited Loading

linux-foundation-easycla bot commented Sep 22, 2023 • edited Loading

k8s-ci-robot commented Sep 22, 2023

k8s-ci-robot commented Sep 22, 2023

cpanato left a comment

Choose a reason for hiding this comment

richardcase commented Oct 26, 2023

gzcharleszhang commented Oct 26, 2023

richardcase commented Oct 27, 2023

richardcase commented Nov 21, 2023

gzcharleszhang commented Dec 6, 2023

richardcase Dec 14, 2023

Choose a reason for hiding this comment

richardcase Dec 14, 2023

Choose a reason for hiding this comment

gzcharleszhang Dec 14, 2023

Choose a reason for hiding this comment

richardcase Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

richardcase Dec 15, 2023

Choose a reason for hiding this comment

gzcharleszhang Dec 15, 2023

Choose a reason for hiding this comment

richardcase Dec 14, 2023

Choose a reason for hiding this comment

richardcase commented Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023

richardcase commented Dec 15, 2023

richardcase commented Dec 15, 2023

richardcase commented Dec 15, 2023

richardcase commented Dec 15, 2023

k8s-ci-robot commented Dec 15, 2023

gzcharleszhang commented Sep 22, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 22, 2023 •

edited

Loading

richardcase Dec 15, 2023 •

edited

Loading