Repository: scylladb/scylla-operator
Pull request #314 (2020-12-28 12:22:29)

Generic Upgrade refactorings

**Checklist:**
- [x] Documentation has been updated, if necessary.
- [x] Image has been built (`make docker-build`) on the last commit.


Patch:
diff --git a/config/operator/crd/bases/scylla.scylladb.com_scyllaclusters.yaml b/config/operator/crd/bases/scylla.scylladb.com_scyllaclusters.yaml
index 3345bb130..71317a69c 100644
--- a/config/operator/crd/bases/scylla.scylladb.com_scyllaclusters.yaml
+++ b/config/operator/crd/bases/scylla.scylladb.com_scyllaclusters.yaml
@@ -1407,10 +1407,7 @@ spec:
                   description: FailureStrategy specifies which logic is executed when upgrade failure happens. Currently only Retry is supported.
                   type: string
                 pollInterval:
-                  description: PollInterval specifies how often upgrade logic polls on state updates. Increasing this value should lower number of requests sent to apiserver, but it may affect overall time required spent during upgrade.
-                  type: string
-                validationTimeout:
-                  description: ValidationTimeout specifies how long it can take for Scylla to boot and enter ready state after upgrade until FailureStrategy is executed.
+                  description: PollInterval specifies how often upgrade logic polls on state updates. Increasing this value should lower number of requests sent to apiserver, but it may affect overall time spent during upgrade.
                   type: string
               type: object
             network:
@@ -1438,7 +1435,7 @@ spec:
                     description: Host to repair, by default all hosts are repaired
                     type: string
                   intensity:
-                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that DO NOT SUPPORT ROW-LEVEL REPAIR, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
+                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that *do not support row-level repair*, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
                     type: string
                   interval:
                     description: Interval task schedule interval e.g. 3d2h10m, valid units are d, h, m, s (default "0").
@@ -1616,7 +1613,7 @@ spec:
                   id:
                     type: string
                   intensity:
-                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that DO NOT SUPPORT ROW-LEVEL REPAIR, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
+                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that *do not support row-level repair*, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
                     type: string
                   interval:
                     description: Interval task schedule interval e.g. 3d2h10m, valid units are d, h, m, s (default "0").
diff --git a/examples/common/operator.yaml b/examples/common/operator.yaml
index 2bb83ee2d..55c3b1bfe 100644
--- a/examples/common/operator.yaml
+++ b/examples/common/operator.yaml
@@ -1422,10 +1422,7 @@ spec:
                   description: FailureStrategy specifies which logic is executed when upgrade failure happens. Currently only Retry is supported.
                   type: string
                 pollInterval:
-                  description: PollInterval specifies how often upgrade logic polls on state updates. Increasing this value should lower number of requests sent to apiserver, but it may affect overall time required spent during upgrade.
-                  type: string
-                validationTimeout:
-                  description: ValidationTimeout specifies how long it can take for Scylla to boot and enter ready state after upgrade until FailureStrategy is executed.
+                  description: PollInterval specifies how often upgrade logic polls on state updates. Increasing this value should lower number of requests sent to apiserver, but it may affect overall time spent during upgrade.
                   type: string
               type: object
             network:
@@ -1453,7 +1450,7 @@ spec:
                     description: Host to repair, by default all hosts are repaired
                     type: string
                   intensity:
-                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that DO NOT SUPPORT ROW-LEVEL REPAIR, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
+                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that *do not support row-level repair*, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
                     type: string
                   interval:
                     description: Interval task schedule interval e.g. 3d2h10m, valid units are d, h, m, s (default "0").
@@ -1631,7 +1628,7 @@ spec:
                   id:
                     type: string
                   intensity:
-                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that DO NOT SUPPORT ROW-LEVEL REPAIR, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
+                    description: Intensity how many token ranges (per shard) to repair in a single Scylla repair job. By default this is 1. If you set it to 0 the number of token ranges is adjusted to the maximum supported by node (see max_repair_ranges_in_parallel in Scylla logs). Valid values are 0 and integers >= 1. Higher values will result in increased cluster load and slightly faster repairs. Changing the intensity impacts repair granularity if you need to resume it, the higher the value the more work on resume. For Scylla clusters that *do not support row-level repair*, intensity can be a decimal between (0,1). In that case it specifies percent of shards that can be repaired in parallel on a repair master node. For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
                     type: string
                   interval:
                     description: Interval task schedule interval e.g. 3d2h10m, valid units are d, h, m, s (default "0").
