From ed49448dcf60f59c72d3118be73033125928e59f Mon Sep 17 00:00:00 2001 From: Kartikeya Pharasi Date: Thu, 19 Dec 2024 12:25:57 -0800 Subject: [PATCH] If present sort GTPs based on update time (#368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 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 Co-authored-by: Kartikeya Pharasi --- admiral/pkg/controller/common/common.go | 20 +- admiral/pkg/controller/common/common_test.go | 186 +++++++++++++++++++ 2 files changed, 201 insertions(+), 5 deletions(-) diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index e724a1a2..dead13d1 100644 --- 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 index fe1586fb..5e2c6efa 100644 --- 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]) + } + } + + }) + } +}