Allow configuration of API server instance group name #1207

bfournie · 2024-04-11T13:27:03Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Provides the ability to override the API server instance group name to allow compatibility when using the Openshift Machine API.

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 #1194

Special notes for your reviewer:

Note that a new LoadBalancerSpec field has been added to GCPClusterSpec. Although this only contains one field it will be populated in the future with additional fields in order to support a LoadBalancer for private clusters.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

A new LoadBalancerSpec field containing the APIServerInstanceGroupTagOverride has been added to GCPClusterSpec.

k8s-ci-robot · 2024-04-11T13:27:13Z

Hi @bfournie. 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.

netlify · 2024-04-11T13:27:21Z

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

Name	Link
🔨 Latest commit	`318600d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-gcp/deploys/662257e9fc1b39000838309d
😎 Deploy Preview	https://deploy-preview-1207--kubernetes-sigs-cluster-api-gcp.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 configuration.

api/v1beta1/gcpcluster_types.go

api/v1beta1/types.go

cpanato

/ok-to-test

cloud/services/compute/loadbalancers/reconcile_test.go

nrb · 2024-04-16T17:57:00Z

cloud/services/compute/loadbalancers/reconcile_test.go

+			name: "instanceGroup name is overridden (should create instanceGroup)",
+			scope: func() Scope {
+				tagOverride := "master"
+				clusterScope.GCPCluster.Spec.LoadBalancer = infrav1.LoadBalancerSpec{


Isn't this changing a variable in a shared scope? If the tests are run in a random order, couldn't this set clusterScope.GCPCluster.Spec.LoadBalancer for the rest of the suite?

Good catch. Changed to use a separate scope to avoid overwriting.

Thanks! That'll work.

Another pattern I've seen in tests for other providers is for the function to take in the "golden" one as an argument and return a new copy with the modifications.

Something like

scope: func(s Scope) Scope { s.GCPCluster.Spec.LoadBalancer = infrav1.LoadBalancerSpec{....} return s }

and

scope: func(s Scope) Scope { return s }

That way, you get to have the top-level one remain clean and passed in on each invocation of test.scope to get a fresh, mutable copy.

Up to you if you want to use that here or not.

Good suggestion. I've reworked it to make the mods only in the test which saves some duplication.

exp/api/v1beta1/gcpmanagedcluster_types.go

api/v1beta1/types.go

cloud/scope/cluster.go

patrickdillon · 2024-04-18T20:22:58Z

cloud/scope/machine.go

+	tag := infrav1.APIServerRoleTagValue
+	if m.ClusterGetter.LoadBalancer().APIServerInstanceGroupTagOverride != nil {
+		tag = *m.ClusterGetter.LoadBalancer().APIServerInstanceGroupTagOverride
+	}


same nit/suggestion to consider using ptr.Deref

api/v1beta1/types.go

bfournie · 2024-04-18T21:03:59Z

/retest

richardcase

This looks good to me, thanks @bfournie . There is one nit.

As the API diff is related to the exported go packages and not the API definition:

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

/approve

api/v1beta1/types.go

k8s-ci-robot · 2024-04-19T09:38:17Z

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

In response to this:

This looks good to me, thanks @bfournie . There is one nit.

As the API diff is related to the exported go packages and not the API definition:

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

/approve

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.

k8s-ci-robot · 2024-04-19T09:38:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bfournie, 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

Provides the ability to override the API server instance group name to allow compatibility when using the Openshift Machine API.

k8s-ci-robot · 2024-04-19T11:42:48Z

@bfournie: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-gcp-apidiff	`318600d`	link	false	`/test pull-cluster-api-provider-gcp-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

damdo

/lgtm

Thanks @bfournie !

damdo · 2024-04-19T13:54:14Z

/tide refresh

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 11, 2024

k8s-ci-robot requested review from cpanato and richardcase April 11, 2024 13:27

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 11, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 11, 2024

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 11, 2024

barbacbd reviewed Apr 11, 2024

View reviewed changes

api/v1beta1/gcpcluster_types.go Outdated Show resolved Hide resolved

barbacbd reviewed Apr 11, 2024

View reviewed changes

api/v1beta1/types.go Show resolved Hide resolved

cpanato requested changes Apr 11, 2024

View reviewed changes

cloud/services/compute/loadbalancers/reconcile_test.go Outdated Show resolved Hide resolved

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 Apr 11, 2024

k8s-ci-robot assigned cpanato Apr 11, 2024

bfournie force-pushed the api-server-name-override branch from f05d645 to 141c227 Compare April 14, 2024 20:57

nrb reviewed Apr 16, 2024

View reviewed changes

bfournie force-pushed the api-server-name-override branch from 141c227 to af5af05 Compare April 16, 2024 18:35

damdo reviewed Apr 17, 2024

View reviewed changes

exp/api/v1beta1/gcpmanagedcluster_types.go Show resolved Hide resolved

bfournie force-pushed the api-server-name-override branch from af5af05 to be92b73 Compare April 17, 2024 11:59

patrickdillon reviewed Apr 17, 2024

View reviewed changes

api/v1beta1/types.go Show resolved Hide resolved

patrickdillon reviewed Apr 18, 2024

View reviewed changes

nrb reviewed Apr 18, 2024

View reviewed changes

api/v1beta1/types.go Outdated Show resolved Hide resolved

bfournie force-pushed the api-server-name-override branch 3 times, most recently from 2c5eab7 to 116f171 Compare April 18, 2024 20:57

bfournie force-pushed the api-server-name-override branch from 116f171 to f740f56 Compare April 18, 2024 21:19

richardcase reviewed Apr 19, 2024

View reviewed changes

api/v1beta1/types.go Outdated Show resolved Hide resolved

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

bfournie force-pushed the api-server-name-override branch from f740f56 to 42a9f03 Compare April 19, 2024 11:21

Allow configuration of API server instance group name

318600d

Provides the ability to override the API server instance group name to allow compatibility when using the Openshift Machine API.

bfournie force-pushed the api-server-name-override branch from 42a9f03 to 318600d Compare April 19, 2024 11:39

damdo reviewed Apr 19, 2024

View reviewed changes

k8s-ci-robot assigned damdo Apr 19, 2024

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

k8s-ci-robot merged commit d0694bd into kubernetes-sigs:main Apr 19, 2024

k8s-ci-robot added this to the v1.1.0 milestone Apr 19, 2024

bfournie deleted the api-server-name-override branch April 19, 2024 14:00

damdo mentioned this pull request Apr 19, 2024

build(deps): Bump the all group with 4 updates #1208

Merged

Allow configuration of API server instance group name #1207

Allow configuration of API server instance group name #1207

Uh oh!

Conversation

bfournie commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 11, 2024

Uh oh!

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

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrb Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

bfournie Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

nrb Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

bfournie Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickdillon Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bfournie commented Apr 18, 2024

Uh oh!

richardcase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 19, 2024

Uh oh!

k8s-ci-robot commented Apr 19, 2024

Uh oh!

k8s-ci-robot commented Apr 19, 2024

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

damdo commented Apr 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bfournie commented Apr 11, 2024 •

edited

Loading

netlify bot commented Apr 11, 2024 •

edited

Loading