Add additional fields to GCPManagedMachinePool #944

akshay196-rafay · 2023-06-06T18:13:15Z

What type of PR is this?

/kind api-change

What this PR does / why we need it:

Support additional fields to GCPManagedMachinePool kind.

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

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

Release note:

Support additional fields to GCPManagedMachinePool type.

linux-foundation-easycla · 2023-06-06T18:13:21Z

The committers listed above are authorized under a signed CLA.

✅ login: akshay196-rafay / name: Akshay Gaikwad (a3bbeb3)

k8s-ci-robot · 2023-06-06T18:13:25Z

Welcome @akshay196-rafay!

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-06-06T18:13:26Z

Hi @akshay196-rafay. 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.

richardcase · 2023-06-20T06:52:13Z

/ok-to-test

akshay196-rafay · 2023-08-16T17:42:19Z

@richardcase This PR is now ready for review.

richardcase · 2023-08-17T09:38:53Z

@akshay196-rafay - i think we should perhaps re-think the approach going forward. So that we split the aditions up into smaller PRs where each contains:

A small number of API additions
Reconciliation changes for the new API fields
Updates to the e2e

I appreciate this is following the original approach for the GKE implementation but i think it may be better for to do this incrementally.

wdyt?

akshay196-rafay · 2023-08-17T11:59:49Z

i think we should perhaps re-think the approach going forward. So that we split the aditions up into smaller PRs where each contains:

A small number of API additions
Reconciliation changes for the new API fields
Updates to the e2e

I appreciate this is following the original approach for the GKE implementation but i think it may be better for to do this incrementally.

I understand where you're coming from. Since GCPManagedMachinePool's new fields are comparatively less I can continue working on this same PR to add reconciliation changes, validation webhooks and e2e of GCPManagedMachinePool.

For the GCPManagedControlPlane PR raised by @aniruddha2000 we can take the incremental approach something like this -

Steps 1
Add new API fields - Description, ClusterNetwork, DefaultNodeLocation, DefaultMaxPodsPerNode
Add reconciliation and webhook for same
Update e2e for these fields

Steps 2
Add new API fields - ClusterSecurity, AddonsConfig, LoggingConfig, MonitoringConfig
Add reconciliation and webhook for same
Update e2e for these fields

richardcase · 2023-08-22T09:55:25Z

@akshay196-rafay - sounds good to me 👍 Ping me when the reconciliation for the new fields has been added. Thank you for taking this work on.

akshay196-rafay · 2023-10-04T11:28:40Z

@richardcase @gzcharleszhang Addressed your review comments.

gzcharleszhang

1 nit comment otherwise LGTM. I'll wait for @richardcase and @cpanato to take a look/approve.

gzcharleszhang · 2023-10-04T18:55:12Z

cloud/services/container/nodepools/reconcile.go

-		err = s.updateNodePoolVersionOrImage(ctx, nodePoolUpdateVersionOrImage)
+	needUpdateNodePool, nodePoolUpdateNodePool := s.checkDiffAndPrepareUpdateNodePool(nodePool)
+	if needUpdateNodePool {
+		log.Info("Node pool version/labels/taints/locations/image type/network tags update required")


I think this can just be something like Node pool config update required, we may update other fields here as well.

richardcase

Thanks @akshay196-rafay , this looks good to me. Just one small nit.

richardcase · 2023-10-26T10:57:58Z

cloud/services/container/nodepools/reconcile.go

@@ -354,6 +354,27 @@ func (s *Service) checkDiffAndPrepareUpdateVersionOrImage(existingNodePool *cont
 			Taints: desiredKubernetesTaints,
 		}
 	}
