[Bugfix] Prevent Runtime update restarts (#1205)

CHANGELOG.md

@@ -38,6 +38,7 @@
 - (Feature) Allow to exclude metrics
 - (Bugfix) Do not stop Sync if Synchronization is in progress
 - (Bugfix) Wait for Pod to be Ready in post-restart actions
+- (Bugfix) Prevent Runtime update restarts
 
 ## [1.2.20](https://github.com/arangodb/kube-arangodb/tree/1.2.20) (2022-10-25)
 - (Feature) Add action progress

pkg/apis/deployment/v1/conditions.go

@@ -144,6 +144,8 @@ type Condition struct {
 	Message string `json:"message,omitempty"`
 	// Hash keep propagation hash id, for example checksum of secret
 	Hash string `json:"hash,omitempty"`
+	// Params keeps additional params for the condition
+	Params ConditionParams `json:"params,omitempty"`
 }
 
 func (c Condition) IsTrue() bool {
@@ -154,6 +156,7 @@ func (c Condition) IsTrue() bool {
 func (c Condition) Equal(other Condition) bool {
 	return c.Type == other.Type &&
 		c.Status == other.Status &&
+		c.Params.Equal(other.Params) &&
 		util.TimeCompareEqual(c.LastUpdateTime, other.LastUpdateTime) &&
 		util.TimeCompareEqual(c.LastTransitionTime, other.LastTransitionTime) &&
 		c.Reason == other.Reason &&

pkg/apis/deployment/v1/conditions_params.go

@@ -0,0 +1,46 @@
+//
+// DISCLAIMER
+//
+// Copyright 2022 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
+//
+
+package v1
+
+const (
+	// ConditionParamContainerUpdatingName define parameter used during Image Runtime update
+	ConditionParamContainerUpdatingName ConditionParam = "updatingContainerName"
+)
+
+// ConditionParam is a strongly typed condition parameter
+type ConditionParam string
+
+type ConditionParams map[ConditionParam]string
+
+// Equal compare two ConditionParams objects
+func (c ConditionParams) Equal(b ConditionParams) bool {
+	if len(c) != len(b) {
+		return false
+	}
+
+	for k, v := range c {
+		if v2, ok := b[k]; !ok || v != v2 {
+			return false
+		}
+	}
+
+	return true
+}

pkg/apis/deployment/v1/zz_generated.deepcopy.go

@@ -809,6 +809,13 @@ func (in *Condition) DeepCopyInto(out *Condition) {
 	*out = *in
 	in.LastUpdateTime.DeepCopyInto(&out.LastUpdateTime)
 	in.LastTransitionTime.DeepCopyInto(&out.LastTransitionTime)
+	if in.Params != nil {
+		in, out := &in.Params, &out.Params
+		*out = make(ConditionParams, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	return
 }
 
@@ -844,6 +851,28 @@ func (in ConditionList) DeepCopy() ConditionList {
 	return *out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in ConditionParams) DeepCopyInto(out *ConditionParams) {
+	{
+		in := &in
+		*out = make(ConditionParams, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+		return
+	}
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ConditionParams.
+func (in ConditionParams) DeepCopy() ConditionParams {
+	if in == nil {
+		return nil
+	}
+	out := new(ConditionParams)
+	in.DeepCopyInto(out)
+	return *out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *DatabaseSpec) DeepCopyInto(out *DatabaseSpec) {
 	*out = *in

pkg/apis/deployment/v2alpha1/conditions.go

@@ -144,6 +144,8 @@ type Condition struct {
 	Message string `json:"message,omitempty"`
 	// Hash keep propagation hash id, for example checksum of secret
 	Hash string `json:"hash,omitempty"`
+	// Params keeps additional params for the condition
+	Params ConditionParams `json:"params,omitempty"`
 }
 
 func (c Condition) IsTrue() bool {
@@ -154,6 +156,7 @@ func (c Condition) IsTrue() bool {
 func (c Condition) Equal(other Condition) bool {
 	return c.Type == other.Type &&
 		c.Status == other.Status &&
+		c.Params.Equal(other.Params) &&
 		util.TimeCompareEqual(c.LastUpdateTime, other.LastUpdateTime) &&
 		util.TimeCompareEqual(c.LastTransitionTime, other.LastTransitionTime) &&
 		c.Reason == other.Reason &&

pkg/apis/deployment/v2alpha1/conditions_params.go

@@ -0,0 +1,46 @@
+//
+// DISCLAIMER
+//
+// Copyright 2022 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
+//
+
+package v2alpha1
+
+const (
+	// ConditionParamContainerUpdatingName define parameter used during Image Runtime update
+	ConditionParamContainerUpdatingName ConditionParam = "updatingContainerName"
+)
+
+// ConditionParam is a strongly typed condition parameter
+type ConditionParam string
+
+type ConditionParams map[ConditionParam]string
+
+// Equal compare two ConditionParams objects
+func (c ConditionParams) Equal(b ConditionParams) bool {
+	if len(c) != len(b) {
+		return false
+	}
+
+	for k, v := range c {
+		if v2, ok := b[k]; !ok || v != v2 {
+			return false
+		}
+	}
+
+	return true
+}

pkg/apis/deployment/v2alpha1/zz_generated.deepcopy.go

@@ -809,6 +809,13 @@ func (in *Condition) DeepCopyInto(out *Condition) {
 	*out = *in
 	in.LastUpdateTime.DeepCopyInto(&out.LastUpdateTime)
 	in.LastTransitionTime.DeepCopyInto(&out.LastTransitionTime)
+	if in.Params != nil {
+		in, out := &in.Params, &out.Params
+		*out = make(ConditionParams, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	return
 }
 
@@ -844,6 +851,28 @@ func (in ConditionList) DeepCopy() ConditionList {
 	return *out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in ConditionParams) DeepCopyInto(out *ConditionParams) {
+	{
+		in := &in
+		*out = make(ConditionParams, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+		return
+	}
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ConditionParams.
+func (in ConditionParams) DeepCopy() ConditionParams {
+	if in == nil {
+		return nil
+	}
+	out := new(ConditionParams)
+	in.DeepCopyInto(out)
+	return *out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *DatabaseSpec) DeepCopyInto(out *DatabaseSpec) {
 	*out = *in

pkg/deployment/reconcile/action.go

@@ -81,6 +81,22 @@ func getActionPost(a Action, ctx context.Context) error {
 	}
 }
 
+// ActionPre keep interface which is executed before action is started.
+type ActionPre interface {
+	Action
+
+	// Pre execute after action is completed
+	Pre(ctx context.Context) error
+}
+
+func getActionPre(a Action, ctx context.Context) error {
+	if c, ok := a.(ActionPre); !ok {
+		return nil
+	} else {
+		return c.Pre(ctx)
+	}
+}
+
 // ActionReloadCachedStatus keeps information about CachedStatus reloading (executed after action has been executed)
 type ActionReloadCachedStatus interface {
 	Action

pkg/deployment/reconcile/action_runtime_container_image_update.go

@@ -43,12 +43,44 @@ func newRuntimeContainerImageUpdateAction(action api.Action, actionCtx ActionCon
 }
 
 var _ ActionPost = &actionRuntimeContainerImageUpdate{}
+var _ ActionPre = &actionRuntimeContainerImageUpdate{}
 
 type actionRuntimeContainerImageUpdate struct {
 	// actionImpl implement timeout and member id functions
 	actionImpl
 }
 
+func (a actionRuntimeContainerImageUpdate) Pre(ctx context.Context) error {
+	a.log.Info("Updating member condition")
+	m, ok := a.actionCtx.GetMemberStatusByID(a.action.MemberID)
+	if !ok {
+		a.log.Info("member is gone already")
+		return nil
+	}
+
+	cname, _, ok := a.getContainerDetails()
+	if !ok {
+		a.log.Info("Unable to find container details")
+		return nil
+	}
+
+	if c, ok := m.Conditions.Get(api.ConditionTypeUpdating); ok {
+		if c.Params == nil {
+			c.Params = api.ConditionParams{}
+		}
+
+		if c.Params[api.ConditionParamContainerUpdatingName] != cname {
+			c.Params[api.ConditionParamContainerUpdatingName] = cname
+
+			if err := a.actionCtx.UpdateMember(ctx, m); err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
 func (a actionRuntimeContainerImageUpdate) Post(ctx context.Context) error {
 	a.log.Info("Updating container image")
 	m, ok := a.actionCtx.GetMemberStatusByID(a.action.MemberID)
@@ -57,6 +89,22 @@ func (a actionRuntimeContainerImageUpdate) Post(ctx context.Context) error {
 		return nil
 	}
 
+	if c, ok := m.Conditions.Get(api.ConditionTypeUpdating); ok {
+		if c.Params != nil {
+			if _, ok := c.Params[api.ConditionParamContainerUpdatingName]; ok {
+				delete(c.Params, api.ConditionParamContainerUpdatingName)
+
+				if len(c.Params) == 0 {
+					c.Params = nil
+				}
+
+				if err := a.actionCtx.UpdateMember(ctx, m); err != nil {
+					return err
+				}
+			}
+		}
+	}
+
 	cname, image, ok := a.getContainerDetails()
 	if !ok {
 		a.log.Info("Unable to find container details")

pkg/deployment/reconcile/action_wait_for_member_ready.go

@@ -63,5 +63,9 @@ func (a *actionWaitForMemberReady) CheckProgress(ctx context.Context) (bool, boo
 		return true, false, nil
 	}
 
+	if a.actionCtx.GetMode() == api.DeploymentModeActiveFailover {
+		return true, false, nil
+	}
+
 	return member.Conditions.IsTrue(api.ConditionTypeReady), false, nil
 }

pkg/deployment/reconcile/plan_builder_high.go

@@ -183,7 +183,7 @@ func (r *Reconciler) updateMemberRotationConditions(apiObject k8sutil.APIObject,
 		return nil, nil
 	}
 
-	if m, _, reason, err := rotation.IsRotationRequired(context.ACS(), spec, member, group, p, arangoMember.Spec.Template, arangoMember.Status.Template); err != nil {
+	if m, _, checksum, reason, err := rotation.IsRotationRequired(context.ACS(), spec, member, group, p, arangoMember.Spec.Template, arangoMember.Status.Template); err != nil {
 		r.log.Err(err).Error("Error while getting rotation details")
 		return nil, err
 	} else {
@@ -208,7 +208,7 @@ func (r *Reconciler) updateMemberRotationConditions(apiObject k8sutil.APIObject,
 			return api.Plan{actions.NewAction(api.ActionTypeSetMemberCondition, group, member, reason).AddParam(api.ConditionTypePendingUpdate.String(), "T")}, nil
 		case rotation.SilentRotation:
 			// Propagate changes without restart
-			return api.Plan{actions.NewAction(api.ActionTypeArangoMemberUpdatePodStatus, group, member, "Propagating status of pod").AddParam(ActionTypeArangoMemberUpdatePodStatusChecksum, arangoMember.Spec.Template.GetChecksum())}, nil
+			return api.Plan{actions.NewAction(api.ActionTypeArangoMemberUpdatePodStatus, group, member, "Propagating status of pod").AddParam(ActionTypeArangoMemberUpdatePodStatusChecksum, checksum)}, nil
 		case rotation.GracefulRotation:
 			if reason != "" {
 				r.log.Bool("enforced", false).Info(reason)

pkg/deployment/reconcile/plan_builder_rotate_upgrade.go

@@ -191,7 +191,7 @@ func (r *Reconciler) createUpdatePlanInternal(apiObject k8sutil.APIObject, spec
 			p = nil
 		}
 
-		if mode, p, reason, err := rotation.IsRotationRequired(context.ACS(), spec, m.Member, m.Group, p, arangoMember.Spec.Template, arangoMember.Status.Template); err != nil {
+		if mode, p, checksum, reason, err := rotation.IsRotationRequired(context.ACS(), spec, m.Member, m.Group, p, arangoMember.Spec.Template, arangoMember.Status.Template); err != nil {
 			r.planLogger.Err(err).Str("member", m.Member.ID).Error("Error while generating update plan")
 			continue
 		} else if mode != rotation.InPlaceRotation {
@@ -201,6 +201,8 @@ func (r *Reconciler) createUpdatePlanInternal(apiObject k8sutil.APIObject, spec
 		} else {
 			p = withWaitForMember(p, m.Group, m.Member)
 
+			p = append(p, actions.NewAction(api.ActionTypeArangoMemberUpdatePodStatus, m.Group, m.Member, "Propagating status of pod").AddParam(ActionTypeArangoMemberUpdatePodStatusChecksum, checksum))
+
 			p = p.Wrap(actions.NewAction(api.ActionTypeSetMemberCondition, m.Group, m.Member, reason).
 				AddParam(api.ConditionTypePendingUpdate.String(), "").AddParam(api.ConditionTypeUpdating.String(), "T"),
 				actions.NewAction(api.ActionTypeSetMemberCondition, m.Group, m.Member, reason).

pkg/deployment/reconcile/plan_executor.go

@@ -219,6 +219,13 @@ func (d *Reconciler) executePlan(ctx context.Context, statusPlan api.Plan, pg pl
 
 		action, actionContext := d.createAction(planAction)
 
+		if !planAction.IsStarted() {
+			if err := getActionPre(action, ctx); err != nil {
+				d.planLogger.Err(err).Error("Pre action failed")
+				return nil, false, false, errors.WithStack(err)
+			}
+		}
+
 		done, abort, recall, retry, err := d.executeAction(ctx, planAction, action)
 		if err != nil {
 			if retry {

pkg/deployment/resources/pod_inspector.go

@@ -128,6 +128,15 @@ func (r *Resources) InspectPods(ctx context.Context, cachedStatus inspectorInter
 		spec := r.context.GetSpec()
 		coreContainers := spec.GetCoreContainers(group)
 
+		if c, ok := memberStatus.Conditions.Get(api.ConditionTypeUpdating); ok {
+			if v, ok := c.Params[api.ConditionParamContainerUpdatingName]; ok {
+				// We are in update phase, container needs to be ignored
+				if v != "" {
+					coreContainers = coreContainers.Remove(v)
+				}
+			}
+		}
+
 		// Update state
 		updateMemberStatusNeeded := false
 		if k8sutil.IsPodSucceeded(pod, coreContainers) {

pkg/deployment/rotation/check.go

@@ -57,7 +57,7 @@ func CheckPossible(member api.MemberStatus) bool {
 	return !member.Conditions.IsTrue(api.ConditionTypeTerminated)
 }
 
-func IsRotationRequired(acs sutil.ACS, spec api.DeploymentSpec, member api.MemberStatus, group api.ServerGroup, pod *core.Pod, specTemplate, statusTemplate *api.ArangoMemberPodTemplate) (mode Mode, plan api.Plan, reason string, err error) {
+func IsRotationRequired(acs sutil.ACS, spec api.DeploymentSpec, member api.MemberStatus, group api.ServerGroup, pod *core.Pod, specTemplate, statusTemplate *api.ArangoMemberPodTemplate) (mode Mode, plan api.Plan, specChecksum string, reason string, err error) {
 	// Determine if rotation is required based on plan and actions
 
 	// Set default mode for return value
@@ -126,10 +126,10 @@ func IsRotationRequired(acs sutil.ACS, spec api.DeploymentSpec, member api.Membe
 	}
 
 	if mode, plan, err := compare(spec, member, group, specTemplate, statusTemplate); err != nil {
-		return SkippedRotation, nil, "", err
+		return SkippedRotation, nil, specTemplate.Checksum, "", err
 	} else if mode == SkippedRotation {
-		return mode, plan, "No rotation needed", nil
+		return mode, plan, specTemplate.Checksum, "No rotation needed", nil
 	} else {
-		return mode, plan, "Pod needs rotation", nil
+		return mode, plan, specTemplate.Checksum, "Pod needs rotation", nil
 	}
 }