✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

EmilienM · 2023-09-29T19:13:15Z

Add ephemeral storage support to the AdditionalBlockDevices

What this PR does / why we need it:

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

Special notes for your reviewer:

We only support unformated disks for now (we don't expose guestFormat) so the user can handle the filesystem themselves. Note that we initially added it and tested swap but it didn't work in CAPO CI. The field can be added later.
While we support multiple ephemeral disks in this PR, we documented that this is to use at your own risks, this hasn't been well tested in Nova.
API union discriminators aren't used but are here just in case at some point kube api will support it.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

netlify · 2023-09-29T19:13:21Z

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

Name	Link
🔨 Latest commit	`71fa48f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/653fa710fc5b48000890eff0
😎 Deploy Preview	https://deploy-preview-1696--kubernetes-sigs-cluster-api-openstack.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/v1alpha7/types.go

mandre · 2023-10-05T17:32:12Z

I'll help on this PR as we'd like to get that merge relatively soon. For now, this is just a simple rebase to resolve the conflicts, it obviously needs more work, and I suspect I broke the tests in the process 😅

api/v1alpha7/types.go

pkg/cloud/services/compute/instance_test.go

api/v1alpha7/types.go

mandre · 2023-10-11T10:17:52Z

/test pull-cluster-api-provider-openstack-e2e-test

EmilienM · 2023-10-26T17:12:39Z

I've removed CEL validations from the PR into a separate commit, see commit message for details, please.

EmilienM · 2023-10-26T17:20:59Z

/test pull-cluster-api-provider-openstack-e2e-test

EmilienM · 2023-10-26T18:02:43Z

api/v1alpha7/openstackmachine_types.go

@@ -89,7 +89,6 @@ type OpenStackMachineSpec struct {
 	// + ---
 	// + While it's possible to have unlimited number of block devices attached to a server instance, the number is
 	// + limited to 255 to avoid rule cost exceeded error in CRD validation.
-	// +kubebuilder:validation:MaxItems=255


#1736 is an issue that I created so at some point we can re-add CEL.

mdbooth · 2023-10-27T10:04:33Z

The offline CEL discussion was with me, btw. For the record I'm very much in favour of CEL and I'd like to replace all webhook validations that can be implemented with it. My concern is that we should not add CEL without tests because it will inevitably be broken, and this will break folks using a cluster where it's enabled. If we're going to add CEL we need to test it. If we can't test it, we shouldn't add it yet. But we will add it.

mdbooth

Nothing major from me. This is nearly ready to go.

.golangci.yml

api/v1alpha7/openstackmachine_types.go

mdbooth · 2023-10-27T10:28:48Z

api/v1alpha7/types.go

+// contains additional storage options.
+// +union
+//
+//nolint:godot


What was godot complaining about?

I can help with this. It is complaining about missing dot in the comment above. And this is also the directive that no-unused was complaining about.

mdbooth · 2023-10-27T10:29:05Z

api/v1alpha7/types.go

-	// AvailabilityZone is the volume availability zone to create the volume in.
+// blockDeviceStorage is the storage type of a block device to create and
+// contains additional storage options.
+// +union


Does +union do anything here?

Joel asked me to put it, I don't think it hurts but I also don't think it's functionally useful now in the way things work.

mdbooth · 2023-10-27T10:33:06Z

api/v1alpha7/types.go

+
+	// volume contains additional storage options for a volume block device.
+	// +optional
+	// +unionMember,optional


pkg/cloud/services/compute/instance.go

pkg/cloud/services/compute/instance_test.go

pkg/cloud/services/compute/instance.go

EmilienM · 2023-10-27T15:58:34Z

api/v1alpha7/openstackmachine_webhook.go

@@ -62,6 +62,14 @@ func (r *OpenStackMachine) ValidateCreate() (admission.Warnings, error) {
 		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "identityRef", "kind"), "must be a Secret"))
 	}

+	if r.Spec.RootVolume != nil && r.Spec.AdditionalBlockDevices != nil {


this file has no unit tests, but I started to write it and i'll publish via another PR if reviewers don't mind.

Ideally these would be envtests, btw. We could then re-use them to validate CEL.

mdbooth · 2023-10-27T16:57:04Z

api/v1alpha7/types.go

+type BlockDeviceStorage struct {
+	// type is the type of block device to create.
+	// This can be either "Volume" or "Local".
+	// +unionDiscriminator


We discussed this. These stay at least for now.

k8s-ci-robot · 2023-10-27T16:59:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, mdbooth

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

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

lentzi90

Not sure if I missed it before or if it was changed, but the doc strings should be matching the code also for capitalization. I get that this will then make the yaml description not match the yaml fields but this is the system we currently follow.

api/v1alpha7/openstackmachine_types.go

api/v1alpha7/types.go

Co-authored-by: Emilien Macchi <[email protected]> Co-authored-by: Martin André <[email protected]>

Hitting golangci/golangci-lint#3228 when adding nolint. So allow to ignore unused nolints.

I've put that into a separate commit so we can re-add it later. It also serves as documentation for the work we have done to add CEL. However and unfortunately, we don't have CEL tests in place now so we decided to not have CEL validations for now; add them later and then we'll be able to revert this commit.

lentzi90

/lgtm

lentzi90 · 2023-10-30T12:54:03Z

/test pull-cluster-api-provider-openstack-e2e-full-test

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

k8s-ci-robot requested review from seanschneeweiss and tobiasgiese September 29, 2023 19:13

EmilienM force-pushed the bd_ephemeral branch 2 times, most recently from 4081b9f to ef93718 Compare September 30, 2023 01:23

EmilienM commented Sep 30, 2023

View reviewed changes

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

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

mandre force-pushed the bd_ephemeral branch from ef93718 to c095bff Compare October 5, 2023 17:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 5, 2023

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

mdbooth reviewed Oct 6, 2023

View reviewed changes

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

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

EmilienM force-pushed the bd_ephemeral branch from fe50028 to 13db8c9 Compare October 10, 2023 15:34

EmilienM commented Oct 10, 2023

View reviewed changes

pkg/cloud/services/compute/instance_test.go Outdated Show resolved Hide resolved

EmilienM commented Oct 10, 2023

View reviewed changes

api/v1alpha7/types.go Show resolved Hide resolved

EmilienM force-pushed the bd_ephemeral branch 3 times, most recently from 38b304d to 080fa93 Compare October 10, 2023 23:59

EmilienM force-pushed the bd_ephemeral branch 3 times, most recently from 26f74c3 to d425b01 Compare October 12, 2023 15:47

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 12, 2023

EmilienM force-pushed the bd_ephemeral branch from d425b01 to a6335e2 Compare October 12, 2023 15:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

EmilienM commented Oct 26, 2023

View reviewed changes

EmilienM force-pushed the bd_ephemeral branch from e44d4fa to 5523e52 Compare October 26, 2023 20:02

mdbooth requested changes Oct 27, 2023

View reviewed changes

EmilienM force-pushed the bd_ephemeral branch from 5523e52 to ff4a773 Compare October 27, 2023 13:36

EmilienM requested a review from mdbooth October 27, 2023 13:40

EmilienM force-pushed the bd_ephemeral branch from ff4a773 to 3279ca0 Compare October 27, 2023 13:49

mdbooth reviewed Oct 27, 2023

View reviewed changes

pkg/cloud/services/compute/instance.go Outdated Show resolved Hide resolved

EmilienM force-pushed the bd_ephemeral branch 2 times, most recently from 423e91d to c757f37 Compare October 27, 2023 14:25

EmilienM requested a review from mdbooth October 27, 2023 14:26

EmilienM commented Oct 27, 2023

View reviewed changes

EmilienM mentioned this pull request Oct 27, 2023

Add unit tests for openstackmachine_webhook #1737

Closed

mdbooth approved these changes Oct 27, 2023

View reviewed changes

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

lentzi90 reviewed Oct 30, 2023

View reviewed changes

EmilienM and others added 3 commits October 30, 2023 08:52

Add ephemeral storage support to the AdditionalBlockDevices

1efeaf7

Co-authored-by: Emilien Macchi <[email protected]> Co-authored-by: Martin André <[email protected]>

ci: relax nolintlint

4336f38

Hitting golangci/golangci-lint#3228 when adding nolint. So allow to ignore unused nolints.

EmilienM force-pushed the bd_ephemeral branch from c757f37 to 71fa48f Compare October 30, 2023 12:52

lentzi90 reviewed Oct 30, 2023

View reviewed changes

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

k8s-ci-robot merged commit 5968640 into kubernetes-sigs:main Oct 30, 2023
3 checks passed

EmilienM deleted the bd_ephemeral branch October 30, 2023 14:14

mandre mentioned this pull request Jan 8, 2024

REQUEST: New membership for mandre kubernetes/org#4663

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

EmilienM commented Sep 29, 2023 •

edited

Loading

netlify bot commented Sep 29, 2023 •

edited

Loading

mandre commented Oct 5, 2023

mandre commented Oct 11, 2023

EmilienM commented Oct 26, 2023

EmilienM commented Oct 26, 2023

EmilienM Oct 26, 2023

mdbooth commented Oct 27, 2023

mdbooth left a comment

mdbooth Oct 27, 2023

lentzi90 Oct 27, 2023

mdbooth Oct 27, 2023

EmilienM Oct 27, 2023

mdbooth Oct 27, 2023

EmilienM Oct 27, 2023

EmilienM Oct 27, 2023

mdbooth Oct 27, 2023

mdbooth Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

lentzi90 left a comment

lentzi90 left a comment

lentzi90 commented Oct 30, 2023

✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

Conversation

EmilienM commented Sep 29, 2023 • edited Loading

netlify bot commented Sep 29, 2023 • edited Loading

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

mandre commented Oct 5, 2023

mandre commented Oct 11, 2023

EmilienM commented Oct 26, 2023

EmilienM commented Oct 26, 2023

Choose a reason for hiding this comment

mdbooth commented Oct 27, 2023

mdbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2023

lentzi90 left a comment

Choose a reason for hiding this comment

lentzi90 left a comment

Choose a reason for hiding this comment

lentzi90 commented Oct 30, 2023

EmilienM commented Sep 29, 2023 •

edited

Loading

netlify bot commented Sep 29, 2023 •

edited

Loading