From b11fcc8c39ce6f56c43a459cf98a762af105efdc Mon Sep 17 00:00:00 2001 From: Zoran Rajic Date: Thu, 3 Aug 2023 03:04:54 -0700 Subject: [PATCH] PWX-32428,PWX-27672: shared mounts fix for PKS and privileged:false * fixes shared mounts for PKS (PWX-32428, regression introduced via PWX-31842) * fixed shared mounts for privileged:false (PWX-27672) Signed-off-by: Zoran Rajic --- drivers/storage/portworx/deployment.go | 95 +++++++++--- drivers/storage/portworx/deployment_test.go | 136 +++++++++++++++++- drivers/storage/portworx/testspec/pks.yaml | 7 +- .../storage/portworx/testspec/pks_2.9.1.yaml | 9 +- .../portworx/testspec/px_pks_with_csi.yaml | 7 +- pkg/migration/generate.go | 1 + 6 files changed, 223 insertions(+), 32 deletions(-) diff --git a/drivers/storage/portworx/deployment.go b/drivers/storage/portworx/deployment.go index eaacf0a2ff..94f6e09fc0 100644 --- a/drivers/storage/portworx/deployment.go +++ b/drivers/storage/portworx/deployment.go @@ -48,6 +48,16 @@ const ( SecretKeyKvdbACLToken = "acl-token" ) +var ( + pxVer2_3_2, _ = version.NewVersion("2.3.2") + pxVer2_5_5, _ = version.NewVersion("2.5.5") + pxVer2_6, _ = version.NewVersion("2.6") + pxVer2_8, _ = version.NewVersion("2.8") + pxVer2_9_1, _ = version.NewVersion("2.9.1") + pxVer3_0, _ = version.NewVersion("3.0") + pxVer3_0_1, _ = version.NewVersion("3.0.1") +) + type volumeInfo struct { name string hostPath string // The path on the host @@ -63,8 +73,9 @@ type volumeInfo struct { } type pksVolumeInfo struct { - hostPath string - mountPath string + hostPath string + mountPath string + mountPropagation *v1.MountPropagationMode } var ( @@ -290,6 +301,14 @@ func (p *portworx) GetStoragePodSpec( t.cloudConfig = cloudConfig } + if _, has := t.cluster.Annotations[pxutil.AnnotationIsPrivileged]; has { + if pxutil.GetPortworxVersion(cluster).LessThan(pxVer3_0_1) { + err = fmt.Errorf("failed to create pod spec: need portworx %s or higher to use annotation '%s'", + pxVer3_0_1, pxutil.AnnotationIsPrivileged) + return v1.PodSpec{}, err + } + } + containers := t.portworxContainer(cluster) podSpec := v1.PodSpec{ HostPID: pxutil.IsHostPidEnabled(cluster), @@ -942,9 +961,8 @@ func (t *template) getArguments() []string { } } else if t.cluster.Spec.CloudStorage != nil { - pxVer, _ := version.NewVersion("2.8") // Cloud provider parameter was added in newer version. - if t.pxVersion.GreaterThanOrEqual(pxVer) { + if t.pxVersion.GreaterThanOrEqual(pxVer2_8) { cloudProvider := t.getCloudProvider() if len(cloudProvider) > 0 { args = append(args, "-cloud_provider", cloudProvider) @@ -1016,6 +1034,14 @@ func (t *template) getArguments() []string { if t.cluster.Annotations[pxutil.AnnotationLogFile] != "" { args = append(args, "--log", t.cluster.Annotations[pxutil.AnnotationLogFile]) } + // for non-privileged and PKS, add shared mounts via parameters + if t.pxVersion.GreaterThanOrEqual(pxVer3_0_1) && + (!pxutil.IsPrivileged(t.cluster) || pxutil.IsPKS(t.cluster)) { + args = append(args, + "-v", "/var/lib/osd/pxns:/var/lib/osd/pxns:shared", + "-v", "/var/lib/osd/mounts:/var/lib/osd/mounts:shared", + ) + } rtOpts := make([]string, 0) for k, v := range t.cluster.Spec.RuntimeOpts { @@ -1068,7 +1094,6 @@ func (t *template) getArguments() []string { marketplaceName := strings.TrimSpace(os.Getenv(pxutil.EnvKeyMarketplaceName)) if marketplaceName != "" { - pxVer2_5_5, _ := version.NewVersion("2.5.5") if t.pxVersion.GreaterThanOrEqual(pxVer2_5_5) { args = append(args, "-marketplace_name", marketplaceName) } @@ -1107,7 +1132,6 @@ func (t *template) getEnvList() []v1.EnvVar { }, } - pxVer2_6, _ := version.NewVersion("2.6") if t.pxVersion.LessThan(pxVer2_6) { envMap["AUTO_NODE_RECOVERY_TIMEOUT_IN_SECS"] = &v1.EnvVar{ Name: "AUTO_NODE_RECOVERY_TIMEOUT_IN_SECS", @@ -1128,9 +1152,13 @@ func (t *template) getEnvList() []v1.EnvVar { } if t.isPKS { + ev := "if [ ! -x /bin/systemctl ]; then apt-get update; apt-get install -y systemd; fi" + if t.pxVersion.GreaterThanOrEqual(pxVer3_0_1) { + ev = "rm -fr /var/lib/osd/driver" + } envMap["PRE-EXEC"] = &v1.EnvVar{ Name: "PRE-EXEC", - Value: "rm -fr /var/lib/osd/driver", + Value: ev, } } @@ -1156,7 +1184,6 @@ func (t *template) getEnvList() []v1.EnvVar { } if t.cluster.Spec.ImagePullSecret != nil && *t.cluster.Spec.ImagePullSecret != "" { - pxVer2_3_2, _ := version.NewVersion("2.3.2") if t.pxVersion.LessThan(pxVer2_3_2) { envMap["REGISTRY_CONFIG"] = &v1.EnvVar{ Name: "REGISTRY_CONFIG", @@ -1288,8 +1315,7 @@ func (t *template) getVolumeMounts() []v1.VolumeMount { if t.cluster.Annotations != nil { preFltCheck = strings.TrimSpace(strings.ToLower(t.cluster.Annotations[pxutil.AnnotationPreflightCheck])) } - pxVer30, _ := version.NewVersion("3.0") - if t.pxVersion.GreaterThanOrEqual(pxVer30) && preFltCheck != "true" { + if t.pxVersion.GreaterThanOrEqual(pxVer3_0) && preFltCheck != "true" { extensions = append(extensions, t.getTelemetryPhoneHomeVolumeInfoList) } for _, fn := range extensions { @@ -1316,8 +1342,18 @@ func (t *template) mountsFromVolInfo(vols []volumeInfo) []v1.VolumeMount { v.hostPath, v.mountPath, pxutil.AnnotationIsPrivileged) volMount.MountPropagation = nil } - if t.isPKS && v.pks != nil && v.pks.mountPath != "" { - volMount.MountPath = v.pks.mountPath + + if t.isPKS && v.pks != nil { + if v.pks.mountPath != "" { + volMount.MountPath = v.pks.mountPath + } + if v.pks.mountPropagation != nil { + if *(v.pks.mountPropagation) == "-" { + volMount.MountPropagation = nil + } else { + volMount.MountPropagation = v.pks.mountPropagation + } + } } if volMount.MountPath != "" { volumeMounts = append(volumeMounts, volMount) @@ -1365,8 +1401,7 @@ func (t *template) getVolumes() []v1.Volume { if t.cluster.Annotations != nil { preFltCheck = strings.TrimSpace(strings.ToLower(t.cluster.Annotations[pxutil.AnnotationPreflightCheck])) } - pxVer30, _ := version.NewVersion("3.0") - if t.pxVersion.GreaterThanOrEqual(pxVer30) && preFltCheck != "true" { + if t.pxVersion.GreaterThanOrEqual(pxVer3_0) && preFltCheck != "true" { extensions = append(extensions, t.getTelemetryPhoneHomeVolumeInfoList) } @@ -1817,17 +1852,35 @@ func getDefaultVolumeInfoList(pxVersion *version.Version) []volumeInfo { // getCommonVolumeList returns a common list of volumes across all containers func getCommonVolumeList(pxVersion *version.Version) []volumeInfo { list := make([]volumeInfo, 0) - pxVer2_9_1, _ := version.NewVersion("2.9.1") - if pxVersion.GreaterThanOrEqual(pxVer2_9_1) { + + if pxVersion.GreaterThanOrEqual(pxVer3_0_1) { list = append(list, volumeInfo{ - name: "varlibosd", - hostPath: "/var/lib/osd", - mountPath: "/var/lib/osd", + name: "varlibosd", + hostPath: "/var/lib/osd", + mountPath: "/var/lib/osd", + mountPropagation: mountPropagationModePtr(v1.MountPropagationBidirectional), pks: &pksVolumeInfo{ - hostPath: "/var/vcap/store/lib/osd", + hostPath: "/var/vcap/store/lib/osd", + mountPropagation: mountPropagationModePtr(v1.MountPropagationMode("-")), }, - mountPropagation: mountPropagationModePtr(v1.MountPropagationBidirectional), }) + } else { + list = append(list, volumeInfo{ + name: "pxlogs", + pks: &pksVolumeInfo{ + mountPath: "/var/lib/osd/log", + hostPath: "/var/vcap/store/lib/osd/log", + }, + }) + + if pxVersion.GreaterThanOrEqual(pxVer2_9_1) { + list = append(list, volumeInfo{ + name: "varlibosd", + hostPath: "/var/lib/osd", + mountPath: "/var/lib/osd", + mountPropagation: mountPropagationModePtr(v1.MountPropagationBidirectional), + }) + } } list = append(list, []volumeInfo{ diff --git a/drivers/storage/portworx/deployment_test.go b/drivers/storage/portworx/deployment_test.go index e4b6f8353c..1691d4d565 100644 --- a/drivers/storage/portworx/deployment_test.go +++ b/drivers/storage/portworx/deployment_test.go @@ -2539,6 +2539,40 @@ func TestPKSPodSpec(t *testing.T) { assert.NoError(t, err, "Unexpected error on GetStoragePodSpec") assertPodSpecEqual(t, expected, &actual) + + // check PKS when using px-2.9.1 + + cluster.Spec.Image = "portworx/oci-monitor:2.9.1" + expected_2_9_1 := expected.DeepCopy() + expected_2_9_1.Containers[0].Image = "docker.io/" + cluster.Spec.Image + podSpecAddMount(expected_2_9_1, "varlibosd", "/var/lib/osd:/var/lib/osd:shared") + podSpecRemoveEnv(expected_2_9_1, "AUTO_NODE_RECOVERY_TIMEOUT_IN_SECS") + + actual, err = driver.GetStoragePodSpec(cluster, nodeName) + assert.NoError(t, err, "Unexpected error on GetStoragePodSpec") + assertPodSpecEqual(t, expected_2_9_1, &actual) + + // check PKS when using px-3.0.1 + + cluster.Spec.Image = "portworx/oci-monitor:3.0.1" + expected_3_0_1 := expected_2_9_1.DeepCopy() + expected_3_0_1.Containers[0].Image = "docker.io/" + cluster.Spec.Image + expected_3_0_1.Containers[0].Args = append(expected_3_0_1.Containers[0].Args, + "-v", "/var/lib/osd/pxns:/var/lib/osd/pxns:shared", + "-v", "/var/lib/osd/mounts:/var/lib/osd/mounts:shared", + ) + podSpecRemoveMount(expected_3_0_1, "varlibosd") + podSpecRemoveMount(expected_3_0_1, "pxlogs") + podSpecAddMount(expected_3_0_1, "varlibosd", "/var/vcap/store/lib/osd:/var/lib/osd") + podSpecRemoveEnv(expected_3_0_1, "PRE-EXEC") + expected_3_0_1.Containers[0].Env = append(expected_3_0_1.Containers[0].Env, v1.EnvVar{ + Name: "PRE-EXEC", Value: "rm -fr /var/lib/osd/driver", + }) + + actual, err = driver.GetStoragePodSpec(cluster, nodeName) + assert.NoError(t, err, "Unexpected error on GetStoragePodSpec") + + assertPodSpecEqual(t, expected_3_0_1, &actual) } func TestOpenshiftRuncPodSpec(t *testing.T) { @@ -2602,16 +2636,39 @@ func TestOpenshiftRuncPodSpec(t *testing.T) { // Test securityContxt w/ privileged=false cluster.ObjectMeta.Annotations[pxutil.AnnotationIsPrivileged] = "false" + + actual, err = driver.GetStoragePodSpec(cluster, nodeName) + require.Error(t, err) + assert.Contains(t, err.Error(), "need portworx 3.0.1 or higher to use annotation '"+ + pxutil.AnnotationIsPrivileged+"'") + + cluster.Spec.Image = "portworx/oci-monitor:3.0.1" + expected_3_0_1 := expected.DeepCopy() + expected_3_0_1.Containers[0].Image = "docker.io/" + cluster.Spec.Image + expected_3_0_1.Containers[0].Args = append(expected_3_0_1.Containers[0].Args, + "-v", "/var/lib/osd/pxns:/var/lib/osd/pxns:shared", + "-v", "/var/lib/osd/mounts:/var/lib/osd/mounts:shared", + ) + expected_3_0_1.Containers[0].SecurityContext = &v1.SecurityContext{ + Privileged: boolPtr(false), + Capabilities: &v1.Capabilities{ + Add: []v1.Capability{ + "SYS_ADMIN", "SYS_PTRACE", "SYS_RAWIO", "SYS_MODULE", "LINUX_IMMUTABLE", + }, + }, + } + podSpecAddMount(expected_3_0_1, "varlibosd", "/var/lib/osd:/var/lib/osd") + podSpecRemoveEnv(expected_3_0_1, "AUTO_NODE_RECOVERY_TIMEOUT_IN_SECS") + actual, err = driver.GetStoragePodSpec(cluster, nodeName) require.NoError(t, err) - pxc := actual.Containers[0] - require.Equal(t, "portworx", pxc.Name) - require.NotNil(t, pxc.SecurityContext.Privileged) - assert.False(t, *pxc.SecurityContext.Privileged) - require.NotNil(t, pxc.SecurityContext.Capabilities) - assert.Equal(t, 5, len(pxc.SecurityContext.Capabilities.Add)) - assert.Empty(t, pxc.SecurityContext.Capabilities.Drop) + assertPodSpecEqual(t, expected_3_0_1, &actual) + + require.Equal(t, 1, len(actual.Containers)) + for _, v := range actual.Containers[0].VolumeMounts { + assert.Nil(t, v.MountPropagation, "Wrong propagation on %v", v) + } } func TestPodSpecForK3s(t *testing.T) { @@ -4324,3 +4381,68 @@ func assertContainerEqual(t *testing.T, expected, actual v1.Container) { assert.ElementsMatch(t, expected.Env, actual.Env) assert.ElementsMatch(t, expected.VolumeMounts, actual.VolumeMounts) } + +func podSpecAddMount(ps *v1.PodSpec, name, mntSpec string) { + if ps == nil || name == "" || mntSpec == "" { + return + } + parts := strings.Split(mntSpec, ":") + src := v1.Volume{ + Name: name, + VolumeSource: v1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{ + Path: parts[0], + }, + }, + } + dst := v1.VolumeMount{ + Name: name, + MountPath: parts[1], + } + if len(parts) > 2 { + if parts[2] == "shared" || parts[2] == "rshared" { + dst.MountPropagation = mountPropagationModePtr(v1.MountPropagationBidirectional) + } + } + ps.Volumes = append(ps.Volumes, src) + ps.Containers[0].VolumeMounts = append(ps.Containers[0].VolumeMounts, dst) +} + +func podSpecRemoveMount(ps *v1.PodSpec, name string) { + if ps == nil || name == "" { + return + } + + newVols := make([]v1.Volume, 0, len(ps.Volumes)) + for _, v := range ps.Volumes { + if v.Name == name { + continue + } + newVols = append(newVols, v) + } + ps.Volumes = newVols + + newVolMmounts := make([]v1.VolumeMount, 0, len(ps.Containers[0].VolumeMounts)) + for _, v := range ps.Containers[0].VolumeMounts { + if v.Name == name { + continue + } + newVolMmounts = append(newVolMmounts, v) + } + ps.Containers[0].VolumeMounts = newVolMmounts +} + +func podSpecRemoveEnv(ps *v1.PodSpec, name string) { + if ps == nil || name == "" { + return + } + + newEnv := make([]v1.EnvVar, 0, len(ps.Containers[0].Env)) + for _, v := range ps.Containers[0].Env { + if v.Name == name { + continue + } + newEnv = append(newEnv, v) + } + ps.Containers[0].Env = newEnv +} diff --git a/drivers/storage/portworx/testspec/pks.yaml b/drivers/storage/portworx/testspec/pks.yaml index dfcdd3b89b..bfec825295 100644 --- a/drivers/storage/portworx/testspec/pks.yaml +++ b/drivers/storage/portworx/testspec/pks.yaml @@ -51,7 +51,7 @@ spec: - name: "PX_TEMPLATE_VERSION" value: "v4" - name: "PRE-EXEC" - value: "rm -fr /var/lib/osd/driver" + value: "if [ ! -x /bin/systemctl ]; then apt-get update; apt-get install -y systemd; fi" livenessProbe: periodSeconds: 30 initialDelaySeconds: 840 # allow image pull in slow networks @@ -85,6 +85,8 @@ spec: mountPath: /etc/pwx - name: optpwx mountPath: /opt/pwx + - name: pxlogs + mountPath: /var/lib/osd/log - name: procmount mountPath: /host_proc - name: sysdmount @@ -127,6 +129,9 @@ spec: - name: optpwx hostPath: path: /var/vcap/store/opt/pwx + - name: pxlogs + hostPath: + path: /var/vcap/store/lib/osd/log - name: procmount hostPath: path: /proc diff --git a/drivers/storage/portworx/testspec/pks_2.9.1.yaml b/drivers/storage/portworx/testspec/pks_2.9.1.yaml index 789eb41d75..661e2aefa4 100644 --- a/drivers/storage/portworx/testspec/pks_2.9.1.yaml +++ b/drivers/storage/portworx/testspec/pks_2.9.1.yaml @@ -38,7 +38,7 @@ spec: - name: "PX_TEMPLATE_VERSION" value: "v4" - name: "PRE-EXEC" - value: "rm -fr /var/lib/osd/driver" + value: "if [ ! -x /bin/systemctl ]; then apt-get update; apt-get install -y systemd; fi" livenessProbe: periodSeconds: 30 initialDelaySeconds: 840 # allow image pull in slow networks @@ -75,6 +75,8 @@ spec: - name: varlibosd mountPath: /var/lib/osd mountPropagation: Bidirectional + - name: pxlogs + mountPath: /var/lib/osd/log - name: procmount mountPath: /host_proc - name: sysdmount @@ -119,7 +121,10 @@ spec: path: /var/vcap/store/opt/pwx - name: varlibosd hostPath: - path: /var/vcap/store/lib/osd + path: /var/lib/osd + - name: pxlogs + hostPath: + path: /var/vcap/store/lib/osd/log - name: procmount hostPath: path: /proc diff --git a/drivers/storage/portworx/testspec/px_pks_with_csi.yaml b/drivers/storage/portworx/testspec/px_pks_with_csi.yaml index 395815c2b4..52e7bb1a4b 100644 --- a/drivers/storage/portworx/testspec/px_pks_with_csi.yaml +++ b/drivers/storage/portworx/testspec/px_pks_with_csi.yaml @@ -38,7 +38,7 @@ spec: - name: "PX_TEMPLATE_VERSION" value: "v4" - name: "PRE-EXEC" - value: "rm -fr /var/lib/osd/driver" + value: "if [ ! -x /bin/systemctl ]; then apt-get update; apt-get install -y systemd; fi" - name: CSI_ENDPOINT value: unix:///var/vcap/data/kubelet/csi-plugins/pxd.portworx.com/csi.sock livenessProbe: @@ -74,6 +74,8 @@ spec: mountPath: /etc/pwx - name: optpwx mountPath: /opt/pwx + - name: pxlogs + mountPath: /var/lib/osd/log - name: procmount mountPath: /host_proc - name: sysdmount @@ -135,6 +137,9 @@ spec: - name: optpwx hostPath: path: /var/vcap/store/opt/pwx + - name: pxlogs + hostPath: + path: /var/vcap/store/lib/osd/log - name: registration-dir hostPath: path: /var/vcap/data/kubelet/plugins_registry diff --git a/pkg/migration/generate.go b/pkg/migration/generate.go index 864e8faa3a..87cb39886b 100644 --- a/pkg/migration/generate.go +++ b/pkg/migration/generate.go @@ -64,6 +64,7 @@ var ( "optpwx": true, "osddriver": true, "procmount": true, + "pxlogs": true, "src": true, "sysdmount": true, }