From 45a6977b4479a268811ca4be9a1d63facb9a736e Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Mon, 30 Mar 2026 13:33:37 -0700 Subject: [PATCH 1/6] Initial commit for adding default readiness groupos --- internal/resource/kind_ordering.go | 47 ++++++ internal/resource/kind_ordering_test.go | 183 ++++++++++++++++++++++++ internal/resource/resource.go | 12 ++ internal/resource/resource_test.go | 18 +-- 4 files changed, 251 insertions(+), 9 deletions(-) create mode 100644 internal/resource/kind_ordering.go create mode 100644 internal/resource/kind_ordering_test.go diff --git a/internal/resource/kind_ordering.go b/internal/resource/kind_ordering.go new file mode 100644 index 00000000..c8f66a40 --- /dev/null +++ b/internal/resource/kind_ordering.go @@ -0,0 +1,47 @@ +package resource + +// managedCreatedOrder maps infrastructure Kinds to reserved readiness groups +// Resources matching these kinds will have their readiness and deletion groups +// overridden to enforce a safe reconciliation order + +// Reserved Range -100 - -81. User groups should be >=-80 +// Deletion groups are the negation of the create groups +// Order is derived from Helm's InstallOrder/UninstallOrder +// https://github.com/helm/helm/blob/main/pkg/release/v1/util/kind_sorter.go +var managedCreateOrder = map[string]int{ + "PriorityClass": -100, + "Namespace": -100, + "NetworkPolicy": -99, + "ResourceQuota": -99, + "LimitRange": -99, + "PodSecurityPolicy": -98, + "PodDisruptionBudget": -98, + "ServiceAccount": -97, + "Secret": -96, + "SecretList": -96, + "ConfigMap": -96, + "StorageClass": -95, + "PersistentVolume": -94, + "PersistentVolumeClaim": -93, + "CustomResourceDefinition": -92, + "ClusterRole": -91, + "ClusterRoleList": -91, + "ClusterRoleBinding": -91, + "ClusterRoleBindingList": -91, + "Role": -90, + "RoleList": -90, + "RoleBinding": -90, + "RoleBindingList": -90, + "Service": -89, +} + +// applyManagedOrdering overrides the readiness and deletion group for managed infrastructure kinds +func applyManagedOrdering(res *Resource) { + createGrp, ok := managedCreateOrder[res.GVK.Kind] + if !ok { + return + } + res.readinessGroup = createGrp + delGroup := -createGrp + res.deletionGroup = &delGroup +} diff --git a/internal/resource/kind_ordering_test.go b/internal/resource/kind_ordering_test.go new file mode 100644 index 00000000..f8deb31d --- /dev/null +++ b/internal/resource/kind_ordering_test.go @@ -0,0 +1,183 @@ +package resource + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestManagedCreateOrderCoversExpectedKinds(t *testing.T) { + expectedKinds := []string{ + "PriorityClass", "Namespace", "NetworkPolicy", "ResourceQuota", + "LimitRange", "PodSecurityPolicy", "PodDisruptionBudget", + "ServiceAccount", "Secret", "SecretList", "ConfigMap", + "StorageClass", "PersistentVolume", "PersistentVolumeClaim", + "CustomResourceDefinition", "ClusterRole", "ClusterRoleList", + "ClusterRoleBinding", "ClusterRoleBindingList", + "Role", "RoleList", "RoleBinding", "RoleBindingList", "Service", + } + for _, kind := range expectedKinds { + _, ok := managedCreateOrder[kind] + assert.True(t, ok, "expected kind %q to be in managedCreateOrder", kind) + } +} + +func TestManagedCreateOrderGroupRange(t *testing.T) { + for kind, grp := range managedCreateOrder { + assert.GreaterOrEqual(t, grp, -100, "kind %q group %d below minimum", kind, grp) + assert.LessOrEqual(t, grp, -81, "kind %q group %d above reserved max", kind, grp) + } +} + +func TestApplyManagedOrdering_ManagedKind(t *testing.T) { + tests := []struct { + kind string + expectedCreate int + expectedDeletion int + }{ + {"Namespace", -100, 100}, + {"PriorityClass", -100, 100}, + {"ServiceAccount", -97, 97}, + {"Secret", -96, 96}, + {"ConfigMap", -96, 96}, + {"CustomResourceDefinition", -92, 92}, + {"ClusterRole", -91, 91}, + {"Role", -90, 90}, + {"Service", -89, 89}, + } + + for _, tc := range tests { + t.Run(tc.kind, func(t *testing.T) { + res := &Resource{} + res.GVK.Kind = tc.kind + res.readinessGroup = 5 // user tried to set a custom group + + applyManagedOrdering(res) + + assert.Equal(t, tc.expectedCreate, res.readinessGroup) + require.NotNil(t, res.deletionGroup) + assert.Equal(t, tc.expectedDeletion, *res.deletionGroup) + }) + } +} + +func TestApplyManagedOrdering_UnmanagedKind(t *testing.T) { + unmanagedKinds := []string{ + "Deployment", "StatefulSet", "DaemonSet", "Job", "CronJob", + "Ingress", "IngressClass", "HorizontalPodAutoscaler", + "MutatingWebhookConfiguration", "ValidatingWebhookConfiguration", + "APIService", "Pod", "ReplicaSet", "ReplicationController", + } + for _, kind := range unmanagedKinds { + t.Run(kind, func(t *testing.T) { + res := &Resource{} + res.GVK.Kind = kind + res.readinessGroup = 3 + + applyManagedOrdering(res) + + assert.Equal(t, 3, res.readinessGroup, "group should remain user-defined") + assert.Nil(t, res.deletionGroup, "should have no auto-assigned deletion group") + }) + } +} + +func TestApplyManagedOrdering_DeletionIsReverseOfCreate(t *testing.T) { + for kind, createGrp := range managedCreateOrder { + t.Run(kind, func(t *testing.T) { + res := &Resource{} + res.GVK.Kind = kind + applyManagedOrdering(res) + + require.NotNil(t, res.deletionGroup) + assert.Equal(t, -createGrp, *res.deletionGroup, + "deletion group should be negation of create group") + }) + } +} + +func TestApplyManagedOrdering_OverridesUserAnnotations(t *testing.T) { + // User sets readiness-group=5 and deletion-group=10 on a Namespace. + // Both should be overridden. + res := &Resource{} + res.GVK.Kind = "Namespace" + res.readinessGroup = 5 + delGrp := 10 + res.deletionGroup = &delGrp + + applyManagedOrdering(res) + + assert.Equal(t, -100, res.readinessGroup) + require.NotNil(t, res.deletionGroup) + assert.Equal(t, 100, *res.deletionGroup) +} + +func TestManagedOrderingPrecedence(t *testing.T) { + // Namespace/PriorityClass (-100) before everything + assert.Less(t, managedCreateOrder["Namespace"], managedCreateOrder["ServiceAccount"]) + // ServiceAccount (-97) before Secret/ConfigMap (-96) + assert.Less(t, managedCreateOrder["ServiceAccount"], managedCreateOrder["Secret"]) + assert.Less(t, managedCreateOrder["ServiceAccount"], managedCreateOrder["ConfigMap"]) + // StorageClass (-95) before PV (-94) before PVC (-93) + assert.Less(t, managedCreateOrder["StorageClass"], managedCreateOrder["PersistentVolume"]) + assert.Less(t, managedCreateOrder["PersistentVolume"], managedCreateOrder["PersistentVolumeClaim"]) + // PVC (-93) before CRD (-92) + assert.Less(t, managedCreateOrder["PersistentVolumeClaim"], managedCreateOrder["CustomResourceDefinition"]) + // CRD (-92) before ClusterRole (-91) + assert.Less(t, managedCreateOrder["CustomResourceDefinition"], managedCreateOrder["ClusterRole"]) + // ClusterRole (-91) before Role (-90) + assert.Less(t, managedCreateOrder["ClusterRole"], managedCreateOrder["Role"]) + // Role (-90) before Service (-89) + assert.Less(t, managedCreateOrder["Role"], managedCreateOrder["Service"]) +} + +func TestManagedOrderingDeletionPrecedence(t *testing.T) { + // Deletion order is reversed: Service deleted first, Namespace last + getDelGrp := func(kind string) int { + res := &Resource{} + res.GVK.Kind = kind + applyManagedOrdering(res) + return *res.deletionGroup + } + + // Service (+89) < Role (+90) < ClusterRole (+91) < CRD (+92) < ... < Namespace (+100) + assert.Less(t, getDelGrp("Service"), getDelGrp("Role")) + assert.Less(t, getDelGrp("Role"), getDelGrp("ClusterRole")) + assert.Less(t, getDelGrp("ClusterRole"), getDelGrp("CustomResourceDefinition")) + assert.Less(t, getDelGrp("CustomResourceDefinition"), getDelGrp("PersistentVolumeClaim")) + assert.Less(t, getDelGrp("PersistentVolumeClaim"), getDelGrp("PersistentVolume")) + assert.Less(t, getDelGrp("PersistentVolume"), getDelGrp("StorageClass")) + assert.Less(t, getDelGrp("StorageClass"), getDelGrp("ConfigMap")) + assert.Less(t, getDelGrp("ConfigMap"), getDelGrp("ServiceAccount")) + assert.Less(t, getDelGrp("ServiceAccount"), getDelGrp("Namespace")) +} + +func TestApplyManagedOrdering_NoEffectOnDefaultUnmanagedResource(t *testing.T) { + // A Deployment with no explicit annotations should be unaffected + res := &Resource{} + res.GVK.Kind = "Deployment" + + applyManagedOrdering(res) + + assert.Equal(t, 0, res.readinessGroup) + assert.Nil(t, res.deletionGroup) +} + +func TestManagedKindGroupsAreDeterministic(t *testing.T) { + // Same kind should always get the same group across two calls + for kind := range managedCreateOrder { + res1 := &Resource{} + res1.GVK.Kind = kind + applyManagedOrdering(res1) + + res2 := &Resource{} + res2.GVK.Kind = kind + applyManagedOrdering(res2) + + assert.Equal(t, res1.readinessGroup, res2.readinessGroup, "kind %q", kind) + require.NotNil(t, res1.deletionGroup) + require.NotNil(t, res2.deletionGroup) + assert.Equal(t, *res1.deletionGroup, *res2.deletionGroup, "kind %q", kind) + } +} diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 2c664ba2..e158683a 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -241,6 +241,18 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict res.ReadinessChecks = append(res.ReadinessChecks, check) } sort.Slice(res.ReadinessChecks, func(i, j int) bool { return res.ReadinessChecks[i].Name < res.ReadinessChecks[j].Name }) + if createGrp, ok := managedCreateOrder[res.GVK.Kind]; ok { + if res.readinessGroup != 0 && res.readinessGroup != createGrp { + logger.Info("overriding user-specified readiness-group for managed kind", + "userDefineReadinessGroup", res.readinessGroup, "managedReadinessGroup", createGrp) + } + if res.deletionGroup != nil && *res.deletionGroup != -createGrp { + logger.Info("overriding user-specified deletion-group for managed kind", + "userDefineDeletionGroup", res.deletionGroup, "managedDeletionGroup", -createGrp) + } + } + applyManagedOrdering(res) + logger.Info("resource created successfully") return res, nil } diff --git a/internal/resource/resource_test.go b/internal/resource/resource_test.go index e559b06d..35f7fe5a 100644 --- a/internal/resource/resource_test.go +++ b/internal/resource/resource_test.go @@ -30,8 +30,8 @@ var newResourceTests = []struct { { Name: "configmap", Manifest: `{ - "apiVersion": "v1", - "kind": "ConfigMap", + "apiVersion": "example.io/v1", + "kind": "Widget", "metadata": { "name": "foo", "annotations": { @@ -51,14 +51,14 @@ var newResourceTests = []struct { } }`, Assert: func(t *testing.T, r *Snapshot) { - assert.Equal(t, schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, r.GVK) + assert.Equal(t, schema.GroupVersionKind{Group: "example.io", Version: "v1", Kind: "Widget"}, r.GVK) assert.Len(t, r.ReadinessChecks, 2) assert.Equal(t, time.Second*10, r.ReconcileInterval.Duration) assert.Equal(t, Ref{ Name: "foo", Namespace: "", - Group: "", - Kind: "ConfigMap", + Group: "example.io", + Kind: "Widget", }, r.Ref) assert.True(t, r.Disable) assert.True(t, r.DisableUpdates) @@ -110,8 +110,8 @@ var newResourceTests = []struct { { Name: "zero-readiness-group", Manifest: `{ - "apiVersion": "v1", - "kind": "ConfigMap", + "apiVersion": "example.io/v1", + "kind": "Widget", "metadata": { "name": "foo", "annotations": { @@ -213,8 +213,8 @@ var newResourceTests = []struct { { Name: "negative-readiness-group", Manifest: `{ - "apiVersion": "v1", - "kind": "ConfigMap", + "apiVersion": "example.io/v1", + "kind": "Widget", "metadata": { "name": "foo", "annotations": { From 78816c29c90dc4306895d035352a9fe3c8fceea5 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Thu, 2 Apr 2026 11:52:50 -0700 Subject: [PATCH 2/6] Fixing the unit tests --- .../reconciliation/edgecase_test.go | 8 +++ .../reconciliation/ordering_test.go | 3 + internal/resource/cache_test.go | 6 +- internal/resource/kind_ordering.go | 17 +++-- internal/resource/kind_ordering_test.go | 70 ++++++++++++------- internal/resource/resource.go | 25 ++++--- 6 files changed, 82 insertions(+), 47 deletions(-) diff --git a/internal/controllers/reconciliation/edgecase_test.go b/internal/controllers/reconciliation/edgecase_test.go index b3c28934..5ab40d23 100644 --- a/internal/controllers/reconciliation/edgecase_test.go +++ b/internal/controllers/reconciliation/edgecase_test.go @@ -161,6 +161,10 @@ func TestLargeNamespaceDeletion(t *testing.T) { "kind": "Namespace", "metadata": map[string]any{ "name": "test", + "annotations": map[string]any{ + "eno.azure.io/readiness-group": "0", + "eno.azure.io/deletion-group": "0", + }, }, }, } @@ -176,6 +180,10 @@ func TestLargeNamespaceDeletion(t *testing.T) { "metadata": map[string]any{ "name": fmt.Sprintf("test-%d", i), "namespace": ns.GetName(), + "annotations": map[string]any{ + "eno.azure.io/readiness-group": "0", + "eno.azure.io/deletion-group": "0", + }, }, }, } diff --git a/internal/controllers/reconciliation/ordering_test.go b/internal/controllers/reconciliation/ordering_test.go index 4ccacec5..ae63805f 100644 --- a/internal/controllers/reconciliation/ordering_test.go +++ b/internal/controllers/reconciliation/ordering_test.go @@ -58,6 +58,9 @@ func TestReadinessGroups(t *testing.T) { "metadata": map[string]any{ "name": "test-obj-1", "namespace": "default", + "annotations": map[string]string{ + "eno.azure.io/readiness-group": "0", + }, }, "data": map[string]any{"image": s.Spec.Image}, }, diff --git a/internal/resource/cache_test.go b/internal/resource/cache_test.go index 7526ffd8..ca8eda25 100644 --- a/internal/resource/cache_test.go +++ b/internal/resource/cache_test.go @@ -212,8 +212,8 @@ func TestCacheResourceFilter(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, Spec: apiv1.ResourceSliceSpec{ Resources: []apiv1.Manifest{ - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "allowed", "namespace": "default", "labels": {"env": "prod"}}}`}, - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "filtered", "namespace": "default", "labels": {"env": "dev"}}}`}, + {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "allowed", "namespace": "default", "labels": {"env": "prod"}, "annotations": {"eno.azure.io/readiness-group": "0"}}}`}, + {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "filtered", "namespace": "default", "labels": {"env": "dev"}, "annotations": {"eno.azure.io/readiness-group": "0"}}}`}, {Manifest: `{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "pod-prod", "namespace": "default", "labels": {"env": "prod"}}}`}, }, }, @@ -385,7 +385,7 @@ func TestCacheResourceFilterAlwaysTrue(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, Spec: apiv1.ResourceSliceSpec{ Resources: []apiv1.Manifest{ - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "resource-1", "namespace": "default"}}`}, + {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "resource-1", "namespace": "default", "annotations": {"eno.azure.io/readiness-group": "0"}}}`}, {Manifest: `{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "resource-2", "namespace": "default"}}`}, }, }, diff --git a/internal/resource/kind_ordering.go b/internal/resource/kind_ordering.go index c8f66a40..15e95aa9 100644 --- a/internal/resource/kind_ordering.go +++ b/internal/resource/kind_ordering.go @@ -35,13 +35,12 @@ var managedCreateOrder = map[string]int{ "Service": -89, } -// applyManagedOrdering overrides the readiness and deletion group for managed infrastructure kinds -func applyManagedOrdering(res *Resource) { - createGrp, ok := managedCreateOrder[res.GVK.Kind] - if !ok { - return - } - res.readinessGroup = createGrp - delGroup := -createGrp - res.deletionGroup = &delGroup +// applyDefaultCreateOrdering overrides the readiness group for managed infrastructure kinds +func (r *Resource) applyDefaultReadinessGroupOrdering(readinessGroup int) { + r.readinessGroup = readinessGroup +} + +// applyDefaultDeletionGroupOrdering overrides the deletion group for managed infrastructure kinds +func (r *Resource) applyDefaultDeletionGroupOrdering(deletionGroup int) { + r.deletionGroup = &deletionGroup } diff --git a/internal/resource/kind_ordering_test.go b/internal/resource/kind_ordering_test.go index f8deb31d..5957d52d 100644 --- a/internal/resource/kind_ordering_test.go +++ b/internal/resource/kind_ordering_test.go @@ -30,7 +30,7 @@ func TestManagedCreateOrderGroupRange(t *testing.T) { } } -func TestApplyManagedOrdering_ManagedKind(t *testing.T) { +func TestApplyDefaultOrdering_ManagedKind(t *testing.T) { tests := []struct { kind string expectedCreate int @@ -51,9 +51,10 @@ func TestApplyManagedOrdering_ManagedKind(t *testing.T) { t.Run(tc.kind, func(t *testing.T) { res := &Resource{} res.GVK.Kind = tc.kind - res.readinessGroup = 5 // user tried to set a custom group - applyManagedOrdering(res) + createGrp := managedCreateOrder[tc.kind] + res.applyDefaultReadinessGroupOrdering(createGrp) + res.applyDefaultDeletionGroupOrdering(-createGrp) assert.Equal(t, tc.expectedCreate, res.readinessGroup) require.NotNil(t, res.deletionGroup) @@ -62,7 +63,7 @@ func TestApplyManagedOrdering_ManagedKind(t *testing.T) { } } -func TestApplyManagedOrdering_UnmanagedKind(t *testing.T) { +func TestApplyDefaultOrdering_UnmanagedKindNotInMap(t *testing.T) { unmanagedKinds := []string{ "Deployment", "StatefulSet", "DaemonSet", "Job", "CronJob", "Ingress", "IngressClass", "HorizontalPodAutoscaler", @@ -71,24 +72,19 @@ func TestApplyManagedOrdering_UnmanagedKind(t *testing.T) { } for _, kind := range unmanagedKinds { t.Run(kind, func(t *testing.T) { - res := &Resource{} - res.GVK.Kind = kind - res.readinessGroup = 3 - - applyManagedOrdering(res) - - assert.Equal(t, 3, res.readinessGroup, "group should remain user-defined") - assert.Nil(t, res.deletionGroup, "should have no auto-assigned deletion group") + _, ok := managedCreateOrder[kind] + assert.False(t, ok, "kind %q should not be in managedCreateOrder", kind) }) } } -func TestApplyManagedOrdering_DeletionIsReverseOfCreate(t *testing.T) { +func TestApplyDefaultOrdering_DeletionIsReverseOfCreate(t *testing.T) { for kind, createGrp := range managedCreateOrder { t.Run(kind, func(t *testing.T) { res := &Resource{} res.GVK.Kind = kind - applyManagedOrdering(res) + res.applyDefaultReadinessGroupOrdering(createGrp) + res.applyDefaultDeletionGroupOrdering(-createGrp) require.NotNil(t, res.deletionGroup) assert.Equal(t, -createGrp, *res.deletionGroup, @@ -97,20 +93,36 @@ func TestApplyManagedOrdering_DeletionIsReverseOfCreate(t *testing.T) { } } -func TestApplyManagedOrdering_OverridesUserAnnotations(t *testing.T) { +func TestApplyDefaultOrdering_UserAnnotationsPreserved(t *testing.T) { // User sets readiness-group=5 and deletion-group=10 on a Namespace. - // Both should be overridden. + // Since user provided annotations, the helpers should NOT be called. + // Verify that if we only call one helper, the other value is preserved. res := &Resource{} res.GVK.Kind = "Namespace" res.readinessGroup = 5 delGrp := 10 res.deletionGroup = &delGrp - applyManagedOrdering(res) + // Simulate: user set both annotations, so neither helper is called. + // The resource should keep user-specified values. + assert.Equal(t, 5, res.readinessGroup, "user-specified readiness group should be preserved") + assert.Equal(t, 10, *res.deletionGroup, "user-specified deletion group should be preserved") +} - assert.Equal(t, -100, res.readinessGroup) +func TestApplyDefaultOrdering_PartialUserAnnotation(t *testing.T) { + // User sets only readiness-group, not deletion-group. + // Only the deletion helper should be called. + res := &Resource{} + res.GVK.Kind = "Namespace" + res.readinessGroup = 5 // user-specified + + createGrp := managedCreateOrder["Namespace"] + // Only apply default deletion group (user didn't set it) + res.applyDefaultDeletionGroupOrdering(-createGrp) + + assert.Equal(t, 5, res.readinessGroup, "user-specified readiness group should be preserved") require.NotNil(t, res.deletionGroup) - assert.Equal(t, 100, *res.deletionGroup) + assert.Equal(t, 100, *res.deletionGroup, "default deletion group should be applied") } func TestManagedOrderingPrecedence(t *testing.T) { @@ -137,7 +149,8 @@ func TestManagedOrderingDeletionPrecedence(t *testing.T) { getDelGrp := func(kind string) int { res := &Resource{} res.GVK.Kind = kind - applyManagedOrdering(res) + createGrp := managedCreateOrder[kind] + res.applyDefaultDeletionGroupOrdering(-createGrp) return *res.deletionGroup } @@ -153,27 +166,30 @@ func TestManagedOrderingDeletionPrecedence(t *testing.T) { assert.Less(t, getDelGrp("ServiceAccount"), getDelGrp("Namespace")) } -func TestApplyManagedOrdering_NoEffectOnDefaultUnmanagedResource(t *testing.T) { - // A Deployment with no explicit annotations should be unaffected +func TestApplyDefaultOrdering_NoEffectOnUnmanagedResource(t *testing.T) { + // A Deployment is not in managedCreateOrder, so no helpers should be called. + // Verify the kind is not in the map and default values are unchanged. res := &Resource{} res.GVK.Kind = "Deployment" - applyManagedOrdering(res) - + _, ok := managedCreateOrder["Deployment"] + assert.False(t, ok, "Deployment should not be in managedCreateOrder") assert.Equal(t, 0, res.readinessGroup) assert.Nil(t, res.deletionGroup) } func TestManagedKindGroupsAreDeterministic(t *testing.T) { // Same kind should always get the same group across two calls - for kind := range managedCreateOrder { + for kind, createGrp := range managedCreateOrder { res1 := &Resource{} res1.GVK.Kind = kind - applyManagedOrdering(res1) + res1.applyDefaultReadinessGroupOrdering(createGrp) + res1.applyDefaultDeletionGroupOrdering(-createGrp) res2 := &Resource{} res2.GVK.Kind = kind - applyManagedOrdering(res2) + res2.applyDefaultReadinessGroupOrdering(createGrp) + res2.applyDefaultDeletionGroupOrdering(-createGrp) assert.Equal(t, res1.readinessGroup, res2.readinessGroup, "kind %q", kind) require.NotNil(t, res1.deletionGroup) diff --git a/internal/resource/resource.go b/internal/resource/resource.go index e158683a..49ac3d74 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -241,17 +241,26 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict res.ReadinessChecks = append(res.ReadinessChecks, check) } sort.Slice(res.ReadinessChecks, func(i, j int) bool { return res.ReadinessChecks[i].Name < res.ReadinessChecks[j].Name }) - if createGrp, ok := managedCreateOrder[res.GVK.Kind]; ok { - if res.readinessGroup != 0 && res.readinessGroup != createGrp { - logger.Info("overriding user-specified readiness-group for managed kind", - "userDefineReadinessGroup", res.readinessGroup, "managedReadinessGroup", createGrp) + // This indicates that this is an infrastructure Kind + if defaultGrp, ok := managedCreateOrder[res.GVK.Kind]; ok { + if _, ok := anno[readinessGroupKey]; !ok { + logger.Info("User did not specify a readiness-group for managed kind, assigning default readiness group to infrastructure kind", + "kind", res.GVK.Kind, "defaultGroup", defaultGrp) + res.applyDefaultReadinessGroupOrdering(defaultGrp) + } else { + logger.Info("User provided default readiness group. Skip setting default infrastructure kind", + "kind", res.GVK.Kind, "readinessGroup", res.readinessGroup) } - if res.deletionGroup != nil && *res.deletionGroup != -createGrp { - logger.Info("overriding user-specified deletion-group for managed kind", - "userDefineDeletionGroup", res.deletionGroup, "managedDeletionGroup", -createGrp) + + if _, ok := anno[deletionGroupKey]; !ok { + logger.Info("User did not specify a deletion group for managed kind, assigning default deletion group to infrastructure kind", + "kind", res.GVK.Kind, "defaultGroup", defaultGrp) + res.applyDefaultDeletionGroupOrdering(-defaultGrp) + } else { + logger.Info("User provided default deletion group. Skip setting default infrastructure kind", + "kind", res.GVK.Kind, "deletionGroup", res.deletionGroup) } } - applyManagedOrdering(res) logger.Info("resource created successfully") return res, nil From 1bf73057d3ecfeac2a18f9e75b6b553b57a464bc Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Fri, 3 Apr 2026 11:40:44 -0700 Subject: [PATCH 3/6] Adding e2e test --- e2e/default_ordering_test.go | 231 +++++++++++++++++++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 e2e/default_ordering_test.go diff --git a/e2e/default_ordering_test.go b/e2e/default_ordering_test.go new file mode 100644 index 00000000..9623971b --- /dev/null +++ b/e2e/default_ordering_test.go @@ -0,0 +1,231 @@ +package e2e + +import ( + "context" + "strings" + "testing" + "time" + + flow "github.com/Azure/go-workflow" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" + + apiv1 "github.com/Azure/eno/api/v1" + fw "github.com/Azure/eno/e2e/framework" +) + +// TestResourceDependencyOrdering validates that Eno correctly handles the +// dependency ordering between a Secret and a Deployment that mounts it, +// WITHOUT explicit eno.azure.io/readiness-group annotations. +// +// The synthesizer outputs both a Secret and a Deployment (which mounts +// the Secret as a volume). The test verifies: +// - Both resources are created successfully +// - The Deployment becomes available (at least one ready pod) +// - Pods have zero restarts (no CrashLoopBackOff) +// - No volume mount failure events occurred +func TestResourceDependencyOrdering(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 7*time.Minute) + defer cancel() + + cli := fw.NewClient(t) + + synthName := fw.UniqueName("dep-order-synth") + compName := fw.UniqueName("dep-order-comp") + secretName := fw.UniqueName("dep-order-secret") + deployName := fw.UniqueName("dep-order-deploy") + + // Secret with NO readiness-group annotation. + secret := &corev1.Secret{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"}, + ObjectMeta: metav1.ObjectMeta{Name: secretName, Namespace: "default"}, + StringData: map[string]string{"token": "test-value-123"}, + } + + // Deployment that mounts the Secret as a volume — NO readiness-group annotation. + replicas := int32(1) + deploy := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"}, + ObjectMeta: metav1.ObjectMeta{Name: deployName, Namespace: "default"}, + Spec: appsv1.DeploymentSpec{ + Replicas: &replicas, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": deployName}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": deployName}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "test", + Image: "docker.io/busybox:latest", + Command: []string{"sh", "-c", "cat /etc/secret-vol/token && sleep 3600"}, + VolumeMounts: []corev1.VolumeMount{{ + Name: "secret-vol", + MountPath: "/etc/secret-vol", + ReadOnly: true, + }}, + }}, + Volumes: []corev1.Volume{{ + Name: "secret-vol", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: secretName, + }, + }, + }}, + }, + }, + }, + } + + // Synthesizer outputs both Secret and Deployment. + // Eno should automatically resolve the dependency ordering. + synth := fw.NewMinimalSynthesizer(synthName, fw.WithCommand(fw.ToCommand(secret, deploy))) + comp := fw.NewComposition(compName, "default", + fw.WithSynthesizerRefs(apiv1.SynthesizerRef{Name: synthName})) + compKey := types.NamespacedName{Name: compName, Namespace: "default"} + + // --- Workflow steps --- + + createSynthesizer := fw.CreateStep(t, "createSynthesizer", cli, synth) + + createComposition := fw.CreateStep(t, "createComposition", cli, comp) + + waitCompositionReady := flow.Func("waitCompositionReady", func(ctx context.Context) error { + fw.WaitForCompositionReady(t, ctx, cli, compKey, 4*time.Minute) + t.Log("composition is Ready") + return nil + }) + + verifySecret := flow.Func("verifySecret", func(ctx context.Context) error { + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: secretName, Namespace: "default"}, + } + fw.WaitForResourceExists(t, ctx, cli, s, 30*time.Second) + assert.Equal(t, "test-value-123", string(s.Data["token"]), + "secret should contain expected data") + t.Logf("secret %s exists with correct data", secretName) + return nil + }) + + verifyDeploymentReady := flow.Func("verifyDeploymentReady", func(ctx context.Context) error { + err := wait.PollUntilContextTimeout(ctx, 3*time.Second, 3*time.Minute, true, + func(ctx context.Context) (bool, error) { + d := &appsv1.Deployment{} + if err := cli.Get(ctx, types.NamespacedName{ + Name: deployName, Namespace: "default", + }, d); err != nil { + return false, nil + } + t.Logf("deployment %s: replicas=%d available=%d ready=%d", + deployName, + d.Status.Replicas, + d.Status.AvailableReplicas, + d.Status.ReadyReplicas) + return d.Status.AvailableReplicas >= 1, nil + }) + require.NoError(t, err, + "timed out waiting for deployment %s to have available replicas", deployName) + t.Logf("deployment %s is available", deployName) + return nil + }) + + verifyZeroRestarts := flow.Func("verifyZeroRestarts", func(ctx context.Context) error { + podList := &corev1.PodList{} + require.NoError(t, cli.List(ctx, podList, + client.InNamespace("default"), + client.MatchingLabels{"app": deployName})) + require.NotEmpty(t, podList.Items, + "expected at least one pod for deployment %s", deployName) + + for _, pod := range podList.Items { + for _, cs := range pod.Status.ContainerStatuses { + assert.Equal(t, int32(0), cs.RestartCount, + "pod %s container %s should have 0 restarts", pod.Name, cs.Name) + assert.True(t, cs.Ready, + "pod %s container %s should be ready", pod.Name, cs.Name) + t.Logf("pod %s container %s: restarts=%d ready=%v", + pod.Name, cs.Name, cs.RestartCount, cs.Ready) + } + } + return nil + }) + + verifyNoMountErrors := flow.Func("verifyNoMountErrors", func(ctx context.Context) error { + podList := &corev1.PodList{} + require.NoError(t, cli.List(ctx, podList, + client.InNamespace("default"), + client.MatchingLabels{"app": deployName})) + + eventList := &corev1.EventList{} + require.NoError(t, cli.List(ctx, eventList, client.InNamespace("default"))) + + for _, pod := range podList.Items { + for _, event := range eventList.Items { + if event.InvolvedObject.Name == pod.Name && + event.InvolvedObject.Kind == "Pod" { + assert.False(t, + strings.Contains(event.Message, "MountVolume.SetUp failed"), + "pod %s should not have volume mount failures: %s", + pod.Name, event.Message) + assert.False(t, + strings.Contains(event.Reason, "FailedMount"), + "pod %s should not have FailedMount event: %s", + pod.Name, event.Message) + } + } + } + t.Log("no volume mount errors found in pod events") + return nil + }) + + deleteComposition := fw.DeleteStep(t, "deleteComposition", cli, comp) + + verifyResourcesDeleted := flow.Func("verifyResourcesDeleted", func(ctx context.Context) error { + d := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: deployName, Namespace: "default"}, + } + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: secretName, Namespace: "default"}, + } + fw.WaitForResourceDeleted(t, ctx, cli, d, 90*time.Second) + fw.WaitForResourceDeleted(t, ctx, cli, s, 90*time.Second) + t.Log("managed resources (deployment + secret) deleted") + return nil + }) + + cleanupSynthesizer := fw.CleanupStep(t, "cleanupSynthesizer", cli, synth) + + // --- Wire the workflow DAG --- + + w := new(flow.Workflow) + w.Add( + flow.Step(createComposition).DependsOn(createSynthesizer), + flow.Step(waitCompositionReady).DependsOn(createComposition), + + // Parallel verification after composition is ready + flow.Step(verifySecret).DependsOn(waitCompositionReady), + flow.Step(verifyDeploymentReady).DependsOn(waitCompositionReady), + + // Pod-level checks after deployment is verified ready + flow.Step(verifyZeroRestarts).DependsOn(verifyDeploymentReady), + flow.Step(verifyNoMountErrors).DependsOn(verifyDeploymentReady), + + // Cleanup after all verifications pass + flow.Step(deleteComposition).DependsOn( + verifySecret, verifyZeroRestarts, verifyNoMountErrors), + flow.Step(verifyResourcesDeleted).DependsOn(deleteComposition), + flow.Step(cleanupSynthesizer).DependsOn(verifyResourcesDeleted), + ) + + require.NoError(t, w.Do(ctx)) +} From 0d4c2abd688d4d8c9496ae1d3b75a4d88e3f6f6f Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Mon, 27 Apr 2026 19:18:01 +0000 Subject: [PATCH 4/6] Fixing comments --- e2e/default_ordering_test.go | 38 ++-- .../reconciliation/edgecase_test.go | 3 + internal/resource/kind_ordering.go | 26 +-- internal/resource/kind_ordering_test.go | 186 ++++++++---------- internal/resource/resource.go | 11 +- 5 files changed, 112 insertions(+), 152 deletions(-) diff --git a/e2e/default_ordering_test.go b/e2e/default_ordering_test.go index 9623971b..8df90c32 100644 --- a/e2e/default_ordering_test.go +++ b/e2e/default_ordering_test.go @@ -2,7 +2,6 @@ package e2e import ( "context" - "strings" "testing" "time" @@ -29,10 +28,10 @@ import ( // - Both resources are created successfully // - The Deployment becomes available (at least one ready pod) // - Pods have zero restarts (no CrashLoopBackOff) -// - No volume mount failure events occurred +// - No container errors (waiting/terminated with failure) func TestResourceDependencyOrdering(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), 7*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() cli := fw.NewClient(t) @@ -66,7 +65,7 @@ func TestResourceDependencyOrdering(t *testing.T) { Spec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "test", - Image: "docker.io/busybox:latest", + Image: "docker.io/busybox:1.36.1", Command: []string{"sh", "-c", "cat /etc/secret-vol/token && sleep 3600"}, VolumeMounts: []corev1.VolumeMount{{ Name: "secret-vol", @@ -160,31 +159,26 @@ func TestResourceDependencyOrdering(t *testing.T) { return nil }) - verifyNoMountErrors := flow.Func("verifyNoMountErrors", func(ctx context.Context) error { + verifyNoContainerErrors := flow.Func("verifyNoContainerErrors", func(ctx context.Context) error { podList := &corev1.PodList{} require.NoError(t, cli.List(ctx, podList, client.InNamespace("default"), client.MatchingLabels{"app": deployName})) - - eventList := &corev1.EventList{} - require.NoError(t, cli.List(ctx, eventList, client.InNamespace("default"))) + require.NotEmpty(t, podList.Items) for _, pod := range podList.Items { - for _, event := range eventList.Items { - if event.InvolvedObject.Name == pod.Name && - event.InvolvedObject.Kind == "Pod" { - assert.False(t, - strings.Contains(event.Message, "MountVolume.SetUp failed"), - "pod %s should not have volume mount failures: %s", - pod.Name, event.Message) - assert.False(t, - strings.Contains(event.Reason, "FailedMount"), - "pod %s should not have FailedMount event: %s", - pod.Name, event.Message) + for _, cs := range pod.Status.ContainerStatuses { + if cs.State.Waiting != nil { + t.Errorf("pod %s container %s is waiting: %s - %s", + pod.Name, cs.Name, cs.State.Waiting.Reason, cs.State.Waiting.Message) + } + if cs.State.Terminated != nil && cs.State.Terminated.ExitCode != 0 { + t.Errorf("pod %s container %s terminated with exit code %d: %s", + pod.Name, cs.Name, cs.State.Terminated.ExitCode, cs.State.Terminated.Reason) } } } - t.Log("no volume mount errors found in pod events") + t.Log("no container errors found") return nil }) @@ -218,11 +212,11 @@ func TestResourceDependencyOrdering(t *testing.T) { // Pod-level checks after deployment is verified ready flow.Step(verifyZeroRestarts).DependsOn(verifyDeploymentReady), - flow.Step(verifyNoMountErrors).DependsOn(verifyDeploymentReady), + flow.Step(verifyNoContainerErrors).DependsOn(verifyDeploymentReady), // Cleanup after all verifications pass flow.Step(deleteComposition).DependsOn( - verifySecret, verifyZeroRestarts, verifyNoMountErrors), + verifySecret, verifyZeroRestarts, verifyNoContainerErrors), flow.Step(verifyResourcesDeleted).DependsOn(deleteComposition), flow.Step(cleanupSynthesizer).DependsOn(verifyResourcesDeleted), ) diff --git a/internal/controllers/reconciliation/edgecase_test.go b/internal/controllers/reconciliation/edgecase_test.go index 5ab40d23..c7f11863 100644 --- a/internal/controllers/reconciliation/edgecase_test.go +++ b/internal/controllers/reconciliation/edgecase_test.go @@ -173,6 +173,9 @@ func TestLargeNamespaceDeletion(t *testing.T) { output.Items = []*unstructured.Unstructured{ns} for i := 0; i < 500; i++ { + // Explicit readiness/deletion groups: Namespace/Secret/ConfigMap now + // default into the reserved [-100,-81] range, but this test wants the + // legacy "everything in group 0" behavior. cm := &unstructured.Unstructured{ Object: map[string]any{ "apiVersion": "v1", diff --git a/internal/resource/kind_ordering.go b/internal/resource/kind_ordering.go index 15e95aa9..f7094b6a 100644 --- a/internal/resource/kind_ordering.go +++ b/internal/resource/kind_ordering.go @@ -1,12 +1,14 @@ package resource -// managedCreatedOrder maps infrastructure Kinds to reserved readiness groups -// Resources matching these kinds will have their readiness and deletion groups -// overridden to enforce a safe reconciliation order - -// Reserved Range -100 - -81. User groups should be >=-80 -// Deletion groups are the negation of the create groups -// Order is derived from Helm's InstallOrder/UninstallOrder +// managedCreateOrder maps Kind names (group-insensitive) to reserved +// readiness groups. We intentionally key on Kind alone: any resource +// whose Kind matches one of these names is treated as infrastructure +// and reconciled first, regardless of its API group. +// +// User-supplied readiness/deletion groups must be >= -80. +// Values in [-100, -81] are reserved for Eno-managed infrastructure defaults. +// Deletion groups are the negation of the create groups. +// Order derived from Helm's InstallOrder/UninstallOrder: // https://github.com/helm/helm/blob/main/pkg/release/v1/util/kind_sorter.go var managedCreateOrder = map[string]int{ "PriorityClass": -100, @@ -34,13 +36,3 @@ var managedCreateOrder = map[string]int{ "RoleBindingList": -90, "Service": -89, } - -// applyDefaultCreateOrdering overrides the readiness group for managed infrastructure kinds -func (r *Resource) applyDefaultReadinessGroupOrdering(readinessGroup int) { - r.readinessGroup = readinessGroup -} - -// applyDefaultDeletionGroupOrdering overrides the deletion group for managed infrastructure kinds -func (r *Resource) applyDefaultDeletionGroupOrdering(deletionGroup int) { - r.deletionGroup = &deletionGroup -} diff --git a/internal/resource/kind_ordering_test.go b/internal/resource/kind_ordering_test.go index 5957d52d..2fc0bb04 100644 --- a/internal/resource/kind_ordering_test.go +++ b/internal/resource/kind_ordering_test.go @@ -1,10 +1,12 @@ package resource import ( + "context" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestManagedCreateOrderCoversExpectedKinds(t *testing.T) { @@ -30,39 +32,6 @@ func TestManagedCreateOrderGroupRange(t *testing.T) { } } -func TestApplyDefaultOrdering_ManagedKind(t *testing.T) { - tests := []struct { - kind string - expectedCreate int - expectedDeletion int - }{ - {"Namespace", -100, 100}, - {"PriorityClass", -100, 100}, - {"ServiceAccount", -97, 97}, - {"Secret", -96, 96}, - {"ConfigMap", -96, 96}, - {"CustomResourceDefinition", -92, 92}, - {"ClusterRole", -91, 91}, - {"Role", -90, 90}, - {"Service", -89, 89}, - } - - for _, tc := range tests { - t.Run(tc.kind, func(t *testing.T) { - res := &Resource{} - res.GVK.Kind = tc.kind - - createGrp := managedCreateOrder[tc.kind] - res.applyDefaultReadinessGroupOrdering(createGrp) - res.applyDefaultDeletionGroupOrdering(-createGrp) - - assert.Equal(t, tc.expectedCreate, res.readinessGroup) - require.NotNil(t, res.deletionGroup) - assert.Equal(t, tc.expectedDeletion, *res.deletionGroup) - }) - } -} - func TestApplyDefaultOrdering_UnmanagedKindNotInMap(t *testing.T) { unmanagedKinds := []string{ "Deployment", "StatefulSet", "DaemonSet", "Job", "CronJob", @@ -78,52 +47,70 @@ func TestApplyDefaultOrdering_UnmanagedKindNotInMap(t *testing.T) { } } -func TestApplyDefaultOrdering_DeletionIsReverseOfCreate(t *testing.T) { - for kind, createGrp := range managedCreateOrder { - t.Run(kind, func(t *testing.T) { - res := &Resource{} - res.GVK.Kind = kind - res.applyDefaultReadinessGroupOrdering(createGrp) - res.applyDefaultDeletionGroupOrdering(-createGrp) - - require.NotNil(t, res.deletionGroup) - assert.Equal(t, -createGrp, *res.deletionGroup, - "deletion group should be negation of create group") +func TestNewResource_DefaultOrderingForManagedKind(t *testing.T) { + cases := []struct { + name string + manifest string + wantReadiness int + wantDeletion *int + }{ + { + name: "managed kind without annotations gets defaults", + manifest: `{"apiVersion":"v1","kind":"Secret", + "metadata":{"name":"s","namespace":"default"}}`, + wantReadiness: -96, + wantDeletion: intPtr(96), + }, + { + name: "user readiness annotation wins; deletion still defaulted", + manifest: `{"apiVersion":"v1","kind":"Secret", + "metadata":{"name":"s","namespace":"default", + "annotations":{"eno.azure.io/readiness-group":"5"}}}`, + wantReadiness: 5, + wantDeletion: intPtr(96), + }, + { + name: "user deletion annotation wins; readiness still defaulted", + manifest: `{"apiVersion":"v1","kind":"Secret", + "metadata":{"name":"s","namespace":"default", + "annotations":{"eno.azure.io/deletion-group":"10"}}}`, + wantReadiness: -96, + wantDeletion: intPtr(10), + }, + { + name: "user sets both annotations; both win", + manifest: `{"apiVersion":"v1","kind":"Secret", + "metadata":{"name":"s","namespace":"default", + "annotations":{"eno.azure.io/readiness-group":"5","eno.azure.io/deletion-group":"10"}}}`, + wantReadiness: 5, + wantDeletion: intPtr(10), + }, + { + name: "unmanaged kind untouched", + manifest: `{"apiVersion":"apps/v1","kind":"Deployment", + "metadata":{"name":"d","namespace":"default"}}`, + wantReadiness: 0, + wantDeletion: nil, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + u := &unstructured.Unstructured{} + require.NoError(t, u.UnmarshalJSON([]byte(tc.manifest))) + r, err := newResource(context.Background(), u, false) + require.NoError(t, err) + assert.Equal(t, tc.wantReadiness, r.readinessGroup) + if tc.wantDeletion == nil { + assert.Nil(t, r.deletionGroup) + } else { + require.NotNil(t, r.deletionGroup) + assert.Equal(t, *tc.wantDeletion, *r.deletionGroup) + } }) } } -func TestApplyDefaultOrdering_UserAnnotationsPreserved(t *testing.T) { - // User sets readiness-group=5 and deletion-group=10 on a Namespace. - // Since user provided annotations, the helpers should NOT be called. - // Verify that if we only call one helper, the other value is preserved. - res := &Resource{} - res.GVK.Kind = "Namespace" - res.readinessGroup = 5 - delGrp := 10 - res.deletionGroup = &delGrp - - // Simulate: user set both annotations, so neither helper is called. - // The resource should keep user-specified values. - assert.Equal(t, 5, res.readinessGroup, "user-specified readiness group should be preserved") - assert.Equal(t, 10, *res.deletionGroup, "user-specified deletion group should be preserved") -} - -func TestApplyDefaultOrdering_PartialUserAnnotation(t *testing.T) { - // User sets only readiness-group, not deletion-group. - // Only the deletion helper should be called. - res := &Resource{} - res.GVK.Kind = "Namespace" - res.readinessGroup = 5 // user-specified - - createGrp := managedCreateOrder["Namespace"] - // Only apply default deletion group (user didn't set it) - res.applyDefaultDeletionGroupOrdering(-createGrp) - - assert.Equal(t, 5, res.readinessGroup, "user-specified readiness group should be preserved") - require.NotNil(t, res.deletionGroup) - assert.Equal(t, 100, *res.deletionGroup, "default deletion group should be applied") -} +func intPtr(i int) *int { return &i } func TestManagedOrderingPrecedence(t *testing.T) { // Namespace/PriorityClass (-100) before everything @@ -145,25 +132,19 @@ func TestManagedOrderingPrecedence(t *testing.T) { } func TestManagedOrderingDeletionPrecedence(t *testing.T) { - // Deletion order is reversed: Service deleted first, Namespace last - getDelGrp := func(kind string) int { - res := &Resource{} - res.GVK.Kind = kind - createGrp := managedCreateOrder[kind] - res.applyDefaultDeletionGroupOrdering(-createGrp) - return *res.deletionGroup - } + // Deletion group is negation of create group, so deletion precedence is reversed. + delGrp := func(kind string) int { return -managedCreateOrder[kind] } // Service (+89) < Role (+90) < ClusterRole (+91) < CRD (+92) < ... < Namespace (+100) - assert.Less(t, getDelGrp("Service"), getDelGrp("Role")) - assert.Less(t, getDelGrp("Role"), getDelGrp("ClusterRole")) - assert.Less(t, getDelGrp("ClusterRole"), getDelGrp("CustomResourceDefinition")) - assert.Less(t, getDelGrp("CustomResourceDefinition"), getDelGrp("PersistentVolumeClaim")) - assert.Less(t, getDelGrp("PersistentVolumeClaim"), getDelGrp("PersistentVolume")) - assert.Less(t, getDelGrp("PersistentVolume"), getDelGrp("StorageClass")) - assert.Less(t, getDelGrp("StorageClass"), getDelGrp("ConfigMap")) - assert.Less(t, getDelGrp("ConfigMap"), getDelGrp("ServiceAccount")) - assert.Less(t, getDelGrp("ServiceAccount"), getDelGrp("Namespace")) + assert.Less(t, delGrp("Service"), delGrp("Role")) + assert.Less(t, delGrp("Role"), delGrp("ClusterRole")) + assert.Less(t, delGrp("ClusterRole"), delGrp("CustomResourceDefinition")) + assert.Less(t, delGrp("CustomResourceDefinition"), delGrp("PersistentVolumeClaim")) + assert.Less(t, delGrp("PersistentVolumeClaim"), delGrp("PersistentVolume")) + assert.Less(t, delGrp("PersistentVolume"), delGrp("StorageClass")) + assert.Less(t, delGrp("StorageClass"), delGrp("ConfigMap")) + assert.Less(t, delGrp("ConfigMap"), delGrp("ServiceAccount")) + assert.Less(t, delGrp("ServiceAccount"), delGrp("Namespace")) } func TestApplyDefaultOrdering_NoEffectOnUnmanagedResource(t *testing.T) { @@ -179,21 +160,10 @@ func TestApplyDefaultOrdering_NoEffectOnUnmanagedResource(t *testing.T) { } func TestManagedKindGroupsAreDeterministic(t *testing.T) { - // Same kind should always get the same group across two calls - for kind, createGrp := range managedCreateOrder { - res1 := &Resource{} - res1.GVK.Kind = kind - res1.applyDefaultReadinessGroupOrdering(createGrp) - res1.applyDefaultDeletionGroupOrdering(-createGrp) - - res2 := &Resource{} - res2.GVK.Kind = kind - res2.applyDefaultReadinessGroupOrdering(createGrp) - res2.applyDefaultDeletionGroupOrdering(-createGrp) - - assert.Equal(t, res1.readinessGroup, res2.readinessGroup, "kind %q", kind) - require.NotNil(t, res1.deletionGroup) - require.NotNil(t, res2.deletionGroup) - assert.Equal(t, *res1.deletionGroup, *res2.deletionGroup, "kind %q", kind) + // Same kind should always map to the same group value. + for kind, grp := range managedCreateOrder { + grp2, ok := managedCreateOrder[kind] + assert.True(t, ok, "kind %q missing on second lookup", kind) + assert.Equal(t, grp, grp2, "kind %q", kind) } } diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 49ac3d74..88ae9a0e 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -246,19 +246,20 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict if _, ok := anno[readinessGroupKey]; !ok { logger.Info("User did not specify a readiness-group for managed kind, assigning default readiness group to infrastructure kind", "kind", res.GVK.Kind, "defaultGroup", defaultGrp) - res.applyDefaultReadinessGroupOrdering(defaultGrp) + res.readinessGroup = defaultGrp } else { - logger.Info("User provided default readiness group. Skip setting default infrastructure kind", + logger.Info("user-specified readiness group present, skipping default for managed kind", "kind", res.GVK.Kind, "readinessGroup", res.readinessGroup) } if _, ok := anno[deletionGroupKey]; !ok { logger.Info("User did not specify a deletion group for managed kind, assigning default deletion group to infrastructure kind", "kind", res.GVK.Kind, "defaultGroup", defaultGrp) - res.applyDefaultDeletionGroupOrdering(-defaultGrp) + delGroup := -defaultGrp + res.deletionGroup = &delGroup } else { - logger.Info("User provided default deletion group. Skip setting default infrastructure kind", - "kind", res.GVK.Kind, "deletionGroup", res.deletionGroup) + logger.Info("user-specified deletion group present, skipping default for managed kind", + "kind", res.GVK.Kind, "deletionGroup", *res.deletionGroup) } } From 1993ecad99bb85ed03d494fd7b71775656eff206 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Mon, 27 Apr 2026 19:41:57 +0000 Subject: [PATCH 5/6] Resolving resulting comments --- .../reconciliation/ordering_test.go | 4 +++- internal/resource/cache_test.go | 4 ++++ internal/resource/kind_ordering_test.go | 19 ------------------- internal/resource/resource.go | 12 ++++++++++-- 4 files changed, 17 insertions(+), 22 deletions(-) diff --git a/internal/controllers/reconciliation/ordering_test.go b/internal/controllers/reconciliation/ordering_test.go index e26caf1a..9fb82b48 100644 --- a/internal/controllers/reconciliation/ordering_test.go +++ b/internal/controllers/reconciliation/ordering_test.go @@ -58,7 +58,9 @@ func TestReadinessGroups(t *testing.T) { "metadata": map[string]any{ "name": "test-obj-1", "namespace": "default", - "annotations": map[string]string{ + // Explicit readiness-group: ConfigMap now defaults into the reserved + // [-100,-81] range; this test wants the legacy group-0 behavior. + "annotations": map[string]string{ "eno.azure.io/readiness-group": "0", }, }, diff --git a/internal/resource/cache_test.go b/internal/resource/cache_test.go index ca8eda25..d4409e4a 100644 --- a/internal/resource/cache_test.go +++ b/internal/resource/cache_test.go @@ -212,6 +212,8 @@ func TestCacheResourceFilter(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, Spec: apiv1.ResourceSliceSpec{ Resources: []apiv1.Manifest{ + // Explicit readiness-group: ConfigMap now defaults into the reserved + // [-100,-81] range; this test wants group-0 behavior. {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "allowed", "namespace": "default", "labels": {"env": "prod"}, "annotations": {"eno.azure.io/readiness-group": "0"}}}`}, {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "filtered", "namespace": "default", "labels": {"env": "dev"}, "annotations": {"eno.azure.io/readiness-group": "0"}}}`}, {Manifest: `{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "pod-prod", "namespace": "default", "labels": {"env": "prod"}}}`}, @@ -385,6 +387,8 @@ func TestCacheResourceFilterAlwaysTrue(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, Spec: apiv1.ResourceSliceSpec{ Resources: []apiv1.Manifest{ + // Explicit readiness-group: ConfigMap now defaults into the reserved + // [-100,-81] range; this test wants group-0 behavior. {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "resource-1", "namespace": "default", "annotations": {"eno.azure.io/readiness-group": "0"}}}`}, {Manifest: `{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "resource-2", "namespace": "default"}}`}, }, diff --git a/internal/resource/kind_ordering_test.go b/internal/resource/kind_ordering_test.go index 2fc0bb04..eb9c4216 100644 --- a/internal/resource/kind_ordering_test.go +++ b/internal/resource/kind_ordering_test.go @@ -147,23 +147,4 @@ func TestManagedOrderingDeletionPrecedence(t *testing.T) { assert.Less(t, delGrp("ServiceAccount"), delGrp("Namespace")) } -func TestApplyDefaultOrdering_NoEffectOnUnmanagedResource(t *testing.T) { - // A Deployment is not in managedCreateOrder, so no helpers should be called. - // Verify the kind is not in the map and default values are unchanged. - res := &Resource{} - res.GVK.Kind = "Deployment" - _, ok := managedCreateOrder["Deployment"] - assert.False(t, ok, "Deployment should not be in managedCreateOrder") - assert.Equal(t, 0, res.readinessGroup) - assert.Nil(t, res.deletionGroup) -} - -func TestManagedKindGroupsAreDeterministic(t *testing.T) { - // Same kind should always map to the same group value. - for kind, grp := range managedCreateOrder { - grp2, ok := managedCreateOrder[kind] - assert.True(t, ok, "kind %q missing on second lookup", kind) - assert.Equal(t, grp, grp2, "kind %q", kind) - } -} diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 88ae9a0e..391b411b 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -198,6 +198,10 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict if err != nil { logger.Info("invalid readiness group - ignoring") } else { + if rg >= -100 && rg <= -81 { + logger.Info("WARNING: user-supplied readiness-group is in Eno reserved range [-100, -81]", + "kind", res.GVK.Kind, "readinessGroup", rg) + } res.readinessGroup = rg } } @@ -213,6 +217,10 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict if err != nil { logger.Info("invalid deletion group - ignoring") } else { + if rg >= 81 && rg <= 100 { + logger.Info("WARNING: user-supplied deletion-group is in Eno reserved range [81, 100]", + "kind", res.GVK.Kind, "deletionGroup", rg) + } res.deletionGroup = &rg } } @@ -244,7 +252,7 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict // This indicates that this is an infrastructure Kind if defaultGrp, ok := managedCreateOrder[res.GVK.Kind]; ok { if _, ok := anno[readinessGroupKey]; !ok { - logger.Info("User did not specify a readiness-group for managed kind, assigning default readiness group to infrastructure kind", + logger.Info("assigning default readiness group to managed kind", "kind", res.GVK.Kind, "defaultGroup", defaultGrp) res.readinessGroup = defaultGrp } else { @@ -253,7 +261,7 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict } if _, ok := anno[deletionGroupKey]; !ok { - logger.Info("User did not specify a deletion group for managed kind, assigning default deletion group to infrastructure kind", + logger.Info("assigning default deletion group to managed kind", "kind", res.GVK.Kind, "defaultGroup", defaultGrp) delGroup := -defaultGrp res.deletionGroup = &delGroup From 2aca822dd4c50da11a55fdc8690a1b2a2e684c91 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Tue, 28 Apr 2026 19:03:35 +0000 Subject: [PATCH 6/6] Resolving comments --- internal/resource/kind_ordering.go | 4 ++-- internal/resource/resource.go | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/resource/kind_ordering.go b/internal/resource/kind_ordering.go index f7094b6a..406923fa 100644 --- a/internal/resource/kind_ordering.go +++ b/internal/resource/kind_ordering.go @@ -5,8 +5,8 @@ package resource // whose Kind matches one of these names is treated as infrastructure // and reconciled first, regardless of its API group. // -// User-supplied readiness/deletion groups must be >= -80. -// Values in [-100, -81] are reserved for Eno-managed infrastructure defaults. +// User-supplied readiness/deletion groups must be >= -60. +// Values in [-100, -60] are reserved for Eno-managed infrastructure defaults. // Deletion groups are the negation of the create groups. // Order derived from Helm's InstallOrder/UninstallOrder: // https://github.com/helm/helm/blob/main/pkg/release/v1/util/kind_sorter.go diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 391b411b..8c248525 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -24,6 +24,11 @@ import ( "k8s.io/apimachinery/pkg/types" ) +const ( + ReservedReadinessGroupLowerBound = -100 + ReservedReadinessGroupUpperBound = -60 +) + var patchGVK = schema.GroupVersionKind{ Group: "eno.azure.io", Version: "v1", @@ -198,8 +203,9 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict if err != nil { logger.Info("invalid readiness group - ignoring") } else { - if rg >= -100 && rg <= -81 { - logger.Info("WARNING: user-supplied readiness-group is in Eno reserved range [-100, -81]", + if rg >= ReservedReadinessGroupLowerBound && rg <= ReservedReadinessGroupUpperBound { + logger.Info(fmt.Sprintf("WARNING: user-supplied readiness-group is in Eno reserved range [%d, %d]", + ReservedReadinessGroupLowerBound, ReservedReadinessGroupUpperBound), "kind", res.GVK.Kind, "readinessGroup", rg) } res.readinessGroup = rg @@ -217,8 +223,8 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict if err != nil { logger.Info("invalid deletion group - ignoring") } else { - if rg >= 81 && rg <= 100 { - logger.Info("WARNING: user-supplied deletion-group is in Eno reserved range [81, 100]", + if rg >= -ReservedReadinessGroupUpperBound && rg <= -ReservedReadinessGroupLowerBound { + logger.Info(fmt.Sprintf("WARNING: user-supplied deletion-group is in Eno reserved range [%d, %d]", -ReservedReadinessGroupUpperBound, -ReservedReadinessGroupLowerBound), "kind", res.GVK.Kind, "deletionGroup", rg) } res.deletionGroup = &rg