[Feature] Fix UpToDate Condition (#552)

Author: ajanikow

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Change Log
 
 ## [master](https://github.com/arangodb/kube-arangodb/tree/master) (N/A)
+- Added additional checks in UpToDate condition
 - Added extended Rotation check for Cluster mode
 - Removed old rotation logic (rotation of ArangoDeployment may be enforced after Operator upgrade)
 - Added UpToDate condition in ArangoDeployment Status

pkg/apis/deployment/v1/plan.go

@@ -33,6 +33,8 @@ import (
 type ActionType string
 
 const (
+	// ActionTypeIdle causes a plan to be recalculated.
+	ActionTypeIdle ActionType = "Idle"
 	// ActionTypeAddMember causes a member to be added.
 	ActionTypeAddMember ActionType = "AddMember"
 	// ActionTypeRemoveMember causes a member to be removed.

pkg/deployment/deployment_inspector.go

@@ -119,14 +119,8 @@ func (d *Deployment) inspectDeploymentWithError(ctx context.Context, lastInterva
 		return minInspectionInterval, errors.Wrapf(err, "Calculation of spec failed")
 	} else {
 		condition, exists := status.Conditions.Get(api.ConditionTypeUpToDate)
-		if (checksum != status.AppliedVersion && (!exists || condition.IsTrue())) ||
-			(checksum == status.AppliedVersion && (!exists || !condition.IsTrue())) {
-			if err = d.WithStatusUpdate(func(s *api.DeploymentStatus) bool {
-				if checksum == status.AppliedVersion {
-					return s.Conditions.Update(api.ConditionTypeUpToDate, true, "Everything is UpToDate", "Spec applied")
-				}
-				return s.Conditions.Update(api.ConditionTypeUpToDate, false, "Spec Changed", "Spec Object changed. Waiting until plan will be applied")
-			}); err != nil {
+		if checksum != status.AppliedVersion && (!exists || condition.IsTrue()) {
+			if err = d.updateCondition(api.ConditionTypeUpToDate, false, "Spec Changed", "Spec Object changed. Waiting until plan will be applied"); err != nil {
 				return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition")
 			}
 
@@ -185,8 +179,37 @@ func (d *Deployment) inspectDeploymentWithError(ctx context.Context, lastInterva
 	}
 
 	// Create scale/update plan
-	if err := d.reconciler.CreatePlan(); err != nil {
+	if err, updated := d.reconciler.CreatePlan(); err != nil {
 		return minInspectionInterval, errors.Wrapf(err, "Plan creation failed")
+	} else if updated {
+		return minInspectionInterval, nil
+	}
+
+	if d.apiObject.Status.Plan.IsEmpty() && status.AppliedVersion != checksum {
+		if err := d.WithStatusUpdate(func(s *api.DeploymentStatus) bool {
+			s.AppliedVersion = checksum
+			return true
+		}); err != nil {
+			return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition")
+		}
+
+		return minInspectionInterval, nil
+	} else if status.AppliedVersion == checksum {
+		if !status.Plan.IsEmpty() && status.Conditions.IsTrue(api.ConditionTypeUpToDate) {
+			if err = d.updateCondition(api.ConditionTypeUpToDate, false, "Plan is not empty", "There are pending operations in plan"); err != nil {
+				return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition")
+			}
+
+			return minInspectionInterval, nil
+		}
+
+		if status.Plan.IsEmpty() && !status.Conditions.IsTrue(api.ConditionTypeUpToDate) {
+			if err = d.updateCondition(api.ConditionTypeUpToDate, true, "Spec is Up To Date", "Spec is Up To Date"); err != nil {
+				return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition")
+			}
+
+			return minInspectionInterval, nil
+		}
 	}
 
 	// Execute current step of scale/update plan
@@ -196,18 +219,6 @@ func (d *Deployment) inspectDeploymentWithError(ctx context.Context, lastInterva
 	}
 	if retrySoon {
 		nextInterval = minInspectionInterval
-	} else {
-		// Do not retry - so plan is empty
-		if status.AppliedVersion != checksum {
-			if err := d.WithStatusUpdate(func(s *api.DeploymentStatus) bool {
-				s.AppliedVersion = checksum
-				return true
-			}); err != nil {
-				return minInspectionInterval, errors.Wrapf(err, "Unable to update UpToDate condition")
-			}
-
-			return minInspectionInterval, nil
-		}
 	}
 
 	// Create access packages
@@ -279,3 +290,14 @@ func (d *Deployment) triggerInspection() {
 func (d *Deployment) triggerCRDInspection() {
 	d.inspectCRDTrigger.Trigger()
 }
+
+func (d *Deployment) updateCondition(conditionType api.ConditionType, status bool, reason, message string) error {
+	d.deps.Log.Info().Str("condition", string(conditionType)).Bool("status", status).Str("reason", reason).Str("message", message).Msg("Updated condition")
+	if err := d.WithStatusUpdate(func(s *api.DeploymentStatus) bool {
+		return s.Conditions.Update(conditionType, status, reason, message)
+	}); err != nil {
+		return errors.Wrapf(err, "Unable to update condition")
+	}
+
+	return nil
+}

pkg/deployment/reconcile/action_idle.go

@@ -0,0 +1,60 @@
+//
+// DISCLAIMER
+//
+// Copyright 2020 ArangoDB GmbH, Cologne, Germany
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// Copyright holder is ArangoDB GmbH, Cologne, Germany
+//
+// Author Adam Janikowski
+//
+
+package reconcile
+
+import (
+	"context"
+
+	api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1"
+	"github.com/rs/zerolog"
+)
+
+func init() {
+	registerAction(api.ActionTypeIdle, newIdleAction)
+}
+
+// newIdleAction creates a new Action that implements the given
+// planned Idle action.
+func newIdleAction(log zerolog.Logger, action api.Action, actionCtx ActionContext) Action {
+	a := &actionIdle{}
+
+	a.actionImpl = newActionImplDefRef(log, action, actionCtx, addMemberTimeout)
+
+	return a
+}
+
+// actionIdle implements an Idle.
+type actionIdle struct {
+	// actionImpl implement timeout and member id functions
+	actionImpl
+
+	// actionEmptyCheckProgress implement check progress with empty implementation
+	actionEmptyCheckProgress
+}
+
+// Start performs the start of the action.
+// Returns true if the action is completely finished, false in case
+// the start time needs to be recorded and a ready condition needs to be checked.
+func (a *actionIdle) Start(ctx context.Context) (bool, error) {
+	return true, nil
+}

pkg/deployment/reconcile/plan_builder.go

@@ -52,12 +52,12 @@ type upgradeDecision struct {
 // CreatePlan considers the current specification & status of the deployment creates a plan to
 // get the status in line with the specification.
 // If a plan already exists, nothing is done.
-func (d *Reconciler) CreatePlan() error {
+func (d *Reconciler) CreatePlan() (error, bool) {
 	// Get all current pods
 	pods, err := d.context.GetOwnedPods()
 	if err != nil {
 		d.log.Debug().Err(err).Msg("Failed to get owned pods")
-		return maskAny(err)
+		return maskAny(err), false
 	}
 
 	// Create plan
@@ -69,19 +69,19 @@ func (d *Reconciler) CreatePlan() error {
 
 	// If not change, we're done
 	if !changed {
-		return nil
+		return nil, false
 	}
 
 	// Save plan
 	if len(newPlan) == 0 {
 		// Nothing to do
-		return nil
+		return nil, false
 	}
 	status.Plan = newPlan
 	if err := d.context.UpdateStatus(status, lastVersion); err != nil {
-		return maskAny(err)
+		return maskAny(err), false
 	}
-	return nil
+	return nil, true
 }
 
 func fetchAgency(log zerolog.Logger,
@@ -112,6 +112,7 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
 	currentPlan api.Plan, spec api.DeploymentSpec,
 	status api.DeploymentStatus, pods []v1.Pod,
 	context PlanBuilderContext) (api.Plan, bool) {
+
 	if !currentPlan.IsEmpty() {
 		// Plan already exists, complete that first
 		return currentPlan, false
@@ -176,7 +177,8 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
 	// Ensure that we were able to get agency info
 	if len(plan) == 0 && agencyErr != nil {
 		log.Err(agencyErr).Msg("unable to build further plan without access to agency")
-		return plan, false
+		return append(plan,
+			api.NewAction(api.ActionTypeIdle, api.ServerGroupUnknown, "")), true
 	}
 
 	// Check for cleaned out dbserver in created state
@@ -200,7 +202,13 @@ func createPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
 
 	// Check for the need to rotate one or more members
 	if plan.IsEmpty() {
-		plan = createRotateOrUpgradePlan(log, apiObject, spec, status, context, pods)
+		newPlan, idle := createRotateOrUpgradePlan(log, apiObject, spec, status, context, pods)
+		if idle {
+			plan = append(plan,
+				api.NewAction(api.ActionTypeIdle, api.ServerGroupUnknown, ""))
+		} else {
+			plan = append(plan, newPlan...)
+		}
 	}
 
 	// Check for the need to rotate TLS certificate of a members

pkg/deployment/reconcile/plan_builder_rotate_upgrade.go

@@ -35,7 +35,7 @@ import (
 
 // createRotateOrUpgradePlan goes over all pods to check if an upgrade or rotate is needed.
 func createRotateOrUpgradePlan(log zerolog.Logger, apiObject k8sutil.APIObject, spec api.DeploymentSpec,
-	status api.DeploymentStatus, context PlanBuilderContext, pods []core.Pod) api.Plan {
+	status api.DeploymentStatus, context PlanBuilderContext, pods []core.Pod) (api.Plan, bool) {
 
 	var newPlan api.Plan
 	var upgradeNotAllowed bool
@@ -103,12 +103,13 @@ func createRotateOrUpgradePlan(log zerolog.Logger, apiObject k8sutil.APIObject,
 	} else if !newPlan.IsEmpty() {
 		if clusterReadyForUpgrade(context) {
 			// Use the new plan
-			return newPlan
+			return newPlan, false
 		} else {
 			log.Info().Msg("Pod needs upgrade but cluster is not ready. Either some shards are not in sync or some member is not ready.")
+			return nil, true
 		}
 	}
-	return nil
+	return nil, false
 }
 
 // podNeedsUpgrading decides if an upgrade of the pod is needed (to comply with

pkg/deployment/reconcile/plan_builder_test.go

@@ -776,7 +776,7 @@ func TestCreatePlan(t *testing.T) {
 			if testCase.Helper != nil {
 				testCase.Helper(testCase.context.ArangoDeployment)
 			}
-			err := r.CreatePlan()
+			err, _ := r.CreatePlan()
 
 			// Assert
 			if testCase.ExpectedEvent != nil {