Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

Merged
merged 3 commits into from
Oct 30, 2023
Merged

✨ Add ephemeral storage support to the AdditionalBlockDevices #1696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1efeaf7
Add ephemeral storage support to the `AdditionalBlockDevices`
EmilienM Sep 29, 2023
4336f38
ci: relax nolintlint
EmilienM Oct 23, 2023
71fa48f
api: remove CEL validations for `AdditionalBlockDevices`
EmilienM Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ linters-settings:
- pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1
alias: controlplanev1

nolintlint:
# https://github.com/golangci/golangci-lint/issues/3228
allow-unused: true
mdbooth marked this conversation as resolved.
Show resolved Hide resolved
staticcheck:
go: "1.17"
stylecheck:
Expand Down
1 change: 1 addition & 0 deletions api/v1alpha7/openstackmachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ type OpenStackMachineSpec struct {
// AdditionalBlockDevices is a list of specifications for additional block devices to attach to the server instance
// +listType=map
// +listMapKey=name
// +optional
mdbooth marked this conversation as resolved.
Show resolved Hide resolved
AdditionalBlockDevices []AdditionalBlockDevice `json:"additionalBlockDevices,omitempty"`

// The server group to assign the machine to
Expand Down
8 changes: 8 additions & 0 deletions api/v1alpha7/openstackmachine_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1737

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for _, device := range r.Spec.AdditionalBlockDevices {
if device.Name == "root" {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "additionalBlockDevices"), "cannot contain a device named \"root\" when rootVolume is set"))
}
}
}

return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
}

Expand Down
67 changes: 58 additions & 9 deletions api/v1alpha7/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,69 @@ type RootVolume struct {
AvailabilityZone string `json:"availabilityZone,omitempty"`
}

type AdditionalBlockDevice struct {
// Name of the Cinder volume in the context of a machine.
// It will be combined with the machine name to make the actual volume name.
Name string `json:"name"`
// Size is the size in GB of the volume.
Size int `json:"diskSize"`
// VolumeType is the volume type of the volume.
// If omitted, the default type will be used.
VolumeType string `json:"volumeType,omitempty"`
// BlockDeviceStorage is the storage type of a block device to create and
// contains additional storage options.
// +union
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does +union do anything here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//
//nolint:godot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was godot complaining about?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type BlockDeviceStorage struct {
EmilienM marked this conversation as resolved.
Show resolved Hide resolved
// Type is the type of block device to create.
// This can be either "Volume" or "Local".
// +unionDiscriminator
EmilienM marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided this didn't do anything, yet. We should probably remove it. Non-functional code markers tend to accumulate because everybody's too scared to remove them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this. These stay at least for now.

Type BlockDeviceType `json:"type"`

// Volume contains additional storage options for a volume block device.
// +optional
// +unionMember,optional
EmilienM marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Volume *BlockDeviceVolume `json:"volume,omitempty"`
}

// BlockDeviceVolume contains additional storage options for a volume block device.
type BlockDeviceVolume struct {
// Type is the Cinder volume type of the volume.
// If omitted, the default Cinder volume type that is configured in the OpenStack cloud
// will be used.
// +optional
Type string `json:"type,omitempty"`

// AvailabilityZone is the volume availability zone to create the volume in.
// If omitted, the availability zone of the server will be used.
// The availability zone must NOT contain spaces otherwise it will lead to volume that belongs
// to this availability zone register failure, see kubernetes/cloud-provider-openstack#1379 for
// further information.
// +optional
AvailabilityZone string `json:"availabilityZone,omitempty"`
}

