If present sort GTPs based on update time (#368)

### Checklist 🚨 Please review this repository's [contribution guidelines](./CONTRIBUTING.md). - [ ] I've read and agree to the project's contribution guidelines. - [ ] I'm requesting to **pull a topic/feature/bugfix branch**. - [ ] I checked that my code additions will pass code linting checks and unit tests. - [ ] I updated unit and integration tests (if applicable). - [ ] I'm ready to notify the team of this contribution. ### Description What does this change do and why? [Link to related ISSUE] Thank you! --------- Signed-off-by: Kartikeya Pharasi <[email protected]> Co-authored-by: Kartikeya Pharasi <[email protected]>
istio-ecosystem · Dec 19, 2024 · ed49448 · ed49448
1 parent 82738c6
commit ed49448
Show file tree

Hide file tree

Showing 2 changed files with 201 additions and 5 deletions.
diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go
@@ -610,14 +610,24 @@ func SortGtpsByPriorityAndCreationTime(gtpsToOrder []*v1.GlobalTrafficPolicy, id
 		iPriority := getGtpPriority(gtpsToOrder[i])
 		jPriority := getGtpPriority(gtpsToOrder[j])
 
-		iTime := gtpsToOrder[i].CreationTimestamp
-		jTime := gtpsToOrder[j].CreationTimestamp
+		iCreationTime := gtpsToOrder[i].CreationTimestamp
+		jCreationTime := gtpsToOrder[j].CreationTimestamp
 		if iPriority != jPriority {
-			log.Debugf("GTP sorting identity=%s env=%s name1=%s creationTime1=%v priority1=%d name2=%s creationTime2=%v priority2=%d", identity, env, gtpsToOrder[i].Name, iTime, iPriority, gtpsToOrder[j].Name, jTime, jPriority)
+			log.Debugf("GTP sorting identity=%s env=%s name1=%s creationTime1=%v priority1=%d name2=%s creationTime2=%v priority2=%d", identity, env, gtpsToOrder[i].Name, iCreationTime, iPriority, gtpsToOrder[j].Name, jCreationTime, jPriority)
 			return iPriority > jPriority
 		}
-		log.Debugf("GTP sorting identity=%s env=%s name1=%s creationTime1=%v priority1=%d name2=%s creationTime2=%v priority2=%d", identity, env, gtpsToOrder[i].Name, iTime, iPriority, gtpsToOrder[j].Name, jTime, jPriority)
-		return iTime.After(jTime.Time)
+
+		if gtpsToOrder[i].Annotations != nil && gtpsToOrder[j].Annotations != nil {
+			if iUpdateTime, ok := gtpsToOrder[i].Annotations["lastUpdatedAt"]; ok {
+				if jUpdateTime, ok := gtpsToOrder[j].Annotations["lastUpdatedAt"]; ok {
+					log.Debugf("GTP sorting identity=%s env=%s name1=%s updateTime1=%v priority1=%d name2=%s updateTime2=%v priority2=%d", identity, env, gtpsToOrder[i].Name, iUpdateTime, iPriority, gtpsToOrder[j].Name, jUpdateTime, jPriority)
+					return iUpdateTime > jUpdateTime
+				}
+			}
+		}
+
+		log.Debugf("GTP sorting identity=%s env=%s name1=%s creationTime1=%v priority1=%d name2=%s creationTime2=%v priority2=%d", identity, env, gtpsToOrder[i].Name, iCreationTime, iPriority, gtpsToOrder[j].Name, jCreationTime, jPriority)
+		return iCreationTime.After(jCreationTime.Time)
 	})
 }
 

diff --git a/admiral/pkg/controller/common/common_test.go b/admiral/pkg/controller/common/common_test.go
@@ -1730,3 +1730,189 @@ func TestMakeDnsPrefixRegionMapping(t *testing.T) {
 		})
 	}
 }