+	// Locations
+	desiredLocations := s.scope.GCPManagedMachinePool.Spec.NodeLocations
+	if !reflect.DeepEqual(desiredLocations, existingNodePool.Locations) {


Can we change this to use cmp.Equals?

(and also the other instances below)

Signed-off-by: Akshay Gaikwad <[email protected]>

richardcase · 2023-10-27T11:05:16Z

Thanks @akshay196-rafay. From my side:

/approve

What do you think @gzcharleszhang or @richardchen331 ?

richardcase · 2023-11-02T12:41:07Z

@cpanato - in case you have some spare cycles to have a look and lgtm if you think its ok.

gzcharleszhang

LGTM

k8s-ci-robot · 2023-11-04T07:12:28Z

@gzcharleszhang: changing LGTM is restricted to collaborators

In response to this:

LGTM

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.

richardchen331 · 2023-11-04T14:38:47Z

Thanks @akshay196-rafay. From my side:

/approve

What do you think @gzcharleszhang or @richardchen331 ?

LGTM. Thanks for the contribution @akshay196-rafay!

k8s-ci-robot · 2023-11-04T14:39:01Z

@richardchen331: changing LGTM is restricted to collaborators

In response to this:

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

thanks so much and sorry for the delay

/lgtm

k8s-ci-robot · 2023-11-05T11:17:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akshay196-rafay, cpanato, gzcharleszhang, richardcase, richardchen331

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

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

akshay196-rafay · 2023-11-05T13:08:06Z

Thank you @richardcase, @gzcharleszhang, @richardchen331 and @cpanato for reviewing and merging this.

k8s-ci-robot requested a review from cpanato June 6, 2023 18:13

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

k8s-ci-robot requested a review from dims June 6, 2023 18:13

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 Jun 6, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2023

akshay196 mentioned this pull request Jun 6, 2023

Support additional fields for GKE #906

Closed

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 Jun 20, 2023

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 Aug 16, 2023

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch from 9228eed to acef7b1 Compare August 16, 2023 17:36

akshay196-rafay marked this pull request as ready for review August 16, 2023 17:36

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 Aug 16, 2023

k8s-ci-robot requested a review from richardcase August 16, 2023 17:36

akshay196-rafay changed the title ~~Add multiple new fields to GCPManagedMachinePool~~ Add additional fields to GCPManagedMachinePool Aug 16, 2023

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch from acef7b1 to 11b6d09 Compare August 25, 2023 11:57

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch from 11b6d09 to cd97dfa Compare September 1, 2023 14:57

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

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch from cd97dfa to b92aa31 Compare September 7, 2023 09:44

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch 4 times, most recently from ff9b4f3 to d3425ba Compare October 4, 2023 11:15

akshay196-rafay requested review from richardcase and gzcharleszhang October 4, 2023 11:28

gzcharleszhang reviewed Oct 4, 2023

View reviewed changes

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch from d3425ba to 86f466b Compare October 5, 2023 10:21

richardcase reviewed Oct 26, 2023

View reviewed changes

Add multiple fields to GCPManagedMachinePool

a3bbeb3

Signed-off-by: Akshay Gaikwad <[email protected]>

akshay196-rafay force-pushed the gcpmanagedmachinepool-additional-fields branch from 86f466b to a3bbeb3 Compare October 26, 2023 11:12

richardcase mentioned this pull request Oct 26, 2023

Support more configurations for GCPManagedMachinePool #1026

Merged

3 tasks

akshay196-rafay requested a review from richardcase October 27, 2023 03:17

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

gzcharleszhang approved these changes Nov 4, 2023

View reviewed changes

richardchen331 approved these changes Nov 4, 2023

View reviewed changes

cpanato approved these changes Nov 5, 2023

View reviewed changes

k8s-ci-robot assigned cpanato Nov 5, 2023

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

k8s-ci-robot merged commit d4689a3 into kubernetes-sigs:main Nov 5, 2023
13 checks passed

k8s-ci-robot added this to the v1.1.0 milestone Nov 5, 2023

akshay196-rafay deleted the gcpmanagedmachinepool-additional-fields branch November 5, 2023 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional fields to GCPManagedMachinePool #944

Add additional fields to GCPManagedMachinePool #944

akshay196-rafay commented Jun 6, 2023

linux-foundation-easycla bot commented Jun 6, 2023 •

edited

Loading

k8s-ci-robot commented Jun 6, 2023

k8s-ci-robot commented Jun 6, 2023

richardcase commented Jun 20, 2023

akshay196-rafay commented Aug 16, 2023

richardcase commented Aug 17, 2023

akshay196-rafay commented Aug 17, 2023

richardcase commented Aug 22, 2023 •

edited

Loading

akshay196-rafay commented Oct 4, 2023

gzcharleszhang left a comment •

edited

Loading

gzcharleszhang Oct 4, 2023

akshay196-rafay Oct 5, 2023

richardcase left a comment

richardcase Oct 26, 2023

akshay196-rafay Oct 27, 2023

richardcase commented Oct 27, 2023

richardcase commented Nov 2, 2023

gzcharleszhang left a comment

k8s-ci-robot commented Nov 4, 2023

richardchen331 commented Nov 4, 2023

k8s-ci-robot commented Nov 4, 2023

cpanato left a comment

k8s-ci-robot commented Nov 5, 2023

akshay196-rafay commented Nov 5, 2023

Add additional fields to GCPManagedMachinePool #944

Add additional fields to GCPManagedMachinePool #944

Conversation

akshay196-rafay commented Jun 6, 2023

linux-foundation-easycla bot commented Jun 6, 2023 • edited Loading

k8s-ci-robot commented Jun 6, 2023

k8s-ci-robot commented Jun 6, 2023

richardcase commented Jun 20, 2023

akshay196-rafay commented Aug 16, 2023

richardcase commented Aug 17, 2023

akshay196-rafay commented Aug 17, 2023

richardcase commented Aug 22, 2023 • edited Loading

akshay196-rafay commented Oct 4, 2023

gzcharleszhang left a comment • edited Loading

Choose a reason for hiding this comment

gzcharleszhang Oct 4, 2023

Choose a reason for hiding this comment

akshay196-rafay Oct 5, 2023

Choose a reason for hiding this comment

richardcase left a comment

Choose a reason for hiding this comment

richardcase Oct 26, 2023

Choose a reason for hiding this comment

akshay196-rafay Oct 27, 2023

Choose a reason for hiding this comment

richardcase commented Oct 27, 2023

richardcase commented Nov 2, 2023

gzcharleszhang left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 4, 2023

richardchen331 commented Nov 4, 2023

k8s-ci-robot commented Nov 4, 2023

cpanato left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 5, 2023

akshay196-rafay commented Nov 5, 2023

linux-foundation-easycla bot commented Jun 6, 2023 •

edited

Loading

richardcase commented Aug 22, 2023 •

edited

Loading

gzcharleszhang left a comment •

edited

Loading