Skip to content

Commit

Permalink
build/vmsingle: properly add data volumeMount for external volume
Browse files Browse the repository at this point in the history 
v0.50.0 releases added changed logic for externally provided `volume`. It removed implicit `volumeMount` add and made it required.
It's a breaking change, which made upgrade procedure not straight forward.
 See this issue for context #1158

 It made impossible to perform smooth migration.

 This commit reverts implicit `volumeMount` add. But instead of unconditional append, operator
performs a check for volume name: `data`. It must make volume management more reliable and less confusing.

Signed-off-by: f41gh7 <[email protected]>
  • Loading branch information
@f41gh7
f41gh7 committed Jan 3, 2025
1 parent 5992757 commit fc5fe87
Show file tree
Hide file tree
Showing 4 changed files with 139 additions and 29 deletions.
19 changes: 16 additions & 3 deletions api/operator/v1beta1/vmsingle_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,25 @@ func (r *VMSingle) sanityCheck() error {
}
}
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)
}
var storageVolumeFound bool
for _, volume := range r.Spec.Volumes {
if volume.Name == "data" {
storageVolumeFound = true
break
}
}
if r.Spec.VMBackup != nil {
if !storageVolumeFound {
return fmt.Errorf("spec.volumes must have at least 1 value with `name: data` for spec.storageDataPath=%q."+
" It's required by operator to correctly mount backup volumeMount", r.Spec.StorageDataPath)
}
}
if len(r.Spec.VolumeMounts) == 0 && !storageVolumeFound {
return fmt.Errorf("spec.volumeMounts must have at least 1 value OR spec.volumes must have volume.name `data` for spec.storageDataPath=%q", r.Spec.StorageDataPath)
}
}
return nil
}
Expand Down
1 change: 1 addition & 0 deletions docs/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ aliases:
## tip

* BUGFIX: [vmagent](https://docs.victoriametrics.com/operator/resources/vmagent/): properly route headless service traffic to vmagent `pods` with `statefulMode` and `shardCount` defined.
* BUGFIX: [vmsingle](https://docs.victoriametrics.com/operator/resources/vmsingle/): properly add `volumeMount` for external `storageDataPath` `volume`.

## [v0.51.2](https://github.com/VictoriaMetrics/operator/releases/tag/v0.51.2)

Expand Down
79 changes: 57 additions & 22 deletions internal/controller/operator/factory/vmsingle/vmsingle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P
// if customStorageDataPath is not empty, do not add volumes
// and volumeMounts
// it's user responsobility to provide correct values
addVolumeMounts := cr.Spec.StorageDataPath == ""
mustAddVolumeMounts := cr.Spec.StorageDataPath == ""

storagePath := vmSingleDataDir
if cr.Spec.StorageDataPath != "" {
Expand Down Expand Up @@ -183,27 +183,7 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P
var volumes []corev1.Volume
var vmMounts []corev1.VolumeMount

// 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})
}
volumes, vmMounts = addVolumeMountsTo(volumes, vmMounts, cr, mustAddVolumeMounts, storagePath)

if cr.Spec.VMBackup != nil && cr.Spec.VMBackup.CredentialsSecret != nil {
volumes = append(volumes, corev1.Volume{
Expand Down Expand Up @@ -485,3 +465,58 @@ func deletePrevStateResources(ctx context.Context, rclient client.Client, cr, pr

return nil
}

func addVolumeMountsTo(volumes []corev1.Volume, vmMounts []corev1.VolumeMount, cr *vmv1beta1.VMSingle, mustAddVolumeMounts bool, storagePath string) ([]corev1.Volume, []corev1.VolumeMount) {

switch {
case mustAddVolumeMounts:
// add volume and mount point by operator directly
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})

case len(cr.Spec.Volumes) > 0:
// add missing volumeMount point for backward compatibility
// it simplifies management of external PVCs
var volumeNamePresent bool
for _, volume := range cr.Spec.Volumes {
if volume.Name == vmDataVolumeName {
volumeNamePresent = true
break
}
}
if volumeNamePresent {
var mustSkipVolumeAdd bool
for _, volumeMount := range cr.Spec.VolumeMounts {
if volumeMount.Name == vmDataVolumeName {
mustSkipVolumeAdd = true
break
}
}
if !mustSkipVolumeAdd {
vmMounts = append(vmMounts, corev1.VolumeMount{
Name: vmDataVolumeName,
MountPath: storagePath,
})
}
}

}

return volumes, vmMounts
}
69 changes: 65 additions & 4 deletions test/e2e/vmsingle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,19 @@ package e2e
import (
"context"

vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1"
"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/finalize"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
appsv1 "k8s.io/api/apps/v1"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"

vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1"
"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/finalize"
)

//nolint:dupl,lll
Expand Down Expand Up @@ -211,6 +210,68 @@ var _ = Describe("test vmsingle Controller", func() {
Expect(ts.Volumes).To(BeEmpty())
Expect(ts.Containers[0].VolumeMounts).To(BeEmpty())
}),
Entry("with external volume", "externalvolume",
&vmv1beta1.VMSingle{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
},
Spec: vmv1beta1.VMSingleSpec{
CommonApplicationDeploymentParams: vmv1beta1.CommonApplicationDeploymentParams{
ReplicaCount: ptr.To[int32](1),
Volumes: []corev1.Volume{
{
Name: "data",
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{},
},
},
{
Name: "backup",
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{},
},
},
{
Name: "unused",
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{},
},
},
},
VolumeMounts: []corev1.VolumeMount{
{
Name: "unused",
MountPath: "/opt/unused/mountpoint",
},
},
},
CommonDefaultableParams: vmv1beta1.CommonDefaultableParams{
UseStrictSecurity: ptr.To(false),
},
RetentionPeriod: "1",
RemovePvcAfterDelete: true,
StorageDataPath: "/custom-path/internal/dir",
Storage: &corev1.PersistentVolumeClaimSpec{},
VMBackup: &vmv1beta1.VMBackup{
AcceptEULA: true,
Destination: "fs:///opt/backup",
VolumeMounts: []corev1.VolumeMount{{Name: "backup", MountPath: "/opt/backup"}},
},
},
},
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(2))
Expect(ts.Volumes).To(HaveLen(3))
Expect(ts.Containers[0].VolumeMounts).To(HaveLen(2))
Expect(ts.Containers[0].VolumeMounts[0].Name).To(Equal("data"))
Expect(ts.Containers[1].VolumeMounts).To(HaveLen(2))
Expect(ts.Containers[1].VolumeMounts[0].Name).To(Equal("data"))
Expect(ts.Containers[1].VolumeMounts[1].Name).To(Equal("backup"))
}),
)

baseSingle := &vmv1beta1.VMSingle{
Expand Down

0 comments on commit fc5fe87

Please sign in to comment.