// AdditionalBlockDevice is a block device to attach to the server.
type AdditionalBlockDevice struct {
// Name of the block device in the context of a machine.
// If the block device is a volume, the Cinder volume will be named
// as a combination of the machine name and this name.
// Also, this name will be used for tagging the block device.
// Information about the block device tag can be obtained from the OpenStack
// metadata API or the config drive.
Name string `json:"name"`

// SizeGiB is the size of the block device in gibibytes (GiB).
SizeGiB int `json:"sizeGiB"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a massive fan of field names that contain the unit. Is it not sufficient for the description and docs to say the unit?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you prefer not to have the units in the name, then you should use a resource.Quantity, which would mean the field would look like size: 128Gi to the user.

This is as per current Kube API conventions.

I'm happy with either but @EmilienM preferred this over the resource.Quantity IIUC

Copy link
Contributor

@lentzi90 lentzi90 Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a strong opinion, but for me it seems preferable to have it part of the name.
Edit: Wrote the comment before I saw what JoelSpeed said. No strong opinion from me still.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having size: 128Gi would be error prone IMO (we would certainly need some validation). I prefer to have the unit in the name. Unless someone has a strong opinion.


// Storage specifies the storage type of the block device and
// additional storage options.
Storage BlockDeviceStorage `json:"storage"`
}

// BlockDeviceType defines the type of block device to create.
type BlockDeviceType string

const (
// LocalBlockDevice is an ephemeral block device attached to the server.
LocalBlockDevice BlockDeviceType = "Local"

// VolumeBlockDevice is a volume block device attached to the server.
VolumeBlockDevice BlockDeviceType = "Volume"
)

// NetworkStatus contains basic information about an existing neutron network.
type NetworkStatus struct {
Name string `json:"name"`
Expand Down
40 changes: 39 additions & 1 deletion api/v1alpha7/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

63 changes: 47 additions & 16 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3774,27 +3774,58 @@ spec:
description: AdditionalBlockDevices is a list of specifications
for additional block devices to attach to the server instance
items:
description: AdditionalBlockDevice is a block device to
attach to the server.
properties:
availabilityZone:
description: AvailabilityZone is the volume availability
zone to create the volume in. If omitted, the availability
zone of the server will be used.
type: string
diskSize:
description: Size is the size in GB of the volume.
type: integer
name:
description: Name of the Cinder volume in the context
of a machine. It will be combined with the machine
name to make the actual volume name.
type: string
volumeType:
description: VolumeType is the volume type of the volume.
If omitted, the default type will be used.
description: Name of the block device in the context
of a machine. If the block device is a volume, the
Cinder volume will be named as a combination of the
machine name and this name. Also, this name will be
used for tagging the block device. Information about
the block device tag can be obtained from the OpenStack
metadata API or the config drive.
type: string
sizeGiB:
description: SizeGiB is the size of the block device
in gibibytes (GiB).
type: integer
storage:
description: Storage specifies the storage type of the
block device and additional storage options.
properties:
type:
description: Type is the type of block device to
create. This can be either "Volume" or "Local".
type: string
volume:
description: Volume contains additional storage
options for a volume block device.
properties:
availabilityZone:
description: AvailabilityZone is the volume
availability zone to create the volume in.
If omitted, the availability zone of the server
will be used. The availability zone must NOT
contain spaces otherwise it will lead to volume
that belongs to this availability zone register
failure, see kubernetes/cloud-provider-openstack#1379
for further information.
type: string
type:
description: Type is the Cinder volume type
of the volume. If omitted, the default Cinder
volume type that is configured in the OpenStack
cloud will be used.
type: string
type: object
required:
- type
type: object
required:
- diskSize
- name
- sizeGiB
- storage
type: object
type: array
x-kubernetes-list-map-keys:
Expand Down
71 changes: 52 additions & 19 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,30 +1615,63 @@ spec:
for additional block devices to attach to the server
instance
items:
description: AdditionalBlockDevice is a block device
to attach to the server.
properties:
availabilityZone:
description: AvailabilityZone is the volume
availability zone to create the volume in.
If omitted, the availability zone of the server
will be used.
type: string
diskSize:
description: Size is the size in GB of the volume.
type: integer
name:
description: Name of the Cinder volume in the
context of a machine. It will be combined
with the machine name to make the actual volume
name.
type: string
volumeType:
description: VolumeType is the volume type of
the volume. If omitted, the default type will
be used.
description: Name of the block device in the
context of a machine. If the block device
is a volume, the Cinder volume will be named
as a combination of the machine name and this
name. Also, this name will be used for tagging
the block device. Information about the block
device tag can be obtained from the OpenStack
metadata API or the config drive.
type: string
sizeGiB:
description: SizeGiB is the size of the block
device in gibibytes (GiB).
type: integer
storage:
description: Storage specifies the storage type
of the block device and additional storage
options.
properties:
type:
description: Type is the type of block device
to create. This can be either "Volume"
or "Local".
type: string
volume:
description: Volume contains additional
storage options for a volume block device.
properties:
availabilityZone:
description: AvailabilityZone is the
volume availability zone to create
the volume in. If omitted, the availability
zone of the server will be used. The
availability zone must NOT contain
spaces otherwise it will lead to volume
that belongs to this availability
zone register failure, see kubernetes/cloud-provider-openstack#1379
for further information.
type: string
type:
description: Type is the Cinder volume
type of the volume. If omitted, the
default Cinder volume type that is
configured in the OpenStack cloud
will be used.
type: string
type: object
required:
- type
type: object
required:
- diskSize
- name
- sizeGiB
- storage
type: object
type: array
x-kubernetes-list-map-keys:
Expand Down
Loading