From 4237aa7ecc953056eff61f1eb249a3ca999e46fb Mon Sep 17 00:00:00 2001 From: Santosh Pillai Date: Tue, 15 Dec 2020 16:18:59 +0530 Subject: [PATCH] ceph: configure health check timeout for OSDs when upgrading added waitTimeoutForHealthyOSD in the cluster crd that defines the time (in minutes) the operator would wait before an OSD can be stopped for upgrade. This PR also removes the ok-to-continue logic for osds as its already handled by ok-to-stop. The default value is 10 minutes. Signed-off-by: Santosh Pillai --- .../charts/rook-ceph/templates/resources.yaml | 4 ++ cluster/examples/kubernetes/ceph/cluster.yaml | 6 +++ cluster/examples/kubernetes/ceph/crds.yaml | 2 + .../kubernetes/ceph/pre-k8s-1.16/crds.yaml | 2 + pkg/apis/ceph.rook.io/v1/types.go | 7 +++ pkg/daemon/ceph/client/info.go | 4 +- pkg/daemon/ceph/client/upgrade.go | 48 +++++++++---------- pkg/daemon/ceph/client/upgrade_test.go | 47 ++++++++++++++++++ pkg/operator/ceph/cluster/osd/osd.go | 17 +++++-- pkg/operator/k8sutil/deployment.go | 6 +-- 10 files changed, 109 insertions(+), 34 deletions(-) diff --git a/cluster/charts/rook-ceph/templates/resources.yaml b/cluster/charts/rook-ceph/templates/resources.yaml index f47311e8b..5ff481ad8 100644 --- a/cluster/charts/rook-ceph/templates/resources.yaml +++ b/cluster/charts/rook-ceph/templates/resources.yaml @@ -63,6 +63,8 @@ spec: type: boolean continueUpgradeAfterChecksEvenIfNotHealthy: type: boolean + waitTimeoutForHealthyOSDInMinutes: + type: integer mon: type: object properties: @@ -1411,6 +1413,8 @@ spec: type: boolean continueUpgradeAfterChecksEvenIfNotHealthy: type: boolean + waitTimeoutForHealthyOSDInMinutes: + type: integer mon: properties: allowMultiplePerNode: diff --git a/cluster/examples/kubernetes/ceph/cluster.yaml b/cluster/examples/kubernetes/ceph/cluster.yaml index 0fda1e94b..77b9e23ff 100644 --- a/cluster/examples/kubernetes/ceph/cluster.yaml +++ b/cluster/examples/kubernetes/ceph/cluster.yaml @@ -37,6 +37,12 @@ spec: skipUpgradeChecks: false # Whether or not continue if PGs are not clean during an upgrade continueUpgradeAfterChecksEvenIfNotHealthy: false + # WaitTimeoutForHealthyOSDInMinutes defines the time (in minutes) the operator would wait before an OSD can be stopped for upgrade or restart. + # If the timeout exceeds and OSD is not ok to stop, then the operator would skip upgrade for the current OSD and proceed with the next one + # if `continueUpgradeAfterChecksEvenIfNotHealthy` is `false`. If `continueUpgradeAfterChecksEvenIfNotHealthy` is `true`, then opertor would + # continue with the upgrade of an OSD even if its not ok to stop after the timeout. This timeout won't be applied if `skipUpgradeChecks` is `true`. + # The default wait timeout is 10 minutes. + waitTimeoutForHealthyOSDInMinutes: 10 mon: # Set the number of mons to be started. Must be an odd number, and is generally recommended to be 3. count: 3 diff --git a/cluster/examples/kubernetes/ceph/crds.yaml b/cluster/examples/kubernetes/ceph/crds.yaml index 02e511461..3fa604a27 100644 --- a/cluster/examples/kubernetes/ceph/crds.yaml +++ b/cluster/examples/kubernetes/ceph/crds.yaml @@ -65,6 +65,8 @@ spec: type: boolean continueUpgradeAfterChecksEvenIfNotHealthy: type: boolean + waitTimeoutForHealthyOSDInMinutes: + type: integer mon: type: object properties: diff --git a/cluster/examples/kubernetes/ceph/pre-k8s-1.16/crds.yaml b/cluster/examples/kubernetes/ceph/pre-k8s-1.16/crds.yaml index 1c2aec7f8..dd4945515 100644 --- a/cluster/examples/kubernetes/ceph/pre-k8s-1.16/crds.yaml +++ b/cluster/examples/kubernetes/ceph/pre-k8s-1.16/crds.yaml @@ -73,6 +73,8 @@ spec: type: boolean continueUpgradeAfterChecksEvenIfNotHealthy: type: boolean + waitTimeoutForHealthyOSDInMinutes: + type: integer mon: properties: allowMultiplePerNode: diff --git a/pkg/apis/ceph.rook.io/v1/types.go b/pkg/apis/ceph.rook.io/v1/types.go index 091bf9294..20eb59f58 100755 --- a/pkg/apis/ceph.rook.io/v1/types.go +++ b/pkg/apis/ceph.rook.io/v1/types.go @@ -98,6 +98,13 @@ type ClusterSpec struct { // ContinueUpgradeAfterChecksEvenIfNotHealthy defines if an upgrade should continue even if PGs are not clean ContinueUpgradeAfterChecksEvenIfNotHealthy bool `json:"continueUpgradeAfterChecksEvenIfNotHealthy,omitempty"` + // WaitTimeoutForHealthyOSDInMinutes defines the time the operator would wait before an OSD can be stopped for upgrade or restart. + // If the timeout exceeds and OSD is not ok to stop, then the operator would skip upgrade for the current OSD and proceed with the next one + // if `continueUpgradeAfterChecksEvenIfNotHealthy` is `false`. If `continueUpgradeAfterChecksEvenIfNotHealthy` is `true`, then opertor would + // continue with the upgrade of an OSD even if its not ok to stop after the timeout. This timeout won't be applied if `skipUpgradeChecks` is `true`. + // The default wait timeout is 10 minutes. + WaitTimeoutForHealthyOSDInMinutes time.Duration `json:"waitTimeoutForHealthyOSDInMinutes,omitempty"` + // A spec for configuring disruption management. DisruptionManagement DisruptionManagementSpec `json:"disruptionManagement,omitempty"` diff --git a/pkg/daemon/ceph/client/info.go b/pkg/daemon/ceph/client/info.go index 818d77074..8eaf54b08 100644 --- a/pkg/daemon/ceph/client/info.go +++ b/pkg/daemon/ceph/client/info.go @@ -19,6 +19,7 @@ package client import ( "fmt" "net" + "time" cephver "github.com/rook/rook/pkg/operator/ceph/version" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -37,7 +38,8 @@ type ClusterInfo struct { OwnerRef metav1.OwnerReference // Hide the name of the cluster since in 99% of uses we want to use the cluster namespace. // If the CR name is needed, access it through the NamespacedName() method. - name string + name string + OsdUpgradeTimeout time.Duration } // MonInfo is a collection of information about a Ceph mon. diff --git a/pkg/daemon/ceph/client/upgrade.go b/pkg/daemon/ceph/client/upgrade.go index 679507f56..4bccf2592 100644 --- a/pkg/daemon/ceph/client/upgrade.go +++ b/pkg/daemon/ceph/client/upgrade.go @@ -27,6 +27,12 @@ import ( "github.com/rook/rook/pkg/util" ) +const ( + defaultMaxRetries = 10 + defaultRetryDelay = 60 * time.Second + defaultOSDRetryDelay = 10 * time.Second +) + // CephDaemonsVersions is a structure that can be used to parsed the output of the 'ceph versions' command type CephDaemonsVersions struct { Mon map[string]int `json:"mon,omitempty"` @@ -131,6 +137,7 @@ func EnableReleaseOSDFunctionality(context *clusterd.Context, clusterInfo *Clust // OkToStop determines if it's ok to stop an upgrade func OkToStop(context *clusterd.Context, clusterInfo *ClusterInfo, deployment, daemonType, daemonName string) error { + okToStopRetries, okToStopDelay := getRetryConfig(clusterInfo, daemonType) versions, err := GetAllCephDaemonVersions(context, clusterInfo) if err != nil { return errors.Wrap(err, "failed to get ceph daemons versions") @@ -161,11 +168,14 @@ func OkToStop(context *clusterd.Context, clusterInfo *ClusterInfo, deployment, d return nil } } + // we don't implement any checks for mon, rgw and rbdmirror since: // - mon: the is done in the monitor code since it ensures all the mons are always in quorum before continuing // - rgw: the pod spec has a liveness probe so if the pod successfully start // - rbdmirror: you can chain as many as you want like mdss but there is no ok-to-stop logic yet - err = okToStopDaemon(context, clusterInfo, deployment, daemonType, daemonName) + err = util.Retry(okToStopRetries, okToStopDelay, func() error { + return okToStopDaemon(context, clusterInfo, deployment, daemonType, daemonName) + }) if err != nil { return errors.Wrapf(err, "failed to check if %s was ok to stop", deployment) } @@ -177,14 +187,6 @@ func OkToStop(context *clusterd.Context, clusterInfo *ClusterInfo, deployment, d func OkToContinue(context *clusterd.Context, clusterInfo *ClusterInfo, deployment, daemonType, daemonName string) error { // the mon case is handled directly in the deployment where the mon checks for quorum switch daemonType { - case "osd": - if osdDoNothing(context, clusterInfo) { - return nil - } - err := okToContinueOSDDaemon(context, clusterInfo) - if err != nil { - return errors.Wrapf(err, "failed to check if %s was ok to continue", deployment) - } case "mds": err := okToContinueMDSDaemon(context, clusterInfo, deployment, daemonType, daemonName) if err != nil { @@ -214,25 +216,12 @@ func okToStopDaemon(context *clusterd.Context, clusterInfo *ClusterInfo, deploym return nil } -// okToContinueOSDDaemon determines whether it's fine to go to the next osd during an upgrade -// This basically makes sure all the PGs have settled -func okToContinueOSDDaemon(context *clusterd.Context, clusterInfo *ClusterInfo) error { - // Reconciliating PGs should not take too long so let's wait up to 10 minutes - err := util.Retry(10, 60*time.Second, func() error { - return IsClusterCleanError(context, clusterInfo) - }) - if err != nil { - return err - } - - return nil -} - // okToContinueMDSDaemon determines whether it's fine to go to the next mds during an upgrade // mostly a placeholder function for the future but since we have standby mds this shouldn't be needed func okToContinueMDSDaemon(context *clusterd.Context, clusterInfo *ClusterInfo, deployment, daemonType, daemonName string) error { // wait for the MDS to be active again or in standby-replay - err := util.Retry(10, 15*time.Second, func() error { + retries, delay := getRetryConfig(clusterInfo, "mds") + err := util.Retry(retries, delay, func() error { return MdsActiveOrStandbyReplay(context, clusterInfo, findFSName(deployment)) }) if err != nil { @@ -400,3 +389,14 @@ func osdDoNothing(context *clusterd.Context, clusterInfo *ClusterInfo) bool { return false } + +func getRetryConfig(clusterInfo *ClusterInfo, daemonType string) (int, time.Duration) { + switch daemonType { + case "osd": + return int(clusterInfo.OsdUpgradeTimeout / defaultOSDRetryDelay), defaultOSDRetryDelay + case "mds": + return defaultMaxRetries, 15 * time.Second + } + + return defaultMaxRetries, defaultRetryDelay +} diff --git a/pkg/daemon/ceph/client/upgrade_test.go b/pkg/daemon/ceph/client/upgrade_test.go index 748ec37d7..e6aaf1518 100644 --- a/pkg/daemon/ceph/client/upgrade_test.go +++ b/pkg/daemon/ceph/client/upgrade_test.go @@ -19,6 +19,7 @@ package client import ( "encoding/json" "testing" + "time" "github.com/pkg/errors" "github.com/rook/rook/pkg/clusterd" @@ -250,3 +251,49 @@ func TestBuildHostListFromTree(t *testing.T) { _, err = buildHostListFromTree(dummyEmptyNodeTree) assert.NoError(t, err) } + +func TestGetRetryConfig(t *testing.T) { + testcases := []struct { + label string + clusterInfo *ClusterInfo + daemonType string + expectedRetries int + expectedDelay time.Duration + }{ + { + label: "case 1: mon daemon", + clusterInfo: &ClusterInfo{}, + daemonType: "mon", + expectedRetries: 10, + expectedDelay: 60 * time.Second, + }, + { + label: "case 2: osd daemon with 5 minutes delay", + clusterInfo: &ClusterInfo{OsdUpgradeTimeout: 5 * time.Minute}, + daemonType: "osd", + expectedRetries: 30, + expectedDelay: 10 * time.Second, + }, + { + label: "case 3: osd daemon with 10 minutes delay", + clusterInfo: &ClusterInfo{OsdUpgradeTimeout: 10 * time.Minute}, + daemonType: "osd", + expectedRetries: 60, + expectedDelay: 10 * time.Second, + }, + { + label: "case 4: mds daemon", + clusterInfo: &ClusterInfo{}, + daemonType: "mds", + expectedRetries: 10, + expectedDelay: 15 * time.Second, + }, + } + + for _, tc := range testcases { + actualRetries, actualDelay := getRetryConfig(tc.clusterInfo, tc.daemonType) + + assert.Equal(t, tc.expectedRetries, actualRetries, "[%s] failed to get correct retry count", tc.label) + assert.Equalf(t, tc.expectedDelay, actualDelay, "[%s] failed to get correct delays between retries", tc.label) + } +} diff --git a/pkg/operator/ceph/cluster/osd/osd.go b/pkg/operator/ceph/cluster/osd/osd.go index 6f43cefce..e69d2658c 100644 --- a/pkg/operator/ceph/cluster/osd/osd.go +++ b/pkg/operator/ceph/cluster/osd/osd.go @@ -23,6 +23,7 @@ import ( "sort" "strconv" "strings" + "time" "k8s.io/client-go/kubernetes" @@ -61,10 +62,11 @@ const ( // AppName is the "app" label on osd pods AppName = "rook-ceph-osd" // FailureDomainKey is the label key whose value is the failure domain of the OSD - FailureDomainKey = "failure-domain" - prepareAppName = "rook-ceph-osd-prepare" - prepareAppNameFmt = "rook-ceph-osd-prepare-%s" - osdAppNameFmt = "rook-ceph-osd-%d" + FailureDomainKey = "failure-domain" + prepareAppName = "rook-ceph-osd-prepare" + prepareAppNameFmt = "rook-ceph-osd-prepare-%s" + osdAppNameFmt = "rook-ceph-osd-%d" + defaultWaitTimeoutForHealthyOSD = 10 * time.Minute // OsdIdLabelKey is the OSD label key OsdIdLabelKey = "ceph-osd-id" serviceAccountName = "rook-ceph-osd" @@ -181,6 +183,13 @@ func (c *Cluster) Start() error { logger.Warningf("useAllNodes is set to false and no nodes, driveGroups, storageClassDevicesets or volumeSources are specified, no OSD pods are going to be created") } + if c.spec.WaitTimeoutForHealthyOSDInMinutes != 0 { + c.clusterInfo.OsdUpgradeTimeout = c.spec.WaitTimeoutForHealthyOSDInMinutes * time.Minute + } else { + c.clusterInfo.OsdUpgradeTimeout = defaultWaitTimeoutForHealthyOSD + } + logger.Infof("wait timeout for healthy OSDs during upgrade or restart is %q", c.clusterInfo.OsdUpgradeTimeout) + // start the jobs to provision the OSD devices logger.Infof("start provisioning the osds on pvcs, if needed") c.startProvisioningOverPVCs(config) diff --git a/pkg/operator/k8sutil/deployment.go b/pkg/operator/k8sutil/deployment.go index 5de304326..3338d71c2 100644 --- a/pkg/operator/k8sutil/deployment.go +++ b/pkg/operator/k8sutil/deployment.go @@ -24,7 +24,6 @@ import ( "github.com/banzaicloud/k8s-objectmatcher/patch" "github.com/rook/rook/pkg/clusterd" - "github.com/rook/rook/pkg/util" apps "k8s.io/api/apps/v1" v1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" @@ -84,10 +83,7 @@ func UpdateDeploymentAndWait(clusterdContext *clusterd.Context, modifiedDeployme logger.Infof("updating deployment %q after verifying it is safe to stop", modifiedDeployment.Name) // Let's verify the deployment can be stopped - // retry for 5 times, every minute - err = util.Retry(5, 60*time.Second, func() error { - return verifyCallback("stop") - }) + err = verifyCallback("stop") if err != nil { return nil, fmt.Errorf("failed to check if deployment %q can be updated. %v", modifiedDeployment.Name, err) }