From 8b65eb6126e1d9c8a8ff2438dbeaca4dbd2b472b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Han?= <seb@redhat.com>
Date: Mon, 1 Feb 2021 18:23:19 +0100
Subject: [PATCH] ceph: do not enable ShareProcessNamespace if HostPID
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If HostPID is enabled, this means the Ceph version is older than Octopus
and we are still running OSD with HostPID due to an internal
inter-process check.
If the log collector is enabled we need to enable ShareProcessNamespace
**only** if HostIPC is disabled.

Both HostPID and ShareProcessNamespace cannot be enabled.

Signed-off-by: Sébastien Han <seb@redhat.com>
---
 pkg/operator/ceph/cluster/osd/spec.go      |  6 ++--
 pkg/operator/ceph/cluster/osd/spec_test.go | 36 ++++++++++++++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/pkg/operator/ceph/cluster/osd/spec.go b/pkg/operator/ceph/cluster/osd/spec.go
index a2d93e31f..c13cea384 100644
--- a/pkg/operator/ceph/cluster/osd/spec.go
+++ b/pkg/operator/ceph/cluster/osd/spec.go
@@ -548,9 +548,9 @@ func (c *Cluster) makeDeployment(osdProps osdProperties, osd OSDInfo, provisionC
 
 	// If the log collector is enabled we add the side-car container
 	if c.spec.LogCollector.Enabled {
-		shareProcessNamespace := true
-		// If HostIPC is already enabled we don't need to activate shareProcessNamespace since all pods already see each others
-		if !podTemplateSpec.Spec.HostIPC {
+		// If HostPID is already enabled we don't need to activate shareProcessNamespace since all pods already see each others
+		if !podTemplateSpec.Spec.HostPID {
+			shareProcessNamespace := true
 			podTemplateSpec.Spec.ShareProcessNamespace = &shareProcessNamespace
 		}
 		podTemplateSpec.Spec.Containers = append(podTemplateSpec.Spec.Containers, *controller.LogCollectorContainer(fmt.Sprintf("ceph-osd.%s", osdID), c.clusterInfo.Namespace, c.spec))
diff --git a/pkg/operator/ceph/cluster/osd/spec_test.go b/pkg/operator/ceph/cluster/osd/spec_test.go
index 60d927f48..7c3d81fe8 100644
--- a/pkg/operator/ceph/cluster/osd/spec_test.go
+++ b/pkg/operator/ceph/cluster/osd/spec_test.go
@@ -407,6 +407,42 @@ func testPodDevices(t *testing.T, dataDir, deviceName string, allDevices bool) {
 	for _, flag := range defaultTuneSlowSettings {
 		assert.Contains(t, deployment.Spec.Template.Spec.Containers[0].Args, flag)
 	}
+
+	// Test shareProcessNamespace presence
+	assert.True(t, deployment.Spec.Template.Spec.HostPID)
+	if deployment.Spec.Template.Spec.ShareProcessNamespace != nil {
+		panic("ShareProcessNamespace should be nil")
+	}
+
+	// Turn on log collector
+	c.spec.LogCollector.Enabled = true
+	deployment, err = c.makeDeployment(osdProp, osd, dataPathMap)
+	assert.NoError(t, err)
+	assert.True(t, deployment.Spec.Template.Spec.HostPID, deployment.Spec.Template.Spec.HostPID)
+	if deployment.Spec.Template.Spec.ShareProcessNamespace != nil {
+		panic("ShareProcessNamespace should be nil")
+	}
+
+	// Test hostPID and ShareProcessNamespace
+	{
+		// now set ceph version to nautilus
+		clusterInfo := &cephclient.ClusterInfo{
+			Namespace:   "ns",
+			CephVersion: cephver.Octopus,
+		}
+		c := New(context, clusterInfo, spec, "rook/rook:myversion")
+		deployment, err = c.makeDeployment(osdProp, osd, dataPathMap)
+		assert.NoError(t, err)
+		assert.False(t, deployment.Spec.Template.Spec.HostPID, deployment.Spec.Template.Spec.HostPID)
+
+		// Turn on log collector
+		c.spec.LogCollector.Enabled = true
+		deployment, err = c.makeDeployment(osdProp, osd, dataPathMap)
+		assert.NoError(t, err)
+		shareProcessNamespace := *deployment.Spec.Template.Spec.ShareProcessNamespace
+		assert.True(t, shareProcessNamespace)
+	}
+
 }
 
 func verifyEnvVar(t *testing.T, envVars []v1.EnvVar, expectedName, expectedValue string, expectedFound bool) {