From db614473c267a698e6c823d8635f0bb4abb5f76c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Han?= Date: Mon, 2 Dec 2019 17:11:37 +0100 Subject: [PATCH] ceph: fix race condition on crd child update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We recently discovered a race condition which happens not to update the child CRDs when CR image changes. The race happens under the following sequence: * orchestration onAdd() is called when the operator image restarts or its image updated * the orchestration then goes into processing mon/mgr/osd * during this the cluster CR is updated so onUpdate() is called, however, onAdd() is not done yet for child CRDs (mds, rgw) * onUpdate() runs against the cluster CR and updates the cluster images * by the time the child CRDs reached onUpdate() from their respective ParentClusterChanged() methods, the image is already up to date so we were exiting since our check for updating these resources are based on the cluster image version To fix this, we changed the comparison, but using isUpgrade instead and slightly changed the way isUpgrade operates. So now, isUpgrade is true even if the Ceph version is identical which allows us to verify whether or not the image changed. Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1775624 Signed-off-by: Sébastien Han --- pkg/operator/ceph/cluster/cluster.go | 3 ++- pkg/operator/ceph/cluster/cluster_test.go | 2 +- pkg/operator/ceph/cluster/mon/spec.go | 1 - pkg/operator/ceph/file/controller.go | 2 +- pkg/operator/ceph/nfs/controller.go | 2 +- pkg/operator/ceph/object/controller.go | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/operator/ceph/cluster/cluster.go b/pkg/operator/ceph/cluster/cluster.go index c2171b5b7..df7344388 100755 --- a/pkg/operator/ceph/cluster/cluster.go +++ b/pkg/operator/ceph/cluster/cluster.go @@ -401,9 +401,10 @@ func diffImageSpecAndClusterRunningVersion(imageSpecVersion cephver.CephVersion, clusterRunningVersion := *version // If this is the same version + // We return 'true' so that we restart using the Ceph checks if cephver.IsIdentical(clusterRunningVersion, imageSpecVersion) { logger.Debugf("both cluster and image spec versions are identical, doing nothing %s", imageSpecVersion.String()) - return false, nil + return true, nil } if cephver.IsSuperior(imageSpecVersion, clusterRunningVersion) { diff --git a/pkg/operator/ceph/cluster/cluster_test.go b/pkg/operator/ceph/cluster/cluster_test.go index d8ecb7839..16265d471 100755 --- a/pkg/operator/ceph/cluster/cluster_test.go +++ b/pkg/operator/ceph/cluster/cluster_test.go @@ -109,7 +109,7 @@ func TestDiffImageSpecAndClusterRunningVersion(t *testing.T) { m, err = diffImageSpecAndClusterRunningVersion(fakeImageVersion, dummyRunningVersions5) assert.NoError(t, err) - assert.False(t, m) + assert.True(t, m) } func TestMinVersion(t *testing.T) { diff --git a/pkg/operator/ceph/cluster/mon/spec.go b/pkg/operator/ceph/cluster/mon/spec.go index 7c72c1616..b6aae760a 100644 --- a/pkg/operator/ceph/cluster/mon/spec.go +++ b/pkg/operator/ceph/cluster/mon/spec.go @@ -330,7 +330,6 @@ func UpdateCephDeploymentAndWait(context *clusterd.Context, deployment *apps.Dep callback := func(action string) error { if !isUpgrade { - logger.Info("this is not a Ceph upgrade, not performing upgrade checks") return nil } diff --git a/pkg/operator/ceph/file/controller.go b/pkg/operator/ceph/file/controller.go index 73c00916d..5afdc3691 100644 --- a/pkg/operator/ceph/file/controller.go +++ b/pkg/operator/ceph/file/controller.go @@ -164,7 +164,7 @@ func (c *FilesystemController) onUpdate(oldObj, newObj interface{}) { // ParentClusterChanged determines wether or not a CR update has been sent func (c *FilesystemController) ParentClusterChanged(cluster cephv1.ClusterSpec, clusterInfo *cephconfig.ClusterInfo, isUpgrade bool) { c.clusterInfo = clusterInfo - if cluster.CephVersion.Image == c.clusterSpec.CephVersion.Image { + if !isUpgrade { logger.Debugf("No need to update the file system after the parent cluster changed") return } diff --git a/pkg/operator/ceph/nfs/controller.go b/pkg/operator/ceph/nfs/controller.go index e35e0d588..3a781e957 100644 --- a/pkg/operator/ceph/nfs/controller.go +++ b/pkg/operator/ceph/nfs/controller.go @@ -165,7 +165,7 @@ func (c *CephNFSController) onDelete(obj interface{}) { // cluster has changed. func (c *CephNFSController) ParentClusterChanged(cluster cephv1.ClusterSpec, clusterInfo *cephconfig.ClusterInfo, isUpgrade bool) { c.clusterInfo = clusterInfo - if cluster.CephVersion.Image == c.clusterSpec.CephVersion.Image || !c.clusterInfo.CephVersion.IsAtLeastNautilus() { + if !isUpgrade || !c.clusterInfo.CephVersion.IsAtLeastNautilus() { logger.Debugf("No need to update the nfs daemons after the parent cluster changed") return } diff --git a/pkg/operator/ceph/object/controller.go b/pkg/operator/ceph/object/controller.go index 3f2d3d0f3..77025f840 100644 --- a/pkg/operator/ceph/object/controller.go +++ b/pkg/operator/ceph/object/controller.go @@ -192,7 +192,7 @@ func (c *ObjectStoreController) onDelete(obj interface{}) { // ParentClusterChanged determines wether or not a CR update has been sent func (c *ObjectStoreController) ParentClusterChanged(cluster cephv1.ClusterSpec, clusterInfo *daemonconfig.ClusterInfo, isUpgrade bool) { c.clusterInfo = clusterInfo - if cluster.CephVersion.Image == c.clusterSpec.CephVersion.Image { + if !isUpgrade { logger.Debugf("No need to update the object store after the parent cluster changed") return }