Additional volumes

[holmesb]

Closes #504.

Description of changes:
Allows extra disks to be specified. Builds upon work done in @mcbenjemaa's draft.

Unlike VMs, which have a unique ID for tracking, PVE provides no server-side ID for configuration items like disks. So this required implementing a pending queue ("pending guard") to avoid duplicates. Without this, spurious "Unused Disks" are created. Is off by default, but enabled in vm.go.

With this PR, we support disk flags: format, discard, iothread, and ssd in additionalVolumes.

[github-actions]

Tests

Please note that running unit and e2e tests requires manual approval from a team member.

e2e tests

We use labels to control which e2e tests contexts are run:

Label	Behaviour
none	Run Generic tests only
e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels	Do not run any e2e tests
e2e/flatcar run Flatcar e2e tests	Add Flatcar tests

ℹ️ Ask a team member to add the requested labels if you don't have enough permissions.

[mcbenjemaa]

thank you for raising this PR.

This is my initial review.
Please address the points i mentioned, until I find more time to look at the reconciliation function.

[mcbenjemaa]

Suggested change

	SSD *bool `json:"ssd,omitempty"`
	// SSDEmulation enables SSD emulation feature
	// SSD emulation sets a drive to be presented to the guest as a solid-state drive rather than a rotational hard disk
	// There is no requirement that the underlying storage actually be backed by SSDs; this feature can be used with physical media of any type
	// SSD emulation is not supported on VirtIO Block drives.
	SSDEmulation *bool `json:"ssdEmulation,omitempty"`

[holmesb]

Nice comment, but Proxmox field is ssd, not ssdEmulation. Do you agree CRDs and CAPMOX settings should align with PVE terminology? Otherwise we'll have to explain in documentation that "ssdEmulation" = the PVE "ssd" setting. Def "emulation" should be used in documentation tho, and I've included your code comment.

[wikkyk]

Thanks for this. I've put hold on it because we're working on v1alpha2 now. I'll help you port it when it's released which should be Soon™. We are wanting to improve the storage config in v1alpha3 and this would fit in.
However, the approach with AdditionalVolumes is wrong. We've made the same mistake with AdditionalNetworks in v1alpha1. This approach proved too inflexible and very costly to change (it's why v1alpha2 is taking so long). The correct way to do this is to follow the zero one infinity rule. Just make it so that you can have multiple volumes.

[holmesb]

ZOI redesign sounds a way off. I get the benefit of making disks uniform tho: simpler representation in schema and less documentation. No need for a second PR to make available features like discard, ioThread, ssd, etc to bootVolume. But trillion dollar clouds don't support this uniform approach, they separate the boot from data volumes the same as this PR. Eg:
AWS has spec.rootVolume. spec.nonRootVolumes[] are separate.
GCP has spec.rootDevice* fields. spec.additionalDisks[] are separate.
vSphere has a spec.diskGiB for the boot disk only. dataDisks are separate.

They all do support multiple disks (ie data disks). This is much more an MVP feature than disk uniformity, as shown by this being the most up-ticked issue in this project.

As an interim, I propose we mark spec.disks.additionalVolumes Deprecated in godoc/CRD descriptions, with a note that it will be replaced by spec.disks.volumes in a future API (v1alpha3). My latest commit just now achieves. Merging with deprecation notices will unblock a widely requested feature and is non-breaking (no breaking change in v1alpha1). We'll still converge to ZOI volumes[] in the upcoming API, which will also bring the flags (discard/ioThread/ssd, etc.) uniformly to the boot disk.

When v1alpha3 starts, we can introduce spec.disks.volumes[] with boot: true on one entry. I can contribute conversion: v1alpha1 → v1alpha3 (map bootVolume to volumes[{boot:true,…}], and additionalVolumes to volumes[{boot:false,…}]).

I’ve pushed the deprecation notes into this PR so godoc and CRD OpenAPI have additionalVolumes as Deprecated with a pointer to the future spec.disks.volumes. We've been using this PR for many weeks without issue, so hopefully it's mergeable more-or-less as-is, but I’m happy to iterate on wording. I’m also happy to help on the v1alpha3 volumes[] impl to ensure a smooth transition.