:sparkles: Additional data volumes for machines by mkjpryor · Pull Request #1668 · kubernetes-sigs/cluster-api-provider-openstack

mkjpryor · 2023-09-13T13:30:57Z

What this PR does / why we need it:

This PR adds the ability for a machine to have additional data volumes attached, e.g. for etcd.

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

netlify · 2023-09-13T13:31:03Z

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

Name	Link
🔨 Latest commit	`94d9690`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/651c295a278dfd00086e887a
😎 Deploy Preview	https://deploy-preview-1668--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.

k8s-ci-robot · 2023-09-13T13:31:07Z

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

Details

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.

mdbooth

Looking good! Lets start the discussion.

api/v1alpha6/conversion.go

api/v1alpha7/types.go

mdbooth · 2023-09-13T13:55:17Z

api/v1alpha7/types.go

+	NameSuffix       string `json:"nameSuffix"`
+	Size             int    `json:"diskSize,omitempty"`
+	VolumeType       string `json:"volumeType,omitempty"`
+	AvailabilityZone string `json:"availabilityZone,omitempty"`


@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?

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.

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.

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

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...)

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

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

}

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.

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.

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

mdbooth

Please could you also add field comments of the form

// FieldName does X. It defaults to Y. It is incompatible with Z.

api/v1alpha7/types.go

api/v1alpha7/openstackmachine_types.go

api/v1alpha7/types.go

mdbooth · 2023-09-13T16:35:06Z

/ok-to-test

EmilienM · 2023-09-20T01:19:25Z

api/v1alpha7/openstackmachine_types.go

 	RootVolume *RootVolume `json:"rootVolume,omitempty"`

+	// DataVolumes is a list of specifications for volumes to attach to the server instance
+	DataVolumes []DataVolume `json:"dataVolumes,omitempty"`


Not a big deal, but just to mention the CAPZ used DataDisks.
I wonder if we want to follow that pattern, since our data might end-up being on volumes and/or local disks at some point.

I used DataVolumes to match RootVolume. I don't really have a strong opinion.

I think we're likely to end up with:

RootVolume

DataVolumes

LocalDataVolume

I'm with @mkjpryor on the lack of a strong opinion on the name. I agree that DataVolumes is consistent with RootVolume. Presumably they're also referenced in... KubeadmControlPlane? What are they called there?

The things you have to set in kubeadmcontrolplane and/or kubeadmbootstrapconfig correspond to cloud-init fields, so DiskSetup and Mounts.

But the OpenStack terminology for these things is volume, so my feeling is that we stick with that 🤷‍♂️

Could we settle on the API, considering that we'll support multiple ephemeral disks and volumes.

Option A. DataVolumes and LocalDataVolumes where each of them have the options needed for their use-case.

Option B. One field (maybe name it BlockDevices to avoid confusion with volumes`) with all fields that we need for both use cases.

I have a preference for Option B. (and the rename) for simplicity's sake.

My preference is also for option B with the name BlockDevices. I like using a name that mirrors the OpenStack API attribute that we are ultimately setting - I think it is probably less confusing for users.

Could we capture that these block devices explicitly don't include the root volume. I feel that DataDisks and DataVolumes already capture that in the word Data. If we really like BlockDevices, perhaps AdditionalBlockDevices?

EmilienM · 2023-09-20T01:21:20Z

api/v1alpha7/types.go

+	// This is then available in the instance metadata, allowing devices to be
+	// identified by tag inside the server.
+	Tag string `json:"tag,omitempty"`
+}


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

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

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.

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

But it is something for a future PR anyway

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

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

EmilienM · 2023-09-20T01:25:04Z

👍 excellent start to me!

Beside a few comments and missing doc (and tests?) this is good to me.

Note: I'm very interested by local disks instead volumes, but with your PR I can easily see it extended via a separate PR. However I have no objection (beside making the PR too big) if you include it in here.

mkjpryor · 2023-09-20T08:45:42Z

Note: I'm very interested by local disks instead volumes, but with your PR I can easily see it extended via a separate PR. However I have no objection (beside making the PR too big) if you include it in here.

I feel like it should be a separate PR, just in case there are unintended consequences that need addressing.

EmilienM · 2023-09-21T19:33:03Z

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

EmilienM · 2023-09-21T19:33:21Z

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

EmilienM · 2023-09-25T02:14:32Z

pkg/cloud/services/compute/instance.go

+				DeleteOnTermination: true,
+				// TODO(mkjpryor) uncomment once gophercloud patch is merged
+				// https://github.com/gophercloud/gophercloud/pull/2777
+				// Tag:                 dataVolumeSpec.Tag,


Once gophercloud/gophercloud#2789 is merged and a release was produced, I'll update vendoring here.

EmilienM · 2023-09-25T02:16:48Z

TODO:

Update Gophercloud vendoring once Prepare 1.7.0 gophercloud/gophercloud#2789 has merged and 1.7.0 was tagged.
Uncomment Tag
Documentation
Rebase on Update Gophercloud to 1.7.0 #1682 once it has merged to remove the vendoring commit.
e2e tests
Squash commits with Co-Authoring.

mdbooth · 2023-09-25T09:02:57Z

TODO:

Update Gophercloud vendoring once Prepare 1.7.0 gophercloud/gophercloud#2789 has merged and 1.7.0 was tagged.

Uncomment Tag

Documentation

Squash commits with Co-Authoring.

See above comment. I'd also like to make it clearer that BlockDevices isn't all block devices because it doesn't include the root volume.

mandre · 2023-09-26T12:18:16Z

api/v1alpha7/types.go

+	// This is then available in the instance metadata, allowing devices to be
+	// identified by tag inside the server.
+	Tag string `json:"tag,omitempty"`
+}


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

test/e2e/suites/e2e/e2e_test.go

mandre · 2023-09-26T12:36:03Z

test/e2e/suites/e2e/e2e_test.go

+				bootVolume := firstVolume
+				additionalVolume := secondVolume
+				// The boot volume is the one with the "bootable" flag set.
+				if firstVolume.Bootable != "true" { // This is genuinely a string, not a bool


Curious as to why we need this gymnastic. Don't we have the guarantee the first volume is always the boot volume?

I need to do more testing to confirm that listing volumes provide an output ordered by creation time. I'll look at that today because I agree that this is ugly 😄 .

I tested it and indeed it seems that the volume used for boot is always first.

mandre · 2023-09-26T12:42:25Z

test/e2e/data/kustomize/multi-az/patch-machine-template-control-plane.yaml

+  - diskSize: 1
+    nameSuffix: extravol
+    volumeType: ${OPENSTACK_VOLUME_TYPE_ALT}
+    availabilityZone: ${OPENSTACK_FAILURE_DOMAIN}


Perhaps it would be good to drop the availabilityZone there, to replicate the test we do for rootVolume?
We're testing setting an availabilityZone for the worker.

mandre · 2023-09-26T12:45:08Z

test/e2e/suites/e2e/e2e_test.go

 				Expect(bootVolume.VolumeType).To(Equal(volumeTypeAlt))
+
+				additionalVolume := additionalVolumes[machine.Name]
+				Expect(additionalVolume.AvailabilityZone).To(Equal(failureDomainAlt))


If we want to ensure the additionalVolumes behave the same as rootVolume, we should probably replicate the test, i.e. expect the value to be failureDomain.

mandre · 2023-09-26T13:11:41Z

pkg/cloud/services/compute/instance_test.go

+				"block_device_mapping_v2": []map[string]interface{}{
+					{
+						"delete_on_termination": true,
+						"destination_type":      "local",


IIUC, we don't support local yet, so it's weird to have it there.

it's the default block device mapping when no root volume is being used.

If you want to specify a non-bootable block device, you also have to specify a bootable one. That means that we need to specify the "from image to local" block device

mandre · 2023-09-26T13:19:52Z

pkg/cloud/services/compute/instance.go

-			might create the volume, but the instance create fails. This would leave us with a dangling
-			volume with no instance.
+			  1. Create the volume
+			  2. Create the instance with the created volume in iAdditionalBlockDevices with DeleteOnTermination=true


vim user spotted.

Suggested change

2. Create the instance with the created volume in iAdditionalBlockDevices with DeleteOnTermination=true

2. Create the instance with the created volume in AdditionalBlockDevices with DeleteOnTermination=true

Feels like we should mention that the root volume is added to AdditionalBlockDevices as it's not completely obvious, no?

Good point. I'm updating the doc.

mandre · 2023-09-26T13:50:59Z

pkg/cloud/services/compute/instance.go

+			BootIndex:           0,
+			DeleteOnTermination: true,
+		})
+	} else {


Just to make sure I understand the reason to set an ephemeral drive from the image, is that because if we don't set it and we have additional block devices, then we don't know where to boot the instance from?

yes exactly. We need an ephemeral storage to be created using the imageID and be bootable (bootindex 0).

Basically, yeah. OpenStack won't let you set non-bootable block devices without setting a bootable one

mandre · 2023-09-26T14:00:04Z

pkg/cloud/services/compute/instance.go

+		return nil, err
 	}
+	if existingVolume != nil {
+		if existingVolume.Size != opts.Size {


Don't we need to also check the availability zone and the type to ensure the existing volume conforms to the spec?
Alternatively, it might be simpler to error out when there is an existing volume with the name we want to use.

What if we do this:

When we create a volume, we put the Kubernetes UUIDs of the OpenStackCluster and the OpenStackMachine in the volume metadata

openstack.infrastructure.cluster.x-k8s.io/cluster-uuid

openstack.infrastructure.cluster.x-k8s.io/machine-uuid

When we search for a volume by name and find one, we then check if the UUIDs in the metadata match the cluster/machine being reconciled

If they do, we own the volume and that is the volume we return

If they don't, we don't own the volume and we error out

If that's a pattern people agree on, I'll be happy to do this via a separated PR if possible. For the sake of not adding too many design changes into one PR.

@mdbooth and I also discussed doing this for Neutron resources using tags

That looks reasonable. I would just change the logic to search for volume by name and by metadata in a single query if that is possible. If it returns a result, that's the one we own and we can use it, otherwise, we know we need to create a new one.

Not sure the single query is possible unfortunately, although I might have missed something. I would be surprised if the metadata items were indexed for searching.

I think it's possible:

https://github.com/gophercloud/gophercloud/blob/1ef3b0843934066d5c65163e64683657608e6975/openstack/blockstorage/v3/volumes/requests.go#L125-L126

We came to the agreement that we (I) will take care of it in a separated PR.

If we ever cut a release containing this feature which does not add the metadata described by @EmilienM then we have an upgrade issue here which would be a whole lot more complicated. If we don't think we can get it into this PR, lets not wait around to implement it.

Like I said on another comment, I'm working on it and this will be done before the next release is cut.

EmilienM · 2023-09-26T18:11:47Z

/hold cancel

This is ready for review.
There are a list of things we want to do next, but we believe that this should be done via a separated PR:

Address this comment to better filter volumes by metadata
Add ephemeral storage support to the AdditionalBlockDevices API; with doc/tests - via Add ephemeral storage support to the AdditionalBlockDevices #1692
Change AZ behaviour for both RootVolume & AdditionalBlockDevices - via Compute AZs should not be used as Volume AZs #1649
Document how to actually use the volumes by tag using diskSetup and mounts - see https://github.com/stackhpc/azimuth-images/pull/64/files for examples

mdbooth

This is really close, thanks!

Can you please also consider how soon we can add the volume metadata support? Ideally this will be within the next couple of months to avoid an upgrade issue.

mdbooth · 2023-09-27T12:20:23Z

api/v1alpha7/types.go

+	NameSuffix       string `json:"nameSuffix"`
+	Size             int    `json:"diskSize,omitempty"`
+	VolumeType       string `json:"volumeType,omitempty"`
+	AvailabilityZone string `json:"availabilityZone,omitempty"`


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.

mdbooth · 2023-09-27T12:31:26Z

pkg/cloud/services/compute/instance.go

+		return nil, err
 	}
+	if existingVolume != nil {
+		if existingVolume.Size != opts.Size {


If we ever cut a release containing this feature which does not add the metadata described by @EmilienM then we have an upgrade issue here which would be a whole lot more complicated. If we don't think we can get it into this PR, lets not wait around to implement it.

pkg/cloud/services/compute/instance.go

mdbooth · 2023-09-27T12:55:39Z

test/e2e/suites/e2e/e2e_test.go

+				Expect(volumes).To(HaveLen(2))

-				bootVolume, err := shared.GetOpenStackVolume(e2eCtx, volumes[0].ID)
+				rootVolume, err := shared.GetOpenStackVolume(e2eCtx, volumes[0].ID)


Is the order of AttachedVolumes stable? I would guess strictly not, although possibly stable enough in practise for this test?

it's apparently stable. The volume attachments listed for a server seem to be indexed by their creation, and the rootVolume is always created first. I was surprised myself too but I've tested it a few times and it was always the case 🤷 .

I'll try to find the code in Nova to confirm what I saw.

The API output is generated here. The bdms parameter is populated here. I followed that call to nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid all the way down to the DB layer and didn't spot any sorting being done. This leads me to believe we're probably relying on the order of the rows being returned by the DB. As you likely know, MySQL doesn't guarantee sorting without a specific ORDER BY so that doesn't seem wise since it won't be consistent.

You should double check this (!) but assuming this is correct, you probably want to explore device tagging. In general, nova doesn't guarantee anything wrt device ordering and asks that you use device tagging if you need this. I don't know if gophercloud support this but you simply want to add the tags field to the BDM for the root volume, e.g.

{ "block_device_mapping_v2": [ { "uuid": "<image_id>", "boot_index": 0, "source_type": "image", "destination_type": "volume", "volume_size": "<volume_size>", "tags": ["foo", "bar"] } ] }

(note: this requires API microversion 2.42).

You can then use the volume attachments API (GET /servers/{serverID}/os-volume-attachments) to list the attached volumes with tags, since this info is not returned in the os-extended-volumes:volumes_attached field in the GET /servers/{serverID} response.

right, that was my thought as well. I'll investigate it.

For now, I've addressed this comment with:

cluster-api-provider-openstack/test/e2e/suites/e2e/e2e_test.go

Lines 579 to 592 in 6855f55

// nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid does not guarantee order of the volumes

// so we need to find the boot volume by checking the "bootable" flag for now.

firstVolumeFound, err := shared.GetOpenStackVolume(e2eCtx, volumes[0].ID)

Expect(err).NotTo(HaveOccurred(), "failed to get OpenStack volume %s for machine %s", volumes[0].ID, machine.Name)

secondVolumeFound, err := shared.GetOpenStackVolume(e2eCtx, volumes[1].ID)

Expect(err).NotTo(HaveOccurred(), "failed to get OpenStack volume %s for machine %s", volumes[1].ID, machine.Name)

rootVolume := firstVolumeFound

additionalVolume := secondVolumeFound

// The boot volume is the one with the "bootable" flag set.

if !firstVolumeFound.Bootable == "true" { // This is genuinely a string, not a bool

rootVolume = secondVolumeFound

additionalVolume = firstVolumeFound

}

Until we find a better solution. With device tagging that I plan to work on next, we'll probably make this better.

EmilienM · 2023-09-27T13:21:20Z

This is really close, thanks!

Can you please also consider how soon we can add the volume metadata support? Ideally this will be within the next couple of months to avoid an upgrade issue.

I'm working on it now, I think the PR will be pushed before Tuesday next week.

lentzi90

Good job with this!
I agree with the decision to implement the AZ in the same way as for the root volume. It will be easier to understand it this way and then break both at the same time.

Could you please add some doc strings for at least the more complicated functions? I commented below on those I think are most important.

pkg/cloud/services/compute/instance.go

lentzi90 · 2023-09-29T05:30:13Z

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

EmilienM · 2023-09-29T18:08:59Z

@lentzi90 comments addressed. Thanks for the review!

mdbooth

Please could you respond to the 3 API requests:

Rename nameSuffix to name
Remove tag in favour of just using name
Addition of strategic merge patch annotation to additionalBlockDevices

There's also an outstanding request from @lentzi90 for a missing docstring.

Apart from that, this looks good and has good test coverage. I don't think there's nothing complicated in the above requests.

mdbooth · 2023-10-03T10:13:04Z

api/v1alpha7/types.go

+type AdditionalBlockDevice struct {
+	// NameSuffix is the suffix to be added to volume names.
+	// It will be combined with the machine name to make the volume name.
+	NameSuffix string `json:"nameSuffix"`


Could we call this name instead? The docstring can explain that the volume name will be the machine name with this appended.

I don't understand why you want to name it Name. This is not a name, this is a name suffix.
This would be a lie to the API user that their block device would be named like this, because this wouldn't be the case.

The actual block device name is chosen from within the code:

volumeName(instanceSpec.Name, blockDevice.NameSuffix)

See below. It would be the name of the volume in the context of this machine. It would also be the name of the volume in the additionalBlockDevices list.

fair enough, I'll do it.

mdbooth · 2023-10-03T10:13:33Z

api/v1alpha7/types.go

+	// Tag is a tag to add to the volume attachment.
+	// This is then available in the instance metadata, allowing devices to be
+	// identified by tag inside the server.
+	Tag string `json:"tag,omitempty"`


Is there any reason to have a separate tag element instead of just using the name?

api/v1alpha7/openstackmachine_types.go

mdbooth · 2023-10-03T13:03:15Z

To explain the above API requests, consider the case where a user wants to specify an additional volume to hold etcd.

Before:

additionalBlockDevices:
- nameSuffix: etcd
  size: 10
  tag: etcd

After:

additionalBlockDevices:
- name: etcd
  size: 10

I prefer the latter.

EmilienM · 2023-10-03T14:19:34Z

@mdbooth @lentzi90 I've applied all of your suggestions, thanks for the feedback. I think this time it's ready to go!

We now have the ability for a machine to have additional block devices to be attached. Here is an example on how to use `additionalBlockDevices` for adding additional Cinder volumes attached to the server instance: ```yaml additionalBlockDevices: - nameSuffix: dbvol1 size: 50 volumeType: my-volume-type availabilityZone: az0 tag: database ``` Co-authored-by: Matt Pryor <matt@stackhpc.com> Co-authored-by: Emilien Macchi <emilien@redhat.com>

k8s-ci-robot · 2023-10-03T15:27:45Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

mdbooth · 2023-10-03T15:29:09Z

Pending tests, of course.

@lentzi90 Can you lgtm if you're happy with the API changes?

lentzi90 · 2023-10-04T05:08:16Z

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

lentzi90

/lgtm

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 13, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 13, 2023

k8s-ci-robot requested review from dulek and lentzi90 September 13, 2023 13:31

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 13, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2023

mdbooth reviewed Sep 13, 2023

View reviewed changes

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

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

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

mdbooth reviewed Sep 13, 2023

View reviewed changes

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

api/v1alpha7/types.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 Sep 13, 2023

mkjpryor force-pushed the feature/data-volumes branch from 8491f71 to 28501e5 Compare September 14, 2023 16:03

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

EmilienM reviewed Sep 20, 2023

View reviewed changes

EmilienM force-pushed the feature/data-volumes branch from 93fc4f6 to fceff67 Compare September 25, 2023 02:13

EmilienM reviewed Sep 25, 2023

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 25, 2023

mandre reviewed Sep 26, 2023

View reviewed changes

EmilienM force-pushed the feature/data-volumes branch from 9fca026 to de72667 Compare September 26, 2023 18:09

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 26, 2023

mdbooth reviewed Sep 27, 2023

View reviewed changes

EmilienM force-pushed the feature/data-volumes branch 3 times, most recently from 6855f55 to b76c92c Compare September 28, 2023 16:53

EmilienM mentioned this pull request Sep 28, 2023

Identify Volumes (rootVolumes and additional block devices) with Metadata #1693

Closed

lentzi90 reviewed Sep 29, 2023

View reviewed changes

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

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

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

EmilienM force-pushed the feature/data-volumes branch from b76c92c to 26e9191 Compare September 29, 2023 15:35

mdbooth suggested changes Oct 3, 2023

View reviewed changes

EmilienM force-pushed the feature/data-volumes branch from 26e9191 to 61afcac Compare October 3, 2023 14:15

EmilienM force-pushed the feature/data-volumes branch from 61afcac to 94d9690 Compare October 3, 2023 14:46

mdbooth approved these changes Oct 3, 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 3, 2023

lentzi90 reviewed Oct 4, 2023

View reviewed changes

k8s-ci-robot assigned lentzi90 Oct 4, 2023

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

k8s-ci-robot merged commit ade1ed0 into kubernetes-sigs:main Oct 4, 2023

EmilienM deleted the feature/data-volumes branch October 4, 2023 11:58

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

	2. Create the instance with the created volume in iAdditionalBlockDevices with DeleteOnTermination=true
	2. Create the instance with the created volume in AdditionalBlockDevices with DeleteOnTermination=true

	// nova.objects.BlockDeviceMappingList.bdms_by_instance_uuid does not guarantee order of the volumes
	// so we need to find the boot volume by checking the "bootable" flag for now.
	firstVolumeFound, err := shared.GetOpenStackVolume(e2eCtx, volumes[0].ID)
	Expect(err).NotTo(HaveOccurred(), "failed to get OpenStack volume %s for machine %s", volumes[0].ID, machine.Name)
	secondVolumeFound, err := shared.GetOpenStackVolume(e2eCtx, volumes[1].ID)
	Expect(err).NotTo(HaveOccurred(), "failed to get OpenStack volume %s for machine %s", volumes[1].ID, machine.Name)

	rootVolume := firstVolumeFound
	additionalVolume := secondVolumeFound
	// The boot volume is the one with the "bootable" flag set.
	if !firstVolumeFound.Bootable == "true" { // This is genuinely a string, not a bool
	rootVolume = secondVolumeFound
	additionalVolume = firstVolumeFound
	}

Conversation

mkjpryor commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Sep 13, 2023

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdbooth Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EmilienM Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkjpryor Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EmilienM Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdbooth commented Sep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkjpryor Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mkjpryor commented Sep 13, 2023 •

edited

Loading

netlify bot commented Sep 13, 2023 •

edited

Loading

mdbooth Sep 13, 2023 •

edited

Loading

EmilienM Sep 22, 2023 •

edited

Loading

mkjpryor Sep 22, 2023 •

edited

Loading

EmilienM Sep 27, 2023 •

edited

Loading

mkjpryor Sep 22, 2023 •

edited

Loading

EmilienM commented Sep 25, 2023 •

edited

Loading