+
+func TestSortGtpsByPriorityAndCreationTime(t *testing.T) {
+	identity := "test-identity"
+	env := "test"
+
+	creationTime1, _ := time.Parse(time.RFC3339, "2024-12-05T06:54:17Z")
+	creationTime2, _ := time.Parse(time.RFC3339, "2024-12-05T08:54:17Z")
+
+	testCases := []struct {
+		name        string
+		gtpsToOrder []*admiralV1Alpha1.GlobalTrafficPolicy
+		expected    []*admiralV1Alpha1.GlobalTrafficPolicy
+	}{
+		{
+			name: "Give lastUpdatedAt is set for both GTPs" +
+				"And priority is same" +
+				"Then sort GTPs based on lastUpdatedAt and ignore creation time",
+			gtpsToOrder: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime1},
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T06:54:17Z",
+						},
+						Labels: map[string]string{"priority": "10"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime2},
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T08:54:17Z",
+						},
+						Labels: map[string]string{"priority": "10"},
+					},
+				},
+			},
+			expected: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime2},
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T08:54:17Z",
+						},
+						Labels: map[string]string{"priority": "10"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime1},
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T06:54:17Z",
+						},
+						Labels: map[string]string{"priority": "10"},
+					},
+				},
+			},
+		},
+		{
+			name: "Give lastUpdatedAt is set for only one GTP" +
+				"And priority is same" +
+				"Then sort GTPs based on creation time and ignore lastUpdatedAt time",
+			gtpsToOrder: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime1},
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T06:54:17Z",
+						},
+						Labels: map[string]string{"priority": "10"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime2},
+						Labels:            map[string]string{"priority": "10"},
+					},
+				},
+			},
+			expected: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime2},
+						Labels:            map[string]string{"priority": "10"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime1},
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T06:54:17Z",
+						},
+						Labels: map[string]string{"priority": "10"},
+					},
+				},
+			},
+		},
+		{
+			name: "Give creationTime is set for both GTPs" +
+				"And priority is same and update time is not set" +
+				"Then sort GTPs based on creationTime",
+			gtpsToOrder: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime1},
+						Labels:            map[string]string{"priority": "10"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime2},
+						Labels:            map[string]string{"priority": "10"},
+					},
+				},
+			},
+			expected: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime2},
+						Labels:            map[string]string{"priority": "10"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						CreationTimestamp: v1.Time{creationTime1},
+						Labels:            map[string]string{"priority": "10"},
+					},
+				},
+			},
+		},
+		{
+			name: "Given priority is set for both GTPs and different" +
+				"Then sort GTPs based on priority and ignore other fields",
+			gtpsToOrder: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T08:54:17Z",
+						},
+						CreationTimestamp: v1.Time{creationTime1},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T06:54:17Z",
+						},
+						CreationTimestamp: v1.Time{creationTime2},
+						Labels:            map[string]string{"priority": "1000000"},
+					},
+				},
+			},
+			expected: []*admiralV1Alpha1.GlobalTrafficPolicy{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T08:54:17Z",
+						},
+						CreationTimestamp: v1.Time{creationTime1},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Annotations: map[string]string{
+							"lastUpdatedAt": "2024-12-05T06:54:17Z",
+						},
+						CreationTimestamp: v1.Time{creationTime2},
+						Labels:            map[string]string{"priority": "1000000"},
+					},
+				},
+			},
+		},
+	}
+
+	for _, c := range testCases {
+		t.Run(c.name, func(t *testing.T) {
+			SortGtpsByPriorityAndCreationTime(c.gtpsToOrder, identity, env)
+			for i := range c.gtpsToOrder {
+				if !reflect.DeepEqual(c.gtpsToOrder[i], c.expected[i]) {
+					t.Errorf("Wanted the GTP sorting at this index: %v to be: %v, got: %v", i, c.expected[i], c.gtpsToOrder[i])
+				}
+			}
+
+		})
+	}
+}