Remove DataNodes from StatelesOnly update

Issue ytsaurus#325
koct9i · Oct 24, 2024 · 215f82e · 215f82e
1 parent 638a5d8
commit 215f82e
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 85 deletions.
diff --git a/api/v1/ytsaurus_types.go b/api/v1/ytsaurus_types.go
@@ -614,7 +614,7 @@ type YtsaurusSpec struct {
 	//+optional
 	EnableFullUpdate bool `json:"enableFullUpdate"`
 	//+optional
-	//+kubebuilder:validation:Enum={"","Nothing","StatelessOnly","MasterOnly","TabletNodesOnly","ExecNodesOnly","Everything"}
+	//+kubebuilder:validation:Enum={"","Nothing","MasterOnly","DataNodesOnly","TabletNodesOnly","ExecNodesOnly","StatelessOnly","Everything"}
 	// UpdateSelector is an experimental field. Behaviour may change.
 	// If UpdateSelector is not empty EnableFullUpdate is ignored.
 	UpdateSelector UpdateSelector `json:"updateSelector"`
@@ -699,15 +699,17 @@ const (
 	UpdateSelectorUnspecified UpdateSelector = ""
 	// UpdateSelectorNothing means that no component could be updated.
 	UpdateSelectorNothing UpdateSelector = "Nothing"
-	// UpdateSelectorStatelessOnly means that only stateless components (everything but master and tablet nodes)
-	// could be updated.
-	UpdateSelectorStatelessOnly UpdateSelector = "StatelessOnly"
 	// UpdateSelectorMasterOnly means that only master could be updated.
 	UpdateSelectorMasterOnly UpdateSelector = "MasterOnly"
+	// UpdateSelectorTabletNodesOnly means that only data nodes could be updated
+	UpdateSelectorDataNodesOnly UpdateSelector = "DataNodesOnly"
 	// UpdateSelectorTabletNodesOnly means that only tablet nodes could be updated
 	UpdateSelectorTabletNodesOnly UpdateSelector = "TabletNodesOnly"
 	// UpdateSelectorExecNodesOnly means that only tablet nodes could be updated
 	UpdateSelectorExecNodesOnly UpdateSelector = "ExecNodesOnly"
+	// UpdateSelectorStatelessOnly means that only stateless components (everything but master, data nodes, and tablet nodes)
+	// could be updated.
+	UpdateSelectorStatelessOnly UpdateSelector = "StatelessOnly"
 	// UpdateSelectorEverything means that all components could be updated.
 	// With this setting and if master or tablet nodes need update all the components would be updated.
 	UpdateSelectorEverything UpdateSelector = "Everything"

diff --git a/config/crd/bases/cluster.ytsaurus.tech_ytsaurus.yaml b/config/crd/bases/cluster.ytsaurus.tech_ytsaurus.yaml
@@ -34637,10 +34637,11 @@ spec:
                 enum:
                 - ""
                 - Nothing
-                - StatelessOnly
                 - MasterOnly
+                - DataNodesOnly
                 - TabletNodesOnly
                 - ExecNodesOnly
+                - StatelessOnly
                 - Everything
                 type: string
               useIpv4:

diff --git a/controllers/sync.go b/controllers/sync.go
@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/ytsaurus/ytsaurus-k8s-operator/pkg/components"
@@ -386,118 +387,104 @@ func (r *YtsaurusReconciler) handleTabletNodesOnly(
 	return nil, nil
 }
 
-func getComponentNames(components []components.Component) []string {
-	if components == nil {
-		return nil
-	}
-	names := make([]string, 0)
-	for _, cmp := range components {
-		names = append(names, cmp.GetName())
-	}
-	return names
-}
-
 type updateMeta struct {
 	flow ytv1.UpdateFlow
 	// componentNames is a list of component names that will be updated. It is built according to the update selector.
 	componentNames []string
 }
 
+func canUpdateComponent(selector ytv1.UpdateSelector, component consts.ComponentType) bool {
+	switch selector {
+	case ytv1.UpdateSelectorNothing:
+		return false
+	case ytv1.UpdateSelectorMasterOnly:
+		return component == consts.MasterType
+	case ytv1.UpdateSelectorDataNodesOnly:
+		return component == consts.DataNodeType
+	case ytv1.UpdateSelectorTabletNodesOnly:
+		return component == consts.TabletNodeType
+	case ytv1.UpdateSelectorExecNodesOnly:
+		return component == consts.ExecNodeType
+	case ytv1.UpdateSelectorStatelessOnly:
+		switch component {
+		case consts.MasterType:
+			return false
+		case consts.DataNodeType:
+			return false
+		case consts.TabletNodeType:
+			return false
+		}
+		return true
+	case ytv1.UpdateSelectorEverything:
+		return true
+	default:
+		return false
+	}
+}
+
 // chooseUpdateFlow considers spec and decides if operator should proceed with update or block.
 // Block case is indicated with non-empty blockMsg.
 // If update is not blocked, updateMeta containing a chosen flow and the component names to update returned.
 func chooseUpdateFlow(spec ytv1.YtsaurusSpec, needUpdate []components.Component) (meta updateMeta, blockMsg string) {
-	isFullUpdateEnabled := spec.EnableFullUpdate
 	configuredSelector := spec.UpdateSelector
+	if configuredSelector == ytv1.UpdateSelectorUnspecified {
+		if spec.EnableFullUpdate {
+			configuredSelector = ytv1.UpdateSelectorEverything
+		} else {
+			configuredSelector = ytv1.UpdateSelectorStatelessOnly
+		}
+	}
+
+	var canUpdate []string
+	var cannotUpdate []string
+	needFullUpdate := false
 
-	masterNeedsUpdate := false
-	tabletNodesNeedUpdate := false
-	execNodesNeedUpdate := false
-	statelessNeedUpdate := false
-	var masterNames []string
-	var tabletNodeNames []string
-	var execNodeNames []string
-	var statelessNames []string
 	for _, comp := range needUpdate {
-		if comp.GetType() == consts.MasterType {
-			masterNeedsUpdate = true
-			masterNames = append(masterNames, comp.GetName())
-			continue
-		}
-		if comp.GetType() == consts.TabletNodeType {
-			tabletNodesNeedUpdate = true
-			tabletNodeNames = append(tabletNodeNames, comp.GetName())
-			continue
-		}
-		if comp.GetType() == consts.ExecNodeType {
-			execNodesNeedUpdate = true
-			execNodeNames = append(execNodeNames, comp.GetName())
-		}
-		statelessNames = append(statelessNames, comp.GetName())
-		statelessNeedUpdate = true
+		component := comp.GetType()
+		if canUpdateComponent(configuredSelector, component) {
+			canUpdate = append(canUpdate, string(component))
+		} else {
+			cannotUpdate = append(cannotUpdate, string(component))
+		}
+		if !canUpdateComponent(ytv1.UpdateSelectorStatelessOnly, component) && component != consts.DataNodeType {
+			needFullUpdate = true
+		}
 	}
-	statefulNeedUpdate := masterNeedsUpdate || tabletNodesNeedUpdate
-
-	allNamesNeedingUpdate := getComponentNames(needUpdate)
 
-	// Fallback to EnableFullUpdate field.
-	if configuredSelector == ytv1.UpdateSelectorUnspecified {
-		if statefulNeedUpdate {
-			if isFullUpdateEnabled {
-				return updateMeta{flow: ytv1.UpdateFlowFull, componentNames: nil}, ""
-			} else {
-				return updateMeta{flow: "", componentNames: nil}, "Full update is not allowed by enableFullUpdate field, ignoring it"
-			}
+	if len(canUpdate) == 0 {
+		if len(cannotUpdate) != 0 {
+			return updateMeta{}, fmt.Sprintf("All components allowed by updateSelector are uptodate, update of {%s} is not allowed", strings.Join(cannotUpdate, ", "))
 		}
-		return updateMeta{flow: ytv1.UpdateFlowStateless, componentNames: allNamesNeedingUpdate}, ""
+		return updateMeta{}, "All components are uptodate"
 	}
 
 	switch configuredSelector {
-	case ytv1.UpdateSelectorNothing:
-		return updateMeta{}, "All updates are blocked by updateSelector field."
 	case ytv1.UpdateSelectorEverything:
-		if statefulNeedUpdate {
+		if needFullUpdate {
 			return updateMeta{
 				flow:           ytv1.UpdateFlowFull,
 				componentNames: nil,
 			}, ""
 		} else {
 			return updateMeta{
 				flow:           ytv1.UpdateFlowStateless,
-				componentNames: allNamesNeedingUpdate,
+				componentNames: canUpdate,
 			}, ""
 		}
 	case ytv1.UpdateSelectorMasterOnly:
-		if !masterNeedsUpdate {
-			return updateMeta{}, "Only Master update is allowed by updateSelector, but it doesn't need update"
-		}
 		return updateMeta{
 			flow:           ytv1.UpdateFlowMaster,
-			componentNames: masterNames,
+			componentNames: canUpdate,
 		}, ""
 	case ytv1.UpdateSelectorTabletNodesOnly:
-		if !tabletNodesNeedUpdate {
-			return updateMeta{}, "Only Tablet nodes update is allowed by updateSelector, but they don't need update"
-		}
 		return updateMeta{
 			flow:           ytv1.UpdateFlowTabletNodes,
-			componentNames: tabletNodeNames,
-		}, ""
-	case ytv1.UpdateSelectorExecNodesOnly:
-		if !execNodesNeedUpdate {
-			return updateMeta{}, "Only Exec nodes update is allowed by updateSelector, but they don't need update"
-		}
-		return updateMeta{
-			flow:           ytv1.UpdateFlowStateless,
-			componentNames: execNodeNames,
+			componentNames: canUpdate,
 		}, ""
-	case ytv1.UpdateSelectorStatelessOnly:
-		if !statelessNeedUpdate {
-			return updateMeta{}, "Only stateless components update is allowed by updateSelector, but they don't need update"
-		}
+	case ytv1.UpdateSelectorDataNodesOnly, ytv1.UpdateSelectorExecNodesOnly, ytv1.UpdateSelectorStatelessOnly:
 		return updateMeta{
 			flow:           ytv1.UpdateFlowStateless,
-			componentNames: statelessNames,
+			componentNames: canUpdate,
 		}, ""
 	default:
 		return updateMeta{}, fmt.Sprintf("Unexpected update selector %s", configuredSelector)
@@ -548,7 +535,7 @@ func (r *YtsaurusReconciler) Sync(ctx context.Context, resource *ytv1.Ytsaurus)
 			err := ytsaurus.SaveClusterState(ctx, ytv1.ClusterStateReconfiguration)
 			return ctrl.Result{Requeue: true}, err
 
-		case needUpdate != nil:
+		case len(needUpdate) != 0:
 			var needUpdateNames []string
 			for _, c := range needUpdate {
 				needUpdateNames = append(needUpdateNames, c.GetName())

diff --git a/docs/api.md b/docs/api.md
@@ -1836,7 +1836,7 @@ _Appears in:_
 | `oauthService` _[OauthServiceSpec](#oauthservicespec)_ |  |  |  |
 | `isManaged` _boolean_ |  | true |  |
 | `enableFullUpdate` _boolean_ |  | true |  |
-| `updateSelector` _[UpdateSelector](#updateselector)_ | UpdateSelector is an experimental field. Behaviour may change.<br />If UpdateSelector is not empty EnableFullUpdate is ignored. |  | Enum: [ Nothing StatelessOnly MasterOnly TabletNodesOnly ExecNodesOnly Everything] <br /> |
+| `updateSelector` _[UpdateSelector](#updateselector)_ | UpdateSelector is an experimental field. Behaviour may change.<br />If UpdateSelector is not empty EnableFullUpdate is ignored. |  | Enum: [ Nothing MasterOnly DataNodesOnly TabletNodesOnly ExecNodesOnly StatelessOnly Everything] <br /> |
 | `nodeSelector` _object (keys:string, values:string)_ |  |  |  |
 | `tolerations` _[Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#toleration-v1-core) array_ |  |  |  |
 | `bootstrap` _[BootstrapSpec](#bootstrapspec)_ |  |  |  |

diff --git a/test/e2e/ytsaurus_controller_test.go b/test/e2e/ytsaurus_controller_test.go
@@ -432,7 +432,7 @@ var _ = Describe("Basic test for Ytsaurus controller", func() {
 				ytsaurus.Spec.Discovery.InstanceCount += 1
 				Expect(k8sClient.Update(ctx, ytsaurus)).Should(Succeed())
 				EventuallyYtsaurus(ctx, name, reactionTimeout).Should(
-					HaveClusterUpdatingComponents("Discovery", "DataNode", "HttpProxy", "ExecNode", "Scheduler", "ControllerAgent"),
+					HaveClusterUpdatingComponents("Discovery", "HttpProxy", "ExecNode", "Scheduler", "ControllerAgent"),
 				)
 				By("Wait cluster update with selector:StatelessOnly complete")
 				EventuallyYtsaurus(ctx, name, upgradeTimeout).Should(HaveClusterState(ytv1.ClusterStateRunning))
@@ -443,10 +443,8 @@ var _ = Describe("Basic test for Ytsaurus controller", func() {
 				Expect(podDiff.created.Equal(NewStringSetFromItems("ds-1", "ds-2"))).To(
 					BeTrue(), "unexpected pod diff created %v", podDiff.created)
 				Expect(podDiff.deleted.IsEmpty()).To(BeTrue(), "unexpected pod diff deleted %v", podDiff.deleted)
-				statelessUpdatedPods := NewStringSetFromMap(podsAfterStatelessUpdate).Difference(
-					NewStringSetFromItems("ms-0", "tnd-0", "tnd-1", "tnd-2", "ds-1", "ds-2"))
 				Expect(podDiff.recreated.Equal(
-					statelessUpdatedPods),
+					NewStringSetFromItems("ca-0", "end-0", "sch-0", "hp-0", "ds-0")),
 				).To(BeTrue(), "unexpected pod diff recreated %v", podDiff.recreated)
 			},
 		)

diff --git a/ytop-chart/templates/crds/ytsaurus.cluster.ytsaurus.tech.yaml b/ytop-chart/templates/crds/ytsaurus.cluster.ytsaurus.tech.yaml
@@ -34648,10 +34648,11 @@ spec:
                 enum:
                 - ""
                 - Nothing
-                - StatelessOnly
                 - MasterOnly
+                - DataNodesOnly
                 - TabletNodesOnly
                 - ExecNodesOnly
+                - StatelessOnly
                 - Everything
                 type: string
               useIpv4: