[Bugfix] Handle optional taints for Storage Operator (#1495)

CHANGELOG.md

@@ -14,6 +14,7 @@
 - (Bugfix) Fix Replaced Member Zone during Replace operation
 - (Feature) (ML) Handlers
 - (Feature) Add P0 Compare Func
+- (Bugfix) Handle optional taints for Storage Operator
 
 ## [1.2.35](https://github.com/arangodb/kube-arangodb/tree/1.2.35) (2023-11-06)
 - (Maintenance) Update go-driver to v1.6.0, update IsNotFound() checks

Makefile

@@ -649,7 +649,7 @@ run-unit-tests: $(SOURCES)
 		$(REPOPATH)/pkg/apis/replication/... \
 		$(REPOPATH)/pkg/apis/storage/... \
 		$(REPOPATH)/pkg/deployment/... \
-		$(REPOPATH)/pkg/storage \
+		$(REPOPATH)/pkg/storage/... \
 	    $(REPOPATH)/pkg/crd/... \
 		$(REPOPATH)/pkg/util/... \
 		$(REPOPATH)/cmd/... \

pkg/storage/pv_creator.go

@@ -130,7 +130,7 @@ func (ls *LocalStorage) createPVs(ctx context.Context, apiObject *api.ArangoLoca
 				continue
 			}
 
-			nodeList = nodeList.FilterSchedulable().FilterPodsTaints(podList)
+			nodeList = nodeList.FilterSchedulable().FilterPodsTaints(podList).SortBySchedulablePodsTaints(podList)
 
 			allowedClients = allowedClients.Filter(func(node string, client provisioner.API) bool {
 				for _, n := range nodeList {

pkg/storage/resources/nodes.go

@@ -22,6 +22,7 @@ package resources
 
 import (
 	"context"
+	"sort"
 
 	core "k8s.io/api/core/v1"
 	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -45,21 +46,41 @@ func (p Nodes) Filter(f func(node *core.Node) bool) Nodes {
 	return r
 }
 
+func (p Nodes) Copy() Nodes {
+	c := make(Nodes, len(p))
+	copy(c, p)
+	return c
+}
+
+func (p Nodes) Sort(f func(a, b *core.Node) bool) Nodes {
+	z := p.Copy()
+	sort.Slice(z, func(i, j int) bool {
+		return f(z[i], z[j])
+	})
+	return z
+}
+
+func (p Nodes) SortBySchedulablePodsTaints(pods Pods) Nodes {
+	return p.Sort(func(a, b *core.Node) bool {
+		return utils.IsNodeSchedulableForPods(a, pods...) > utils.IsNodeSchedulableForPods(b, pods...)
+	})
+}
+
+func (p Nodes) SortBySchedulablePodTaints(pod *core.Pod) Nodes {
+	return p.Sort(func(a, b *core.Node) bool {
+		return utils.IsNodeSchedulableForPod(a, pod) > utils.IsNodeSchedulableForPod(b, pod)
+	})
+}
+
 func (p Nodes) FilterPodsTaints(pods Pods) Nodes {
 	return p.Filter(func(node *core.Node) bool {
-		for _, pod := range pods {
-			if utils.IsNodeSchedulableForPod(node, pod) {
-				return true
-			}
-		}
-
-		return false
+		return utils.IsNodeSchedulableForPods(node, pods...).Schedulable()
 	})
 }
 
 func (p Nodes) FilterTaints(pod *core.Pod) Nodes {
 	return p.Filter(func(node *core.Node) bool {
-		return utils.IsNodeSchedulableForPod(node, pod)
+		return utils.IsNodeSchedulableForPod(node, pod).Schedulable()
 	})
 }
 

pkg/storage/resources/nodes_test.go

@@ -0,0 +1,179 @@
+//
+// DISCLAIMER
+//
+// Copyright 2023 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 resources
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	core "k8s.io/api/core/v1"
+	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/arangodb/kube-arangodb/pkg/storage/utils"
+)
+
+func Test_Nodes_OptionalScheduling(t *testing.T) {
+	nodes := Nodes{
+		{
+			ObjectMeta: meta.ObjectMeta{
+				Name: "NodeA",
+			},
+			Spec: core.NodeSpec{
+				Taints: []core.Taint{
+					{
+						Key:    "test2",
+						Value:  "test",
+						Effect: core.TaintEffectPreferNoSchedule,
+					},
+				},
+			},
+		},
+		{
+			ObjectMeta: meta.ObjectMeta{
+				Name: "NodeB",
+			},
+			Spec: core.NodeSpec{
+				Taints: []core.Taint{
+					{
+						Key:    "test1",
+						Value:  "test",
+						Effect: core.TaintEffectNoSchedule,
+					},
+				},
+			},
+		},
+		{
+			ObjectMeta: meta.ObjectMeta{
+				Name: "NodeC",
+			},
+			Spec: core.NodeSpec{
+				Taints: []core.Taint{},
+			},
+		},
+	}
+
+	t.Run("Without tolerations", func(t *testing.T) {
+		require.Len(t, nodes.FilterTaints(&core.Pod{
+			Spec: core.PodSpec{
+				Tolerations: []core.Toleration{},
+			},
+		}), 2)
+	})
+
+	t.Run("With toleration - match fully", func(t *testing.T) {
+		require.Len(t, nodes.FilterTaints(&core.Pod{
+			Spec: core.PodSpec{
+				Tolerations: []core.Toleration{
+					{
+						Key:      "test1",
+						Operator: core.TolerationOpEqual,
+						Value:    "test",
+						Effect:   core.TaintEffectNoSchedule,
+					},
+				},
+			},
+		}), 3)
+	})
+
+	t.Run("With toleration - invalid effect", func(t *testing.T) {
+		require.Len(t, nodes.FilterTaints(&core.Pod{
+			Spec: core.PodSpec{
+				Tolerations: []core.Toleration{
+					{
+						Key:      "test1",
+						Operator: core.TolerationOpEqual,
+						Value:    "test",
+						Effect:   core.TaintEffectNoExecute,
+					},
+				},
+			},
+		}), 2)
+	})
+
+	t.Run("With toleration - invalid value", func(t *testing.T) {
+		require.Len(t, nodes.FilterTaints(&core.Pod{
+			Spec: core.PodSpec{
+				Tolerations: []core.Toleration{
+					{
+						Key:      "test1",
+						Operator: core.TolerationOpEqual,
+						Value:    "test1",
+						Effect:   core.TaintEffectNoSchedule,
+					},
+				},
+			},
+		}), 2)
+	})
+
+	t.Run("With toleration - invalid key", func(t *testing.T) {
+		require.Len(t, nodes.FilterTaints(&core.Pod{
+			Spec: core.PodSpec{
+				Tolerations: []core.Toleration{
+					{
+						Key:      "test",
+						Operator: core.TolerationOpEqual,
+						Value:    "test",
+						Effect:   core.TaintEffectNoSchedule,
+					},
+				},
+			},
+		}), 2)
+	})
+
+	t.Run("With toleration - exists", func(t *testing.T) {
+		require.Len(t, nodes.FilterTaints(&core.Pod{
+			Spec: core.PodSpec{
+				Tolerations: []core.Toleration{
+					{
+						Key:      "test1",
+						Operator: core.TolerationOpExists,
+						Value:    "test-445",
+						Effect:   core.TaintEffectNoSchedule,
+					},
+				},
+			},
+		}), 3)
+	})
+
+	t.Run("Nodes order by optional", func(t *testing.T) {
+		pod := &core.Pod{
+			Spec: core.PodSpec{},
+		}
+
+		sorted := nodes.SortBySchedulablePodTaints(pod)
+
+		actual := make([]utils.ScheduleOption, len(nodes))
+		schedules := make([]utils.ScheduleOption, len(sorted))
+
+		for id := range sorted {
+			actual[id] = utils.IsNodeSchedulableForPod(nodes[id], pod)
+			schedules[id] = utils.IsNodeSchedulableForPod(sorted[id], pod)
+		}
+
+		require.Equal(t, utils.ScheduleAllowed, schedules[0])
+		require.Equal(t, utils.ScheduleOptional, schedules[1])
+		require.Equal(t, utils.ScheduleBlocked, schedules[2])
+
+		require.Equal(t, utils.ScheduleOptional, actual[0])
+		require.Equal(t, utils.ScheduleBlocked, actual[1])
+		require.Equal(t, utils.ScheduleAllowed, actual[2])
+	})
+}

pkg/storage/utils/taints.go

@@ -1,7 +1,7 @@
 //
 // DISCLAIMER
 //
-// Copyright 2016-2022 ArangoDB GmbH, Cologne, Germany
+// Copyright 2016-2023 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.
@@ -26,21 +26,61 @@ import (
 	core "k8s.io/api/core/v1"
 )
 
-func IsNodeSchedulableForPod(node *core.Node, pod *core.Pod) bool {
-	return AreTaintsTolerated(pod.Spec.Tolerations, node.Spec.Taints)
+type ScheduleOption int
+
+const (
+	ScheduleBlocked ScheduleOption = iota
+	ScheduleOptional
+	ScheduleAllowed
+)
+
+func (s ScheduleOption) Schedulable() bool {
+	switch s {
+	case ScheduleOptional, ScheduleAllowed:
+		return true
+	default:
+		return false
+	}
 }
 
-func AreTaintsTolerated(tolerations []core.Toleration, taints []core.Taint) bool {
-	for _, taint := range taints {
-		if !IsTaintTolerated(tolerations, taint) {
-			return false
+func IsNodeSchedulableForPods(node *core.Node, pods ...*core.Pod) ScheduleOption {
+	schedule := ScheduleAllowed
+
+	for _, pod := range pods {
+		if taintSchedule := IsNodeSchedulableForPod(node, pod); taintSchedule == ScheduleBlocked {
+			return ScheduleBlocked
+		} else if taintSchedule == ScheduleOptional {
+			schedule = ScheduleOptional
 		}
 	}
 
-	return true
+	return schedule
 }
 
-func IsTaintTolerated(tolerations []core.Toleration, taint core.Taint) bool {
+func IsNodeSchedulableForPod(node *core.Node, pod *core.Pod) ScheduleOption {
+	return AreTaintsTolerated(pod.Spec.Tolerations, node.Spec.Taints)
+}
+
+func AreTaintsTolerated(tolerations []core.Toleration, taints []core.Taint) ScheduleOption {
+	schedule := ScheduleAllowed
+
+	for _, taint := range taints {
+		if taintSchedule := IsTaintTolerated(tolerations, taint); taintSchedule == ScheduleBlocked {
+			return ScheduleBlocked
+		} else if taintSchedule == ScheduleOptional {
+			schedule = ScheduleOptional
+		}
+	}
+
+	return schedule
+}
+
+func IsTaintTolerated(tolerations []core.Toleration, taint core.Taint) ScheduleOption {
+	if taint.Effect == core.TaintEffectPreferNoSchedule {
+		// Taint is Soft one, schedule allowed
+		return ScheduleOptional
+	}
+
 	for _, toleration := range tolerations {
 		if toleration.Effect != "" && toleration.Effect != taint.Effect {
 			// Not same effect
@@ -80,8 +120,8 @@ func IsTaintTolerated(tolerations []core.Toleration, taint core.Taint) bool {
 			}
 		}
 
-		return true
+		return ScheduleAllowed
 	}
 
-	return false
+	return ScheduleBlocked
 }

pkg/storage/utils/taints_test.go

@@ -227,9 +227,9 @@ func Test_Taints(t *testing.T) {
 			schedulable := AreTaintsTolerated(c.tolerations, c.taints)
 
 			if c.schedulable {
-				require.True(t, schedulable)
+				require.True(t, schedulable.Schedulable())
 			} else {
-				require.False(t, schedulable)
+				require.False(t, schedulable.Schedulable())
 			}
 		})
 	}