chore: refactor InstanceAdditionalDiskSpec for reuse in MachinePool #1507

justinsb · 2025-08-16T19:40:29Z


NONE

netlify · 2025-08-16T19:40:37Z

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

Name	Link
🔨 Latest commit	`e6b73ee`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-cluster-api-gcp/deploys/68ba3e6c488256000897fa3b
😎 Deploy Preview	https://deploy-preview-1507--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 project configuration.

justinsb · 2025-08-16T19:45:47Z

This will avoid code duplication in MachinePool (#1506)

An alternative approach to the long-list of parameters would be to define a struct, which would effectively give us named arguments, so something like this:

additionalDisks :=  instanceAdditionalDiskSpec(ctx, instanceAdditionalDiskSpecOptions{
  AdditionalDisks: m.GCPMachine.Spec.AdditionalDisks,
  RootDiskEncryptionKey: m.GCPMachine.Spec.RootDiskEncryptionKey,
  Zone: m.Zone(), 
  ResourceManagerTags: m.ResourceManagerTags()),
})

I think that's not worth it because the arguments are already relatively self documenting, but happy to switch if people would prefer that.

justinsb · 2025-08-23T15:48:59Z

Should I tweak apidiff to ignore our internal functions? I consider cluster-api-provider-gcp as a leaf, so I don't think we really want apidiff except on our CRDs?

justinsb · 2025-08-23T16:04:05Z

/retest

k8s-ci-robot · 2025-08-23T16:08:10Z

@justinsb: 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	`6ad6bb0`	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-sigs/prow repository. I understand the commands that are listed here.

bochengchu · 2025-08-27T16:39:14Z

/lgtm
/hold for second look from approvers

k8s-ci-robot · 2025-08-27T16:39:19Z

@bochengchu: changing LGTM is restricted to collaborators

In response to this:

/lgtm
/hold for second look from approvers

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-sigs/prow repository.

cpanato · 2025-08-27T18:20:32Z

cloud/scope/machine_test.go

@@ -12,6 +13,8 @@ import (
 // This test verifies that if a user selects "local-ssd"
 // as a disk type then the MachineScope correctly detects it as such.
 func TestMachineLocalSSDDiskType(t *testing.T) {
+	ctx := context.Background()


should we use the T.Context() here?

Oh wow! I have been wanting this for a long time. I had missed that it was introduced - but it was introduced in 1.24.

I think we're pinned to go 1.23 in go.mod, but it gets auto-bumped if we move to CAPI 1.11 (#1509)

We can also bump the go toolchain in go.mod - do we want to do that?

(I totally agree that when we bump to go 1.24 we should use T.Context here and probably in many other places!)

Agree let's defer that after we get CAPI 1.11 and go 1.24 in

cpanato

A clarification question: Is there any impact on the existing running clusters? When the user upgrades to a version that has this change

justinsb · 2025-08-28T17:53:48Z

A clarification question: Is there any impact on the existing running clusters? When the user upgrades to a version that has this change

Not sure if you mean this PR - this PR is a mechanical function extraction to make #1506 a little shorter and easier to review. It should behave 100% identically unless I screwed up (which is not unlikely, but my intent is no observable external change)

If you mean #1509 - the CAPI upgrade - I am indeed trying to do that without breaking changes. AIUI capi 1.11 does not remove v1beta1, but adds v1beta2. We are encouraged to move our own APIs to v1beta2 conventions, but I have not done that yet. The intent of #1509 is just to add support for the new libraries / new versions. But that upgrade is much riskier so I would like to follow whatever branching policy we're using before merging that one.

damdo · 2025-08-29T08:02:10Z

If you mean #1509 - the CAPI upgrade - I am indeed trying to do that without breaking changes. AIUI capi 1.11 does not remove v1beta1, but adds v1beta2. We are encouraged to move our own APIs to v1beta2 conventions, but I have not done that yet. The intent of #1509 is just to add support for the new libraries / new versions. But that upgrade is much riskier so I would like to follow whatever branching policy we're using before merging that one.

I agree, I think at first we can bump to CAPI v1.11 and keep us on v1beta1, and then consider jumping on v1beta2 as a follow up or in a follow up minor release.

justinsb · 2025-09-04T13:01:43Z

Trying to track down why that apidiff job is hanging ... hopefully it is just a networking flake pulling go dependencies, so I have proposed a timeout in kubernetes/test-infra#35469 (and thank you @damdo for approving that!). But of course I did touch this recently, so it may need a deeper fix ... TBD

Assuming I can get that test passing, I think that this one should not block on the CAPI update (and thus nor should it block on T.Context), because it is in support of MachinePool, not the CAPI upgrade. I am assuming we will support MachinePool before CAPI 1.11, because they are logically separate, but happy to discuss that assumption: I'd rather get MachinePool in sooner, but it also looks like we can bump to CAPI 1.11 without too much trouble (I'm working on that PR next!)

This is preparing for reuse in MachinePool.

damdo

So this is a pre-factor right?

/approve
/lgtm

/hold
Feel free to unhold when ready to merge it

damdo · 2025-09-05T08:35:48Z

cloud/scope/machine_test.go

@@ -12,6 +13,8 @@ import (
 // This test verifies that if a user selects "local-ssd"
 // as a disk type then the MachineScope correctly detects it as such.
 func TestMachineLocalSSDDiskType(t *testing.T) {
+	ctx := context.Background()


Agree let's defer that after we get CAPI 1.11 and go 1.24 in

k8s-ci-robot · 2025-09-05T08:56:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damdo, justinsb

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~~ [damdo,justinsb]

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

justinsb · 2025-09-05T14:04:25Z

So this is a pre-factor right?

Exactly! I can continue to do these to break down #1506 to make it smaller as needed. I love "pre-factor", I hadn't heard that and will start using that term...

I think it's ready!

/hold cancel

damdo · 2025-09-05T14:17:36Z

Exactly! I can continue to do these to break down #1506 to make it smaller as needed. I love "pre-factor", I hadn't heard that and will start using that term...

@justinsb 🙌 haha nice

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 16, 2025

k8s-ci-robot requested review from cpanato and dims August 16, 2025 19:40

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 16, 2025

justinsb force-pushed the setup_dedup_InstanceAdditionalDiskSpec branch from dd13c9c to 6ad6bb0 Compare August 16, 2025 19:41

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Aug 23, 2025

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

cpanato reviewed Aug 27, 2025

View reviewed changes

justinsb mentioned this pull request Aug 28, 2025

Bump go version to 1.24 and replace context.Background() with t.Context() #1514

Open

3 tasks

justinsb force-pushed the setup_dedup_InstanceAdditionalDiskSpec branch from 6ad6bb0 to 43876e5 Compare September 3, 2025 14:13

justinsb mentioned this pull request Sep 4, 2025

Add 3h timeouts to CAPG jobs kubernetes/test-infra#35469

Merged

chore: refactor InstanceAdditionalDiskSpec for reuse in MachinePool

e6b73ee

This is preparing for reuse in MachinePool.

justinsb force-pushed the setup_dedup_InstanceAdditionalDiskSpec branch from 43876e5 to e6b73ee Compare September 5, 2025 01:35

damdo reviewed Sep 5, 2025

View reviewed changes

k8s-ci-robot assigned damdo Sep 5, 2025

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

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

k8s-ci-robot merged commit a71f33c into kubernetes-sigs:main Sep 5, 2025
15 of 16 checks passed

chore: refactor InstanceAdditionalDiskSpec for reuse in MachinePool #1507

chore: refactor InstanceAdditionalDiskSpec for reuse in MachinePool #1507

Conversation

justinsb commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Aug 16, 2025 • 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!

justinsb commented Aug 16, 2025

Uh oh!

justinsb commented Aug 23, 2025

Uh oh!

justinsb commented Aug 23, 2025

Uh oh!

k8s-ci-robot commented Aug 23, 2025

Uh oh!

bochengchu commented Aug 27, 2025

Uh oh!

k8s-ci-robot commented Aug 27, 2025

Uh oh!

cpanato Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

justinsb Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

damdo Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

justinsb commented Aug 28, 2025

Uh oh!

damdo commented Aug 29, 2025

Uh oh!

justinsb commented Sep 4, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

damdo Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 5, 2025

Uh oh!

justinsb commented Sep 5, 2025

Uh oh!

damdo commented Sep 5, 2025

Uh oh!

Uh oh!

Uh oh!

justinsb commented Aug 16, 2025 •

edited

Loading

netlify bot commented Aug 16, 2025 •

edited

Loading