We talked a bit about this out of band. Maybe we can perform some validation on these AccessModes to prevent someone from adding something invalid that's just going to result in the cluster not forming.

I'm pretty sure the system won't catch an issue with the access mode until the kubevirt controllers actually attempt to provision the storage using the datavolume api. Pushing this validation up a bit would provide a faster feedback loop for users.

If you feel comfortable with the idea, i believe it's possible for this validation to live on the crd schema. It might require some kubebuilder trickery where we create a PersistentVolumeMode local to this api that references the corev1 one... Maybe something like

// +kubebuilder:validation:Enum=ReadWriteOnce;ReadWriteMany;ReadOnly;ReadWriteOncePod
type PersistentVolumeAccessMode corev1.PersistentVolumeAccessMode

// AccessModes is an array that contains the desired Access Modes the root volume should have.
// More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#access-modes
//
// +optional
AccessModes []PersistentVolumeAccessMode `json:"accessModes,omitempty"`

Thanks @davidvossel for the suggestion.
However, this creates a problem in a sense that the "local" type of PersistentVolumeAccessMode when we want to assign that array into dataVolume.Spec.Storage.AccessModes when the DV is creates according to the NodePool configuration:
https://github.com/openshift/hypershift/pull/1583/files#diff-276580ea82853abc6e9445605e0303e4881c564bcd9a67b87f5f7f7c6d5fff04R169

If we do that, the assignment is not possible:
Cannot use 'kvPlatform.RootVolume.Persistent.AccessModes' (type []"github.com/openshift/hypershift/api/v1alpha1".PersistentVolumeAccessMode) as the type []PersistentVolumeAccessMode

If we want to do this, we'll need to perform a casting between these types, entry by entry.
Do you see another option for us to use CRD validation using kubebuilder markers?

Thanks.

Hi @davidvossel ,
I've managed to get that working by casting the slice elements from the local type to the corev1.PersistentVolumeAccessMode type.

Now, a creation of a NodePool with unrecognized AccessMode is getting rejected by the API Server thanks to the CRD validation:

$ ./bin/hypershift create nodepool kubevirt --name oren-nodepool --root-volume-size 16 --root-volume-access-modes ReadWriteMany,ReadWriteOnce,david --cluster-name $CLUSTER_NAME
2022-07-24T16:19:50+03:00	ERROR	Failed to create nodepool	[...]

Error: NodePool.hypershift.openshift.io "oren-nodepool" is invalid: spec.platform.kubevirt.rootVolume.persistent.accessModes[2]: Unsupported value: "david": supported values: "ReadWriteOnce", "ReadWriteMany", "ReadOnly", "ReadWriteOncePod"

When submitting valid values only to --root-volume-access-modes, they are getting into the DV and the PVC, in order.

Could you please re-review?

shouldn't this be something like...

if len(kvPlatform.RootVolume.Persistent.AccessModes) != 0 {

otherwise i'm pretty sure this condition will always != nil which means the if statement wouldn't really be necessary.

Right, because it is an array so it can't be nil.
Fixing, thanks.