[Feature] Add condition to postpone changing storage class name (#875) (#895)

Author: informalict

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## [master](https://github.com/arangodb/kube-arangodb/tree/master) (N/A)
 - Do not check License V2 on Community images
 - Add status.members.<group>.
+- Don't replace pod immediately when storage class changes
 - Define MemberReplacementRequired condition
 - Remove pod immediately when annotation is turned on
 - (ARM64) Add support for ARM64 enablement

chart/kube-arangodb/templates/deployment.yaml

@@ -105,6 +105,9 @@ spec:
 {{ if .Values.operator.features.backup }}
                     - --operator.backup
 {{- end }}
+{{- if .Values.operator.debug }}
+                    - --mode.single
+{{- end }}
 {{ if .Values.operator.features.apps }}
                     - --operator.apps
 {{- end }}

pkg/apis/deployment/v1/server_group_spec.go

@@ -116,8 +116,9 @@ type ServerGroupSpec struct {
 	// VolumeClaimTemplate specifies a template for volume claims
 	VolumeClaimTemplate *core.PersistentVolumeClaim `json:"volumeClaimTemplate,omitempty"`
 	// VolumeResizeMode specified resize mode for pvc
-	VolumeResizeMode  *PVCResizeMode `json:"pvcResizeMode,omitempty"`
-	VolumeAllowShrink *bool          `json:"volumeAllowShrink,omitempty"`
+	VolumeResizeMode *PVCResizeMode `json:"pvcResizeMode,omitempty"`
+	// Deprecated: VolumeAllowShrink allows shrink the volume
+	VolumeAllowShrink *bool `json:"volumeAllowShrink,omitempty"`
 	// AntiAffinity specified additional antiAffinity settings in ArangoDB Pod definitions
 	AntiAffinity *core.PodAntiAffinity `json:"antiAffinity,omitempty"`
 	// Affinity specified additional affinity settings in ArangoDB Pod definitions
@@ -682,6 +683,7 @@ func (s ServerGroupSpec) ResetImmutableFields(group ServerGroup, fieldPrefix str
 	return resetFields
 }
 
+// Deprecated: GetVolumeAllowShrink returns true when it is possible to shrink the volume.
 func (s ServerGroupSpec) GetVolumeAllowShrink() bool {
 	if s.VolumeAllowShrink == nil {
 		return false // Default value

pkg/apis/deployment/v2alpha1/server_group_spec.go

@@ -116,8 +116,9 @@ type ServerGroupSpec struct {
 	// VolumeClaimTemplate specifies a template for volume claims
 	VolumeClaimTemplate *core.PersistentVolumeClaim `json:"volumeClaimTemplate,omitempty"`
 	// VolumeResizeMode specified resize mode for pvc
-	VolumeResizeMode  *PVCResizeMode `json:"pvcResizeMode,omitempty"`
-	VolumeAllowShrink *bool          `json:"volumeAllowShrink,omitempty"`
+	VolumeResizeMode *PVCResizeMode `json:"pvcResizeMode,omitempty"`
+	// Deprecated: VolumeAllowShrink allows shrink the volume
+	VolumeAllowShrink *bool `json:"volumeAllowShrink,omitempty"`
 	// AntiAffinity specified additional antiAffinity settings in ArangoDB Pod definitions
 	AntiAffinity *core.PodAntiAffinity `json:"antiAffinity,omitempty"`
 	// Affinity specified additional affinity settings in ArangoDB Pod definitions
@@ -682,6 +683,7 @@ func (s ServerGroupSpec) ResetImmutableFields(group ServerGroup, fieldPrefix str
 	return resetFields
 }
 
+// Deprecated: GetVolumeAllowShrink returns true when it is possible to shrink the volume.
 func (s ServerGroupSpec) GetVolumeAllowShrink() bool {
 	if s.VolumeAllowShrink == nil {
 		return false // Default value

pkg/deployment/member/phase_updates.go

@@ -86,6 +86,7 @@ func removeMemberConditionsMapFunc(m *api.MemberStatus) {
 	m.Conditions.Remove(api.ConditionTypeUpdateFailed)
 	m.Conditions.Remove(api.ConditionTypeCleanedOut)
 	m.Conditions.Remove(api.ConditionTypeTopologyAware)
+	m.Conditions.Remove(api.MemberReplacementRequired)
 
 	m.RemoveTerminationsBefore(time.Now().Add(-1 * recentTerminationsKeepPeriod))
 

pkg/deployment/reconcile/action_set_member_condition_v2.go

@@ -70,26 +70,44 @@ func (a actionSetMemberConditionV2) Start(ctx context.Context) (bool, error) {
 		as := a.action.Params[setConditionActionV2KeyStatus] == string(core.ConditionTrue)
 
 		if err := a.actionCtx.WithStatusUpdateErr(ctx, func(s *api.DeploymentStatus) (bool, error) {
-			m, _, ok := s.Members.ElementByID(a.action.MemberID)
-			if !ok {
+			var changed bool
+
+			s.Members.ForServerGroup(func(group api.ServerGroup, members api.MemberStatusList) error {
+				for i := range members {
+					if members[i].ID == a.action.MemberID {
+						changed = members[i].Conditions.UpdateWithHash(api.ConditionType(aa), as, ar, am, ah)
+						return nil
+					}
+				}
+
 				a.log.Info().Msg("can not set the condition because the member is gone already")
-				return false, nil
-			}
+				return nil
+			}, a.action.Group)
 
-			return m.Conditions.UpdateWithHash(api.ConditionType(aa), as, ar, am, ah), nil
+			// If not found then false is returned.
+			return changed, nil
 		}); err != nil {
 			a.log.Warn().Err(err).Msgf("unable to update status")
 			return true, nil
 		}
 	case setConditionActionV2KeyTypeRemove:
 		if err := a.actionCtx.WithStatusUpdateErr(ctx, func(s *api.DeploymentStatus) (bool, error) {
-			m, _, ok := s.Members.ElementByID(a.action.MemberID)
-			if !ok {
-				a.log.Info().Msg("can not set the condition because the member is gone already")
-				return false, nil
-			}
-
-			return m.Conditions.Remove(api.ConditionType(aa)), nil
+			var changed bool
+
+			s.Members.ForServerGroup(func(group api.ServerGroup, members api.MemberStatusList) error {
+				for i := range members {
+					if members[i].ID == a.action.MemberID {
+						changed = members[i].Conditions.Remove(api.ConditionType(aa))
+						return nil
+					}
+				}
+
+				a.log.Info().Msg("can not remove the condition because the member is gone already")
+				return nil
+			}, a.action.Group)
+
+			// If not found then false is returned.
+			return changed, nil
 		}); err != nil {
 			a.log.Warn().Err(err).Msgf("unable to update status")
 			return true, nil

pkg/deployment/reconcile/condition_member_recreation.go

@@ -22,12 +22,18 @@ package reconcile
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
+	"github.com/rs/zerolog"
+	core "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+
+	"github.com/arangodb/kube-arangodb/pkg/apis/deployment"
 	api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1"
+	"github.com/arangodb/kube-arangodb/pkg/util"
 	"github.com/arangodb/kube-arangodb/pkg/util/k8sutil"
 	inspectorInterface "github.com/arangodb/kube-arangodb/pkg/util/k8sutil/inspector"
-	"github.com/rs/zerolog"
 )
 
 func createMemberRecreationConditionsPlan(ctx context.Context,
@@ -37,17 +43,18 @@ func createMemberRecreationConditionsPlan(ctx context.Context,
 	var p api.Plan
 
 	for _, m := range status.Members.AsList() {
-		resp, recreate := EvaluateMemberRecreationCondition(ctx, log, apiObject, spec, status, m.Group, m.Member, cachedStatus, context)
+		message, recreate := EvaluateMemberRecreationCondition(ctx, log, apiObject, spec, status, m.Group, m.Member,
+			cachedStatus, context, isStorageClassChanged, isVolumeSizeChanged)
 
 		if !recreate {
 			if _, ok := m.Member.Conditions.Get(api.MemberReplacementRequired); ok {
 				// Unset condition
 				p = append(p, removeMemberConditionActionV2("Member replacement not required", api.MemberReplacementRequired, m.Group, m.Member.ID))
 			}
 		} else {
-			if c, ok := m.Member.Conditions.Get(api.MemberReplacementRequired); !ok || !c.IsTrue() || c.Message != resp {
+			if c, ok := m.Member.Conditions.Get(api.MemberReplacementRequired); !ok || !c.IsTrue() || c.Message != message {
 				// Update condition
-				p = append(p, updateMemberConditionActionV2("Member replacement required", api.MemberReplacementRequired, m.Group, m.Member.ID, true, "Member replacement required", resp, ""))
+				p = append(p, updateMemberConditionActionV2("Member replacement required", api.MemberReplacementRequired, m.Group, m.Member.ID, true, "Member replacement required", message, ""))
 			}
 		}
 	}
@@ -59,7 +66,7 @@ type MemberRecreationConditionEvaluator func(ctx context.Context,
 	log zerolog.Logger, apiObject k8sutil.APIObject,
 	spec api.DeploymentSpec, status api.DeploymentStatus,
 	group api.ServerGroup, member api.MemberStatus,
-	cachedStatus inspectorInterface.Inspector, context PlanBuilderContext) (string, bool)
+	cachedStatus inspectorInterface.Inspector, context PlanBuilderContext) (bool, string, error)
 
 func EvaluateMemberRecreationCondition(ctx context.Context,
 	log zerolog.Logger, apiObject k8sutil.APIObject,
@@ -69,10 +76,170 @@ func EvaluateMemberRecreationCondition(ctx context.Context,
 	args := make([]string, 0, len(evaluators))
 
 	for _, e := range evaluators {
-		if s, ok := e(ctx, log, apiObject, spec, status, group, member, cachedStatus, context); ok {
+		ok, s, err := e(ctx, log, apiObject, spec, status, group, member, cachedStatus, context)
+		if err != nil {
+			// When one of an evaluator requires pod's replacement then it should be done.
+			continue
+		}
+
+		if ok {
 			args = append(args, s)
 		}
 	}
 
 	return strings.Join(args, ", "), len(args) > 0
 }
+
+// isStorageClassChanged returns true and reason when the member should be replaced.
+func isStorageClassChanged(_ context.Context, log zerolog.Logger, apiObject k8sutil.APIObject, spec api.DeploymentSpec,
+	_ api.DeploymentStatus, group api.ServerGroup, member api.MemberStatus,
+	cachedStatus inspectorInterface.Inspector, context PlanBuilderContext) (bool, string, error) {
+	if spec.GetMode() == api.DeploymentModeSingle {
+		// Storage cannot be changed in single server deployments.
+		return false, "", nil
+	}
+
+	if member.Phase != api.MemberPhaseCreated {
+		// Only make changes when phase is created.
+		return false, "", nil
+	}
+
+	if member.PersistentVolumeClaimName == "" {
+		// Plan is irrelevant without PVC.
+		return false, "", nil
+	}
+
+	groupSpec := spec.GetServerGroupSpec(group)
+	storageClassName := groupSpec.GetStorageClassName()
+	if storageClassName == "" {
+		// A storage class is not set.
+		return false, "", nil
+	}
+
+	// Check if a storage class changed.
+	if pvc, ok := cachedStatus.PersistentVolumeClaim(member.PersistentVolumeClaimName); !ok {
+		log.Warn().Str("role", group.AsRole()).Str("id", member.ID).Msg("Failed to get PVC")
+		return false, "", fmt.Errorf("failed to get PVC %s", member.PersistentVolumeClaimName)
+	} else {
+		pvcClassName := util.StringOrDefault(pvc.Spec.StorageClassName)
+		if pvcClassName == storageClassName {
+			// A storage class has not been changed.
+			return false, "", nil
+		}
+		if pvcClassName == "" {
+			// TODO what to do here?
+			return false, "", nil
+		}
+	}
+
+	// From here on, it is known that a storage class has changed.
+	if group != api.ServerGroupDBServers && group != api.ServerGroupAgents {
+		// Only agents & DB servers are allowed to change their storage class.
+		context.CreateEvent(k8sutil.NewCannotChangeStorageClassEvent(apiObject, member.ID, group.AsRole(), "Not supported"))
+		return false, "", nil
+	}
+
+	// From here on it is known that the member requires replacement, so `true` must be returned.
+	// If pod does not exist then it will try next time.
+	if pod, ok := cachedStatus.Pod(member.PodName); ok {
+		if _, ok := pod.GetAnnotations()[deployment.ArangoDeploymentPodReplaceAnnotation]; !ok {
+			log.Warn().
+				Str("pod-name", member.PodName).
+				Str("server-group", group.AsRole()).
+				Msgf("try changing a storage class name, but %s", getRequiredReplaceMessage(member.PodName))
+			// No return here.
+		}
+	} else {
+		return false, "", fmt.Errorf("failed to get pod %s", member.PodName)
+	}
+
+	return true, "Storage class has changed", nil
+}
+
+// isVolumeSizeChanged returns true and reason when the member should be replaced.
+func isVolumeSizeChanged(_ context.Context, log zerolog.Logger, _ k8sutil.APIObject, spec api.DeploymentSpec,
+	_ api.DeploymentStatus, group api.ServerGroup, member api.MemberStatus,
+	cachedStatus inspectorInterface.Inspector, _ PlanBuilderContext) (bool, string, error) {
+	if spec.GetMode() == api.DeploymentModeSingle {
+		// Storage cannot be changed in single server deployments.
+		return false, "", nil
+	}
+
+	if member.Phase != api.MemberPhaseCreated {
+		// Only make changes when phase is created.
+		return false, "", nil
+	}
+
+	if member.PersistentVolumeClaimName == "" {
+		// Plan is irrelevant without PVC.
+		return false, "", nil
+	}
+
+	pvc, ok := cachedStatus.PersistentVolumeClaim(member.PersistentVolumeClaimName)
+	if !ok {
+		log.Warn().
+			Str("role", group.AsRole()).
+			Str("id", member.ID).
+			Msg("Failed to get PVC")
+
+		return false, "", fmt.Errorf("failed to get PVC %s", member.PersistentVolumeClaimName)
+	}
+
+	groupSpec := spec.GetServerGroupSpec(group)
+	ok, volumeSize, requestedSize := shouldVolumeResize(groupSpec, pvc)
+	if !ok {
+		return false, "", nil
+	}
+
+	if group != api.ServerGroupDBServers {
+		log.Error().
+			Str("pvc-storage-size", volumeSize.String()).
+			Str("requested-size", requestedSize.String()).
+			Msgf("Volume size should not shrink, because it is not possible for \"%s\"", group.AsRole())
+
+		return false, "", nil
+	}
+
+	// From here on it is known that the member requires replacement, so `true` must be returned.
+	// If pod does not exist then it will try next time.
+	if pod, ok := cachedStatus.Pod(member.PodName); ok {
+		if _, ok := pod.GetAnnotations()[deployment.ArangoDeploymentPodReplaceAnnotation]; !ok {
+			log.Warn().Str("pod-name", member.PodName).
+				Msgf("try shrinking volume size, but %s", getRequiredReplaceMessage(member.PodName))
+			// No return here.
+		}
+	} else {
+		return false, "", fmt.Errorf("failed to get pod %s", member.PodName)
+	}
+
+	return true, "Volume is shrunk", nil
+}
+
+// shouldVolumeResize returns false when a volume should not resize.
+// Currently, it is only possible to shrink a volume size.
+// When return true then the actual and required volume size are returned.
+func shouldVolumeResize(groupSpec api.ServerGroupSpec,
+	pvc *core.PersistentVolumeClaim) (bool, resource.Quantity, resource.Quantity) {
+	var res core.ResourceList
+	if groupSpec.HasVolumeClaimTemplate() {
+		res = groupSpec.GetVolumeClaimTemplate().Spec.Resources.Requests
+	} else {
+		res = groupSpec.Resources.Requests
+	}
+
+	if requestedSize, ok := res[core.ResourceStorage]; ok {
+		if volumeSize, ok := pvc.Spec.Resources.Requests[core.ResourceStorage]; ok {
+			if volumeSize.Cmp(requestedSize) > 0 {
+				// The actual PVC's volume size is greater than requested size, so it can be shrunk to the requested size.
+				return true, volumeSize, requestedSize
+			}
+		}
+	}
+
+	return false, resource.Quantity{}, resource.Quantity{}
+}
+
+func getRequiredReplaceMessage(podName string) string {
+	return fmt.Sprintf("%s annotation is required to be set on the pod %s",
+		deployment.ArangoDeploymentPodReplaceAnnotation, podName)
+}

pkg/deployment/reconcile/plan_builder_generator.go

@@ -86,7 +86,9 @@ func (d *Reconciler) generatePlan(ctx context.Context, cachedStatus inspectorInt
 				action := result.plan[id]
 				d.context.CreateEvent(k8sutil.NewPlanAppendEvent(d.context.GetAPIObject(), action.Type.String(), action.Group.AsRole(), action.MemberID, action.Reason))
 				if r := action.Reason; r != "" {
-					d.log.Info().Str("Action", action.Type.String()).Str("Role", action.Group.AsRole()).Str("Member", action.MemberID).Str("Type", strings.Title(result.planner.Type())).Msgf(r)
+					d.log.Info().Str("Action", action.Type.String()).
+						Str("Role", action.Group.AsRole()).Str("Member", action.MemberID).
+						Str("Type", strings.Title(result.planner.Type())).Msgf(r)
 				}
 			}
 

pkg/deployment/reconcile/plan_builder_normal.go

@@ -72,7 +72,6 @@ func createNormalPlan(ctx context.Context, log zerolog.Logger, apiObject k8sutil
 		ApplySubPlanIfEmpty(createTLSStatusPropagatedFieldUpdate, createCARenewalPlan).
 		ApplySubPlanIfEmpty(createTLSStatusPropagatedFieldUpdate, createCAAppendPlan).
 		ApplyIfEmpty(createKeyfileRenewalPlan).
-		ApplyIfEmpty(createRotateServerStoragePlan).
 		ApplyIfEmpty(createRotateServerStorageResizePlan).
 		ApplySubPlanIfEmpty(createTLSStatusPropagatedFieldUpdate, createRotateTLSServerSNIPlan).
 		ApplyIfEmpty(createRestorePlan).