From 7ba9ecba84ae46edfc8ed8e0213fb7390345df60 Mon Sep 17 00:00:00 2001 From: f41gh7 Date: Wed, 20 Nov 2024 10:08:50 +0100 Subject: [PATCH] vmsingle: properly add volumes with non-empty storageDataPath Previously, operator inconsistenetely added volumeMounts for vmsingle deployment, if spec.storageDataPath is set. It added only volumeMount without volumes. And it produces runtime deployment creation error. Intentional behaviour and the main puprose of storageDataPath option is to allow to override storage settings configured by operator. This commit skips adding volumeMounts for this case and fixes runtime error. Also it adds additional check to the validation webhook, which validates if volumes and volumeMounts is set for non-mepty storageDataPath. Related issue: https://github.com/VictoriaMetrics/operator/issues/1158 Signed-off-by: f41gh7 --- api/operator/v1beta1/vmsingle_types.go | 2 + api/operator/v1beta1/vmsingle_webhook.go | 8 +++ config/crd/overlay/crd.yaml | 2 + docs/api.md | 4 +- .../operator/factory/vmsingle/vmsingle.go | 49 ++++++++++--------- test/e2e/vmsingle_test.go | 33 +++++++++++++ 6 files changed, 72 insertions(+), 26 deletions(-) diff --git a/api/operator/v1beta1/vmsingle_types.go b/api/operator/v1beta1/vmsingle_types.go index e62ec114..8146781f 100644 --- a/api/operator/v1beta1/vmsingle_types.go +++ b/api/operator/v1beta1/vmsingle_types.go @@ -38,10 +38,12 @@ type VMSingleSpec struct { LogFormat string `json:"logFormat,omitempty"` // StorageDataPath disables spec.storage option and overrides arg for victoria-metrics binary --storageDataPath, // its users responsibility to mount proper device into given path. + // It requires to provide spec.volumes and spec.volumeMounts with at least 1 value // +optional StorageDataPath string `json:"storageDataPath,omitempty"` // Storage is the definition of how storage will be used by the VMSingle // by default it`s empty dir + // this option is ignored if storageDataPath is set // +optional Storage *v1.PersistentVolumeClaimSpec `json:"storage,omitempty"` diff --git a/api/operator/v1beta1/vmsingle_webhook.go b/api/operator/v1beta1/vmsingle_webhook.go index ee194b85..bcbb17d4 100644 --- a/api/operator/v1beta1/vmsingle_webhook.go +++ b/api/operator/v1beta1/vmsingle_webhook.go @@ -46,6 +46,14 @@ func (r *VMSingle) sanityCheck() error { return err } } + if r.Spec.StorageDataPath != "" { + if len(r.Spec.VolumeMounts) == 0 { + return fmt.Errorf("spec.volumeMounts must have at least 1 value for spec.storageDataPath=%q", r.Spec.StorageDataPath) + } + if len(r.Spec.Volumes) == 0 { + return fmt.Errorf("spec.volumes must have at least 1 value for spec.storageDataPath=%q", r.Spec.StorageDataPath) + } + } return nil } diff --git a/config/crd/overlay/crd.yaml b/config/crd/overlay/crd.yaml index 516629b8..a55a70b4 100644 --- a/config/crd/overlay/crd.yaml +++ b/config/crd/overlay/crd.yaml @@ -28248,6 +28248,7 @@ spec: description: |- Storage is the definition of how storage will be used by the VMSingle by default it`s empty dir + this option is ignored if storageDataPath is set properties: accessModes: description: |- @@ -28447,6 +28448,7 @@ spec: description: |- StorageDataPath disables spec.storage option and overrides arg for victoria-metrics binary --storageDataPath, its users responsibility to mount proper device into given path. + It requires to provide spec.volumes and spec.volumeMounts with at least 1 value type: string storageMetadata: description: StorageMeta defines annotations and labels attached to diff --git a/docs/api.md b/docs/api.md index dfa2b839..d44416fb 100644 --- a/docs/api.md +++ b/docs/api.md @@ -3897,8 +3897,8 @@ _Appears in:_ | `serviceAccountName` | ServiceAccountName is the name of the ServiceAccount to use to run the pods | _string_ | false | | `serviceScrapeSpec` | ServiceScrapeSpec that will be added to vmsingle VMServiceScrape spec | _[VMServiceScrapeSpec](#vmservicescrapespec)_ | false | | `serviceSpec` | ServiceSpec that will be added to vmsingle service spec | _[AdditionalServiceSpec](#additionalservicespec)_ | false | -| `storage` | Storage is the definition of how storage will be used by the VMSingle
by default it`s empty dir | _[PersistentVolumeClaimSpec](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#persistentvolumeclaimspec-v1-core)_ | false | -| `storageDataPath` | StorageDataPath disables spec.storage option and overrides arg for victoria-metrics binary --storageDataPath,
its users responsibility to mount proper device into given path. | _string_ | false | +| `storage` | Storage is the definition of how storage will be used by the VMSingle
by default it`s empty dir
this option is ignored if storageDataPath is set | _[PersistentVolumeClaimSpec](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#persistentvolumeclaimspec-v1-core)_ | false | +| `storageDataPath` | StorageDataPath disables spec.storage option and overrides arg for victoria-metrics binary --storageDataPath,
its users responsibility to mount proper device into given path.
It requires to provide spec.volumes and spec.volumeMounts with at least 1 value | _string_ | false | | `storageMetadata` | StorageMeta defines annotations and labels attached to PVC for given vmsingle CR | _[EmbeddedObjectMetadata](#embeddedobjectmetadata)_ | false | | `streamAggrConfig` | StreamAggrConfig defines stream aggregation configuration for VMSingle | _[StreamAggrConfig](#streamaggrconfig)_ | true | | `terminationGracePeriodSeconds` | TerminationGracePeriodSeconds period for container graceful termination | _integer_ | false | diff --git a/internal/controller/operator/factory/vmsingle/vmsingle.go b/internal/controller/operator/factory/vmsingle/vmsingle.go index dd2a87af..7853a224 100644 --- a/internal/controller/operator/factory/vmsingle/vmsingle.go +++ b/internal/controller/operator/factory/vmsingle/vmsingle.go @@ -138,8 +138,10 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P fmt.Sprintf("-retentionPeriod=%s", cr.Spec.RetentionPeriod), } - // if customStorageDataPath is not empty, do not add pvc. - shouldAddPVC := cr.Spec.StorageDataPath == "" + // if customStorageDataPath is not empty, do not add volumes + // and volumeMounts + // it's user responsobility to provide correct values + addVolumeMounts := cr.Spec.StorageDataPath == "" storagePath := vmSingleDataDir if cr.Spec.StorageDataPath != "" { @@ -165,27 +167,32 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P var ports []corev1.ContainerPort ports = append(ports, corev1.ContainerPort{Name: "http", Protocol: "TCP", ContainerPort: intstr.Parse(cr.Spec.Port).IntVal}) ports = build.AppendInsertPorts(ports, cr.Spec.InsertPorts) - volumes := []corev1.Volume{} - storageSpec := cr.Spec.Storage + var volumes []corev1.Volume + var vmMounts []corev1.VolumeMount - if storageSpec == nil { - volumes = append(volumes, corev1.Volume{ - Name: vmDataVolumeName, - VolumeSource: corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{}, - }, - }) - } else if shouldAddPVC { - volumes = append(volumes, corev1.Volume{ - Name: vmDataVolumeName, - VolumeSource: corev1.VolumeSource{ + // conditionally add volume mounts + if addVolumeMounts { + vmMounts = append(vmMounts, corev1.VolumeMount{ + Name: vmDataVolumeName, + MountPath: storagePath}, + ) + + vlSource := corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + } + if cr.Spec.Storage != nil { + vlSource = corev1.VolumeSource{ PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ ClaimName: cr.PrefixedName(), }, - }, - }) + } + } + volumes = append(volumes, corev1.Volume{ + Name: vmDataVolumeName, + VolumeSource: vlSource}) } + if cr.Spec.VMBackup != nil && cr.Spec.VMBackup.CredentialsSecret != nil { volumes = append(volumes, corev1.Volume{ Name: k8stools.SanitizeVolumeName("secret-" + cr.Spec.VMBackup.CredentialsSecret.Name), @@ -196,14 +203,8 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P }, }) } - volumes = append(volumes, cr.Spec.Volumes...) - vmMounts := []corev1.VolumeMount{ - { - Name: vmDataVolumeName, - MountPath: storagePath, - }, - } + volumes = append(volumes, cr.Spec.Volumes...) vmMounts = append(vmMounts, cr.Spec.VolumeMounts...) for _, s := range cr.Spec.Secrets { diff --git a/test/e2e/vmsingle_test.go b/test/e2e/vmsingle_test.go index bb6e7c65..70f44bdf 100644 --- a/test/e2e/vmsingle_test.go +++ b/test/e2e/vmsingle_test.go @@ -195,6 +195,39 @@ var _ = Describe("test vmsingle Controller", func() { Expect(*createdDeploy.Spec.Template.Spec.Containers[0].SecurityContext.RunAsNonRoot).To(BeTrue()) }), + Entry("with data emptyDir", "emptydir", + &vmv1beta1.VMSingle{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + }, + Spec: vmv1beta1.VMSingleSpec{ + CommonApplicationDeploymentParams: vmv1beta1.CommonApplicationDeploymentParams{ + ReplicaCount: ptr.To[int32](1), + }, + CommonDefaultableParams: vmv1beta1.CommonDefaultableParams{ + UseStrictSecurity: ptr.To(false), + }, + RetentionPeriod: "1", + RemovePvcAfterDelete: true, + StorageDataPath: "/tmp/", + Storage: &corev1.PersistentVolumeClaimSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Gi"), + }, + }, + }, + }, + }, + func(cr *vmv1beta1.VMSingle) { + createdChildObjects := types.NamespacedName{Namespace: namespace, Name: cr.PrefixedName()} + var createdDeploy appsv1.Deployment + Expect(k8sClient.Get(ctx, createdChildObjects, &createdDeploy)).To(Succeed()) + ts := createdDeploy.Spec.Template.Spec + Expect(ts.Containers).To(HaveLen(1)) + Expect(ts.Volumes).To(HaveLen(0)) + Expect(ts.Containers[0].VolumeMounts).To(HaveLen(0)) + }), ) existSingle := &vmv1beta1.VMSingle{