diff --git a/pkg/api/v1alpha1/cluster_types.go b/pkg/api/v1alpha1/cluster_types.go
index f2c8299ab..88e99662f 100644
--- a/pkg/api/v1alpha1/cluster_types.go
+++ b/pkg/api/v1alpha1/cluster_types.go
@@ -76,9 +76,6 @@ const (
 
 // GenericUpgradeSpec hold generic upgrade procedure parameters.
 type GenericUpgradeSpec struct {
-	// ValidationTimeout specifies how long it can take for Scylla to boot and enter ready state
-	// after image upgrade until FailureStrategy is executed.
-	ValidationTimeout *metav1.Duration `json:"validationTimeout,omitempty"`
 	// FailureStrategy specifies which logic is executed when upgrade failure happens.
 	// Currently only Retry is supported.
 	FailureStrategy GenericUpgradeFailureStrategy `json:"failureStrategy,omitempty"`
diff --git a/pkg/api/v1alpha1/cluster_webhook.go b/pkg/api/v1alpha1/cluster_webhook.go
index 568896a22..ba9640c78 100644
--- a/pkg/api/v1alpha1/cluster_webhook.go
+++ b/pkg/api/v1alpha1/cluster_webhook.go
@@ -45,6 +45,10 @@ func (r *ScyllaCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Defaulter = &ScyllaCluster{}
 var _ webhook.Validator = &ScyllaCluster{}
 
+const (
+	DefaultGenericUpgradePollInterval = time.Second
+)
+
 func (c *ScyllaCluster) Default() {
 	for _, r := range c.Spec.Datacenter.Racks {
 		// Empty agent resources
@@ -106,12 +110,8 @@ func (c *ScyllaCluster) Default() {
 		c.Spec.GenericUpgrade.FailureStrategy = GenericUpgradeFailureStrategyRetry
 	}
 
-	if c.Spec.GenericUpgrade.ValidationTimeout == nil {
-		c.Spec.GenericUpgrade.ValidationTimeout = &metav1.Duration{Duration: 30 * time.Minute}
-	}
-
 	if c.Spec.GenericUpgrade.PollInterval == nil {
-		c.Spec.GenericUpgrade.PollInterval = &metav1.Duration{Duration: time.Second}
+		c.Spec.GenericUpgrade.PollInterval = &metav1.Duration{Duration: DefaultGenericUpgradePollInterval}
 	}
 }
 
diff --git a/pkg/api/v1alpha1/zz_generated.deepcopy.go b/pkg/api/v1alpha1/zz_generated.deepcopy.go
index f6c35adf5..3d942f5eb 100644
--- a/pkg/api/v1alpha1/zz_generated.deepcopy.go
+++ b/pkg/api/v1alpha1/zz_generated.deepcopy.go
@@ -240,11 +240,6 @@ func (in *DatacenterSpec) DeepCopy() *DatacenterSpec {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *GenericUpgradeSpec) DeepCopyInto(out *GenericUpgradeSpec) {
 	*out = *in
-	if in.ValidationTimeout != nil {
-		in, out := &in.ValidationTimeout, &out.ValidationTimeout
-		*out = new(v1.Duration)
-		**out = **in
-	}
 	if in.PollInterval != nil {
 		in, out := &in.PollInterval, &out.PollInterval
 		*out = new(v1.Duration)
diff --git a/pkg/controllers/cluster/actions/upgrade_version.go b/pkg/controllers/cluster/actions/upgrade_version.go
index 879ba5df8..1229fb273 100644
--- a/pkg/controllers/cluster/actions/upgrade_version.go
+++ b/pkg/controllers/cluster/actions/upgrade_version.go
@@ -140,7 +140,14 @@ func (a *ClusterVersionUpgrade) Execute(ctx context.Context, s *State) error {
 	a.cc = s.Client
 	a.kubeClient = s.kubeclient
 	a.recorder = s.recorder
-	a.pollInterval = a.Cluster.Spec.GenericUpgrade.PollInterval.Duration
+
+	a.pollInterval = scyllav1alpha1.DefaultGenericUpgradePollInterval
+
+	if a.Cluster.Spec.GenericUpgrade != nil {
+		if a.Cluster.Spec.GenericUpgrade.PollInterval != nil {
+			a.pollInterval = a.Cluster.Spec.GenericUpgrade.PollInterval.Duration
+		}
+	}
 
 	switch a.upgradeProcedure(ctx) {
 	case genericUpgradeProcedure:
@@ -663,8 +670,8 @@ func (a *ClusterVersionUpgrade) deletePod(ctx context.Context) (fsm.Event, error
 	return ActionSuccess, nil
 }
 
-func (a *ClusterVersionUpgrade) validateUpgrade(ctx context.Context) (fsm.Event, error) {
-	err := wait.PollImmediate(a.pollInterval, a.Cluster.Spec.GenericUpgrade.ValidationTimeout.Duration, func() (done bool, err error) {
+func (a *ClusterVersionUpgrade) nodeUpgradedConditionFunc(ctx context.Context) func() (bool, error) {
+	return func() (bool, error) {
 		node, err := a.getCurrentNode(ctx)
 		if err != nil {
 			return false, errors.Wrap(err, "get current node")
@@ -684,15 +691,22 @@ func (a *ClusterVersionUpgrade) validateUpgrade(ctx context.Context) (fsm.Event,
 
 		a.logger.Debug(ctx, "Node validation", "node", node.Name, "ready", podReady(node), "ver", ver)
 		return podReady(node) && a.Cluster.Status.Upgrade.ToVersion == ver, nil
-	})
-	if err != nil {
-		if errors.Is(err, wait.ErrWaitTimeout) {
-			if a.Cluster.Spec.GenericUpgrade.FailureStrategy == scyllav1alpha1.GenericUpgradeFailureStrategyRetry {
-				// Return error to reschedule reconciliation loop.
-				return ActionFailure, errors.Wrap(err, "wait for ready pods")
-			}
+	}
+}
+
+func (a *ClusterVersionUpgrade) validateUpgrade(ctx context.Context) (fsm.Event, error) {
+	failureStrategy := scyllav1alpha1.GenericUpgradeFailureStrategyRetry
+	if a.Cluster.Spec.GenericUpgrade != nil {
+		failureStrategy = a.Cluster.Spec.GenericUpgrade.FailureStrategy
+	}
+
+	switch failureStrategy {
+	case scyllav1alpha1.GenericUpgradeFailureStrategyRetry:
+		if err := wait.PollImmediateInfinite(a.pollInterval, a.nodeUpgradedConditionFunc(ctx)); err != nil {
+			return ActionFailure, errors.Wrap(err, "validate node upgrade")
 		}
-		return ActionFailure, errors.Wrap(err, "validate node upgrade")
+	default:
+		return ActionFailure, errors.Errorf("unsupported failure strategy %s", failureStrategy)
 	}
 
 	return ActionSuccess, nil


Test patch:
diff --git a/pkg/controllers/cluster/actions/upgrade_version_integration_test.go b/pkg/controllers/cluster/actions/upgrade_version_integration_test.go
index cac19cdeb..969413b49 100644
--- a/pkg/controllers/cluster/actions/upgrade_version_integration_test.go
+++ b/pkg/controllers/cluster/actions/upgrade_version_integration_test.go
@@ -54,8 +54,7 @@ var _ = Describe("Cluster controller", func() {
 		BeforeEach(func() {
 			scylla = testEnv.SingleRackCluster(ns)
 			scylla.Spec.GenericUpgrade = &scyllav1alpha1.GenericUpgradeSpec{
-				PollInterval:      &metav1.Duration{Duration: 200 * time.Millisecond},
-				ValidationTimeout: &metav1.Duration{Duration: 5 * time.Second},
+				PollInterval: &metav1.Duration{Duration: 200 * time.Millisecond},
 			}
 
 			Expect(testEnv.Create(ctx, scylla)).To(Succeed())