Tests for Controller Revision (#297)

* added unit tests for revision, and integration tests for it * removed unnecessary log * added check for revisions in history * addressed comments * addressed second round of comments * added selector for workers, changed from expect to eventually when checking leaderPod delete status * added test to simluate lws update, currently failing * fixed bug with revision test * fixed for cr test
kubernetes-sigs · Jan 3, 2025 · 7a3cd29 · 7a3cd29
1 parent e69c972
commit 7a3cd29
Show file tree

Hide file tree

Showing 6 changed files with 609 additions and 23 deletions.
diff --git a/pkg/controllers/leaderworkerset_controller.go b/pkg/controllers/leaderworkerset_controller.go
@@ -577,7 +577,7 @@ func (r *LeaderWorkerSetReconciler) getOrCreateRevisionIfNonExist(ctx context.Co
 		// the revisionKey was used to detect update instead of controller revision.
 		revisionKey = revisionutils.GetRevisionKey(sts)
 	}
-	if stsRevision, err := revisionutils.GetRevision(ctx, r.Client, lws, revisionKey); sts != nil || err != nil {
+	if stsRevision, err := revisionutils.GetRevision(ctx, r.Client, lws, revisionKey); stsRevision != nil || err != nil {
 		return stsRevision, err
 	}
 	revision, err := revisionutils.NewRevision(ctx, r.Client, lws, revisionKey)

diff --git a/pkg/utils/revision/revision_utils.go b/pkg/utils/revision/revision_utils.go
@@ -1,3 +1,19 @@
+/*
+Copyright 2025.
+
+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.
+*/
+
 package revision
 
 import (
@@ -97,7 +113,8 @@ func GetRevision(ctx context.Context, k8sClient client.Client, lws *leaderworker
 		return nil, nil
 	}
 	selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{MatchLabels: map[string]string{
-		leaderworkerset.RevisionKey: revisionKey,
+		leaderworkerset.SetNameLabelKey: lws.Name,
+		leaderworkerset.RevisionKey:     revisionKey,
 	}})
 	if err != nil {
 		return nil, err
@@ -170,10 +187,6 @@ func EqualRevision(lhs *appsv1.ControllerRevision, rhs *appsv1.ControllerRevisio
 		return lhs == rhs
 	}
 
-	if GetRevisionKey(lhs) == GetRevisionKey(rhs) {
-		return true
-	}
-
 	return bytes.Equal(lhs.Data.Raw, rhs.Data.Raw) && apiequality.Semantic.DeepEqual(lhs.Data.Object, rhs.Data.Object)
 }
 

diff --git a/pkg/utils/revision/revision_utils_test.go b/pkg/utils/revision/revision_utils_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/ptr"
@@ -31,7 +32,7 @@ import (
 func TestApplyRevision(t *testing.T) {
 	client := fake.NewClientBuilder().Build()
 
-	lws := BuildLeaderWorkerSet("default")
+	lws := BuildLeaderWorkerSet("default").Obj()
 	revision, err := NewRevision(context.TODO(), client, lws, "")
 	if err != nil {
 		t.Fatal(err)
@@ -77,7 +78,159 @@ func TestApplyRevision(t *testing.T) {
 	}
 }
 
-func BuildLeaderWorkerSet(nsName string) *leaderworkerset.LeaderWorkerSet {
+func TestEqualRevision(t *testing.T) {
+	client := fake.NewClientBuilder().Build()
+	tests := []struct {
+		name             string
+		leftLws          *leaderworkerset.LeaderWorkerSet
+		rightLws         *leaderworkerset.LeaderWorkerSet
+		leftRevisionKey  string
+		rightRevisionKey string
+		equal            bool
+	}{
+		{
+			name:             "same LeaderWorkerTemplate, networkConfig, should be equal",
+			leftLws:          BuildLeaderWorkerSet("default").Obj(),
+			rightLws:         BuildLeaderWorkerSet("default").Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            true,
+		},
+		{
+			name:             "same LeaderWorkerTemplate, networkConfig, different revisionKey, should be equal",
+			leftLws:          BuildLeaderWorkerSet("default").Obj(),
+			rightLws:         BuildLeaderWorkerSet("default").Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "templateHash",
+			equal:            true,
+		},
+		{
+			name:             "same LeaderWorkerTemplate, shared subdomainpolicy & nil, should be equal",
+			leftLws:          BuildLeaderWorkerSet("default").SubdomainPolicy(leaderworkerset.SubdomainShared).Obj(),
+			rightLws:         BuildLeaderWorkerSet("default").SubdomainNil().Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            true,
+		},
+		{
+			name:             "left nil, right nil, should be equal",
+			leftLws:          nil,
+			rightLws:         nil,
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            true,
+		},
+		{
+			name:             "semantically same LeaderWorkerTemplate, different fields set, same networkConfig, should be equal",
+			leftLws:          BuildLeaderWorkerSet("default").WorkerTemplateSpec(MakeWorkerPodSpecWithVolumeAndNilImage()).Obj(),
+			rightLws:         BuildLeaderWorkerSet("default").WorkerTemplateSpec(MakeWorkerPodSpecWithVolume()).Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            true,
+		},
+		{
+			name:             "left nil, right non-nil, should not be equal",
+			leftLws:          nil,
+			rightLws:         BuildLeaderWorkerSet("default").Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            false,
+		},
+		{
+			name:             "same LeaderWorkerTemplate, different networkConfig, should not be equal",
+			leftLws:          BuildLeaderWorkerSet("default").SubdomainPolicy(leaderworkerset.SubdomainUniquePerReplica).Obj(),
+			rightLws:         BuildLeaderWorkerSet("default").Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            false,
+		},
+		{
+			name:             "different LeaderWorkerTemplate, same networkConfig, should not be equal",
+			leftLws:          BuildLeaderWorkerSet("default").Obj(),
+			rightLws:         BuildLeaderWorkerSet("default").WorkerTemplateSpec(MakeLeaderPodSpec()).Obj(),
+			leftRevisionKey:  "",
+			rightRevisionKey: "",
+			equal:            false,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			var leftRevision *appsv1.ControllerRevision
+			var rightRevision *appsv1.ControllerRevision
+			var err error
+			if tc.leftLws != nil {
+				leftRevision, err = NewRevision(context.TODO(), client, tc.leftLws, tc.leftRevisionKey)
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+			if tc.rightLws != nil {
+				rightRevision, err = NewRevision(context.TODO(), client, tc.rightLws, tc.rightRevisionKey)
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+			equal := EqualRevision(leftRevision, rightRevision)
+			if tc.equal != equal {
+				t.Errorf("Expected equality between controller revisions to be %t, but was %t", tc.equal, equal)
+			}
+		})
+	}
+}
+
+func TestGetHighestRevision(t *testing.T) {
+	client := fake.NewClientBuilder().Build()
+	lws := BuildLeaderWorkerSet("default").Obj()
+	revision1, err := NewRevision(context.TODO(), client, lws, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	revision2 := revision1.DeepCopy()
+	revision2.Revision = 2
+	revision3 := revision2.DeepCopy()
+	revision3.Revision = 3
+	tests := []struct {
+		name             string
+		revisions        []*appsv1.ControllerRevision
+		expectedRevision *appsv1.ControllerRevision
+	}{
+		{
+			name:             "empty revision list, returns nil",
+			revisions:        []*appsv1.ControllerRevision{},
+			expectedRevision: nil,
+		},
+		{
+			name:             "only one revision in list, returns it",
+			revisions:        []*appsv1.ControllerRevision{revision1},
+			expectedRevision: revision1,
+		},
+		{
+			name:             "returns the revision with highest revision number",
+			revisions:        []*appsv1.ControllerRevision{revision2, revision3, revision2},
+			expectedRevision: revision3,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			revision := getHighestRevision(tc.revisions)
+			if tc.expectedRevision == nil {
+				if revision != nil {
+					t.Errorf("Expected revision to be nil")
+				}
+			} else {
+				if tc.expectedRevision.Revision != revision.Revision {
+					t.Errorf("Expected revision number to be %d, but it was %d", tc.expectedRevision.Revision, revision.Revision)
+				}
+			}
+		})
+	}
+}
+
+type LeaderWorkerSetWrapper struct {
+	leaderworkerset.LeaderWorkerSet
+}
+
+func BuildLeaderWorkerSet(nsName string) *LeaderWorkerSetWrapper {
 	lws := leaderworkerset.LeaderWorkerSet{}
 	lws.Name = "test-sample"
 	lws.Namespace = nsName
@@ -102,7 +255,34 @@ func BuildLeaderWorkerSet(nsName string) *leaderworkerset.LeaderWorkerSet {
 		SubdomainPolicy: &subdomainPolicy,
 	}
 
-	return &lws
+	return &LeaderWorkerSetWrapper{
+		lws,
+	}
+}
+
+func (lwsWrapper *LeaderWorkerSetWrapper) Obj() *leaderworkerset.LeaderWorkerSet {
+	return &lwsWrapper.LeaderWorkerSet
+}
+
+func (lwsWrapper *LeaderWorkerSetWrapper) SubdomainPolicy(subdomainPolicy leaderworkerset.SubdomainPolicy) *LeaderWorkerSetWrapper {
+	lwsWrapper.Spec.NetworkConfig = &leaderworkerset.NetworkConfig{
+		SubdomainPolicy: &subdomainPolicy,
+	}
+	return lwsWrapper
+}
+func (lwsWrapper *LeaderWorkerSetWrapper) SubdomainNil() *LeaderWorkerSetWrapper {
+	lwsWrapper.Spec.NetworkConfig = nil
+	return lwsWrapper
+}
+
+func (lwsWrapper *LeaderWorkerSetWrapper) MaxUnavailable(value int) *LeaderWorkerSetWrapper {
+	lwsWrapper.Spec.RolloutStrategy.RollingUpdateConfiguration.MaxUnavailable = intstr.FromInt(value)
+	return lwsWrapper
+}
+
+func (lwsWrapper *LeaderWorkerSetWrapper) MaxSurge(value int) *LeaderWorkerSetWrapper {
+	lwsWrapper.Spec.RolloutStrategy.RollingUpdateConfiguration.MaxSurge = intstr.FromInt(value)
+	return lwsWrapper
 }
 
 func MakeLeaderPodSpec() corev1.PodSpec {
@@ -132,3 +312,55 @@ func MakeWorkerPodSpec() corev1.PodSpec {
 		},
 	}
 }
+
+func MakeWorkerPodSpecWithVolume() corev1.PodSpec {
+	return corev1.PodSpec{
+		Containers: []corev1.Container{
+			{
+				Name:  "leader",
+				Image: "nginx:1.14.2",
+				Ports: []corev1.ContainerPort{
+					{
+						ContainerPort: 8080,
+						Protocol:      "TCP",
+					},
+				},
+			},
+		},
+		Volumes: []corev1.Volume{
+			{
+				Name: "dshm",
+			},
+		},
+	}
+}
+
+func MakeWorkerPodSpecWithVolumeAndNilImage() corev1.PodSpec {
+	return corev1.PodSpec{
+		Containers: []corev1.Container{
+			{
+				Name:  "leader",
+				Image: "nginx:1.14.2",
+				Ports: []corev1.ContainerPort{
+					{
+						ContainerPort: 8080,
+						Protocol:      "TCP",
+					},
+				},
+			},
+		},
+		Volumes: []corev1.Volume{
+			{
+				Name: "dshm",
+				VolumeSource: corev1.VolumeSource{
+					Image: nil,
+				},
+			},
+		},
+	}
+}
+
+func (lwsWrapper *LeaderWorkerSetWrapper) WorkerTemplateSpec(spec corev1.PodSpec) *LeaderWorkerSetWrapper {
+	lwsWrapper.Spec.LeaderWorkerTemplate.WorkerTemplate.Spec = spec
+	return lwsWrapper
+}