Skip to content

✨ Additional data volumes for machines #1668

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

Merged
k8s-ci-robot merged 1 commit into from
Oct 4, 2023
Merged

✨ Additional data volumes for machines #1668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions api/v1alpha5/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,3 +433,7 @@ func Convert_v1alpha5_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(

return nil
}

func Convert_v1alpha7_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec(in *infrav1.OpenStackMachineSpec, out *OpenStackMachineSpec, s conversion.Scope) error {
return autoConvert_v1alpha7_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec(in, out, s)
}
16 changes: 6 additions & 10 deletions api/v1alpha5/zz_generated.conversion.go
View file
Open in desktop

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

11 changes: 7 additions & 4 deletions api/v1alpha6/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,12 @@ func restorev1alpha7MachineSpec(previous *infrav1.OpenStackMachineSpec, dst *inf
// PropagateUplinkStatus has been added in v1alpha7.
// We restore the whole Ports since they are anyway immutable.
dst.Ports = previous.Ports
dst.AdditionalBlockDevices = previous.AdditionalBlockDevices
}

func restorev1alpha7Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
// PropagateUplinkStatus has been added in v1alpha7.
// We restore the whole Ports since they are anyway immutable.
if *previous != nil && (*previous).Instance.Ports != nil && *dst != nil && (*dst).Instance.Ports != nil {
(*dst).Instance.Ports = (*previous).Instance.Ports
if *previous != nil && *dst != nil {
restorev1alpha7MachineSpec(&(*previous).Instance, &(*dst).Instance)
}
}

Expand Down Expand Up @@ -646,3 +645,7 @@ func Convert_v1alpha6_OpenStackClusterStatus_To_v1alpha7_OpenStackClusterStatus(

return nil
}

func Convert_v1alpha7_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *infrav1.OpenStackMachineSpec, out *OpenStackMachineSpec, s apiconversion.Scope) error {
return autoConvert_v1alpha7_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in, out, s)
}
16 changes: 6 additions & 10 deletions api/v1alpha6/zz_generated.conversion.go
View file
Open in desktop

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

5 changes: 5 additions & 0 deletions api/v1alpha7/openstackmachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ type OpenStackMachineSpec struct {
// The volume metadata to boot from
RootVolume *RootVolume `json:"rootVolume,omitempty"`

// AdditionalBlockDevices is a list of specifications for additional block devices to attach to the server instance
// +listType=map
// +listMapKey=name
AdditionalBlockDevices []AdditionalBlockDevice `json:"additionalBlockDevices,omitempty"`

// The server group to assign the machine to
ServerGroupID string `json:"serverGroupID,omitempty"`

Expand Down
14 changes: 14 additions & 0 deletions api/v1alpha7/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,20 @@ 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"`
// AvailabilityZone is the volume availability zone to create the volume in.
// If omitted, the availability zone of the server will be used.
AvailabilityZone string `json:"availabilityZone,omitempty"`
Copy link
Copy Markdown
Contributor

@mdbooth mdbooth Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@stephenfin Very much interested in your input here.

I've proposed adding useMachineAZ (default false) to a future api version for the root volume. What do you think the semantics of this one should be?

Options:

  • Empty AZ means no AZ
  • Empty AZ means something machine related
  • Anything else?

Is there any reason to have an option here to use the same AZ as the machine?

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 13, 2023

Choose a reason for hiding this comment

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

I was planning to make the semantics the same as the root volume for now. I think that is less confusing.

Then when we change the root volume we can change this as well. My plan is to have one "volume creating" function so hopefully the logic doesn't have to be duplicated.

Copy link
Copy Markdown
Contributor

@mdbooth mdbooth Sep 13, 2023

Choose a reason for hiding this comment

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

The problem is we know the semantics of the rootVolume are wrong, and we're going to have a minor upgrade problem there. If we know what we're going to do with rootVolume I'd prefer to get this right in the first instance.

See #1662 for WIP.

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 13, 2023

Choose a reason for hiding this comment

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

What is left to do on the root volume WIP? Should we get that merged first?

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 13, 2023

Choose a reason for hiding this comment

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

It looks like a sensible change to me. Are you worried about it being a breaking change because the default behaviour changes from "machine AZ" to "no AZ"?

(Which should have been the default all along IMHO, but what's done is done...)

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 22, 2023

Choose a reason for hiding this comment

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

I guess if we want a non-breaking change, then the useServerAZ flag defaults to true.

Copy link
Copy Markdown
Contributor

@EmilienM EmilienM Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess if we want a non-breaking change, then the useServerAZ flag defaults to true.

That's what I believe is already CAPO's behaviour on rootVolumes:

cluster-api-provider-openstack/pkg/cloud/services/compute/instance.go

Lines 439 to 442 in 9d183bd

availabilityZone := instanceSpec.FailureDomain
if rootVolume.AvailabilityZone != "" {
availabilityZone = rootVolume.AvailabilityZone
}

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes - I mirrored the current root behaviour for the additional volumes on purpose.

I think we should change the behaviour for all types of volume together in a separate PR.

Copy link
Copy Markdown
Contributor

@mdbooth mdbooth Sep 27, 2023

Choose a reason for hiding this comment

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

I don't like this behaviour for the same reason we need to change it for the root volume, namely what @JohnGarbutt called the '80% case' is impossible. We are definitely have to change it.

However, I can also see the benefit of being consistent with the root volume, and making a breaking change to both APIs at the same time in the future.

Copy link
Copy Markdown
Contributor

@EmilienM EmilienM Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We came to an agreement to bulk change it via another PR. I added it to the TODO.

}
Copy link
Copy Markdown
Contributor

@EmilienM EmilienM Sep 20, 2023

Choose a reason for hiding this comment

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

This can be addressed in a separate PR but for supporting local disks, we'll need to add more fields.

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 20, 2023

Choose a reason for hiding this comment

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

Just one I think, to indicate whether it should be a local or Cinder disk? The rest can be reused in both cases.

Copy link
Copy Markdown
Contributor

@mdbooth mdbooth Sep 21, 2023

Choose a reason for hiding this comment

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

If we accept my argument that there will be a single local disk then I would prefer it to have a separate field because it's more obvious to the user and doesn't require validation. I would only put a local flag here if we decide it's useful to have multiple local disks.

But as you say we don't need to consider this yet.

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 22, 2023

Choose a reason for hiding this comment

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

@mdbooth as I raised in Slack, I think I have a use case for multiple ephemeral devices carved out of the ephemeral volume, so I would like to see an option that permits that.

Copy link
Copy Markdown
Author

@mkjpryor mkjpryor Sep 22, 2023

Choose a reason for hiding this comment

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

But it is something for a future PR anyway

Copy link
Copy Markdown
Contributor

@EmilienM EmilienM Sep 25, 2023

Choose a reason for hiding this comment

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

@mkjpryor Please look at my recent refactor & patches, I think the way we're proposing now is the same behaviour as with rootVolumes. And indeed this can be bulked changed later.

Copy link
Copy Markdown
Contributor

@mandre mandre Sep 26, 2023

Choose a reason for hiding this comment

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

If we were to add support for local ephemeral drives, we would need the DestinationType field and that's it, I think.


// NetworkStatus contains basic information about an existing neutron network.
type NetworkStatus struct {
Name string `json:"name"`
Expand Down
20 changes: 20 additions & 0 deletions 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.

30 changes: 30 additions & 0 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 @@ -3770,6 +3770,36 @@ spec:
instance:
description: Instance for the bastion itself
properties:
additionalBlockDevices:
description: AdditionalBlockDevices is a list of specifications
for additional block devices to attach to the server instance
items:
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.
type: string
required:
- diskSize
- name
type: object
type: array
x-kubernetes-list-map-keys:
- name
x-kubernetes-list-type: map
cloudName:
description: The name of the cloud to use from the clouds
secret
Expand Down
34 changes: 34 additions & 0 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 @@ -1610,6 +1610,40 @@ spec:
instance:
description: Instance for the bastion itself
properties:
additionalBlockDevices:
description: AdditionalBlockDevices is a list of specifications
for additional block devices to attach to the server
instance
items:
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.
type: string
required:
- diskSize
- name
type: object
type: array
x-kubernetes-list-map-keys:
- name
x-kubernetes-list-type: map
cloudName:
description: The name of the cloud to use from the
clouds secret
Expand Down
30 changes: 30 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1159,6 +1159,36 @@ spec:
spec:
description: OpenStackMachineSpec defines the desired state of OpenStackMachine.
properties:
additionalBlockDevices:
description: AdditionalBlockDevices is a list of specifications for
additional block devices to attach to the server instance
items:
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.
type: string
required:
- diskSize
- name
type: object
type: array
x-kubernetes-list-map-keys:
- name
x-kubernetes-list-type: map
cloudName:
description: The name of the cloud to use from the clouds secret
type: string
Expand Down
30 changes: 30 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,36 @@ spec:
description: Spec is the specification of the desired behavior
of the machine.
properties:
additionalBlockDevices:
description: AdditionalBlockDevices is a list of specifications
for additional block devices to attach to the server instance
items:
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.
type: string
required:
- diskSize
- name
type: object
type: array
x-kubernetes-list-map-keys:
- name
x-kubernetes-list-type: map
cloudName:
description: The name of the cloud to use from the clouds
secret
Expand Down
23 changes: 12 additions & 11 deletions controllers/openstackmachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,17 +445,18 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl

func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) *compute.InstanceSpec {
instanceSpec := compute.InstanceSpec{
Name: openStackMachine.Name,
Image: openStackMachine.Spec.Image,
ImageUUID: openStackMachine.Spec.ImageUUID,
Flavor: openStackMachine.Spec.Flavor,
SSHKeyName: openStackMachine.Spec.SSHKeyName,
UserData: userData,
Metadata: openStackMachine.Spec.ServerMetadata,
ConfigDrive: openStackMachine.Spec.ConfigDrive != nil && *openStackMachine.Spec.ConfigDrive,
RootVolume: openStackMachine.Spec.RootVolume,
ServerGroupID: openStackMachine.Spec.ServerGroupID,
Trunk: openStackMachine.Spec.Trunk,
Name: openStackMachine.Name,
Image: openStackMachine.Spec.Image,
ImageUUID: openStackMachine.Spec.ImageUUID,
Flavor: openStackMachine.Spec.Flavor,
SSHKeyName: openStackMachine.Spec.SSHKeyName,
UserData: userData,
Metadata: openStackMachine.Spec.ServerMetadata,
ConfigDrive: openStackMachine.Spec.ConfigDrive != nil && *openStackMachine.Spec.ConfigDrive,
RootVolume: openStackMachine.Spec.RootVolume,
AdditionalBlockDevices: openStackMachine.Spec.AdditionalBlockDevices,
ServerGroupID: openStackMachine.Spec.ServerGroupID,
Trunk: openStackMachine.Spec.Trunk,
}

// Add the failure domain only if specified
Expand Down
Loading