From 090c74f3eac58af2528f728a793d49f710482567 Mon Sep 17 00:00:00 2001 From: Britania Rodriguez Reyes Date: Fri, 20 Dec 2024 16:04:34 -0600 Subject: [PATCH 1/2] add PDB trackability and unit test --- go.mod | 1 + pkg/controllers/work/apply_controller.go | 23 ++++ pkg/controllers/work/apply_controller_test.go | 101 +++++++++++++++++- pkg/utils/common.go | 7 ++ 4 files changed, 131 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 2199552f2..55baae704 100644 --- a/go.mod +++ b/go.mod @@ -30,6 +30,7 @@ require ( k8s.io/apimachinery v0.30.3 k8s.io/client-go v0.30.3 k8s.io/component-base v0.30.2 + k8s.io/component-helpers v0.28.3 k8s.io/klog/v2 v2.130.1 k8s.io/metrics v0.25.2 k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 2892ce93e..68d67ea7a 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -29,6 +29,7 @@ import ( "go.uber.org/atomic" appv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" apiextensionshelpers "k8s.io/apiextensions-apiserver/pkg/apihelpers" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -41,6 +42,7 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/dynamic" "k8s.io/client-go/tools/record" + "k8s.io/component-helpers/apps/poddisruptionbudget" "k8s.io/klog/v2" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -453,6 +455,9 @@ func trackResourceAvailability(gvr schema.GroupVersionResource, curObj *unstruct case utils.CustomResourceDefinitionGVR: return trackCRDAvailability(curObj) + case utils.PodDisruptionBudgetGVR: + return trackPDBAvailability(curObj) + default: if isDataResource(gvr) { klog.V(2).InfoS("Data resources are available immediately", "gvr", gvr, "resource", klog.KObj(curObj)) @@ -463,6 +468,24 @@ func trackResourceAvailability(gvr schema.GroupVersionResource, curObj *unstruct } } +func trackPDBAvailability(curObj *unstructured.Unstructured) (ApplyAction, error) { + var pdb policyv1.PodDisruptionBudget + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(curObj.Object, &pdb); err != nil { + return errorApplyAction, controller.NewUnexpectedBehaviorError(err) + } + // Check if conditions are up-to-date + if poddisruptionbudget.ConditionsAreUpToDate(&pdb) { + if cond := meta.FindStatusCondition(pdb.Status.Conditions, policyv1.DisruptionAllowedCondition); cond != nil { + if cond.Status == metav1.ConditionTrue && cond.Reason == policyv1.SufficientPodsReason { + klog.V(2).InfoS("PodDisruptionBudget is available", "pdb", klog.KObj(curObj)) + return manifestAvailableAction, nil + } + } + } + klog.V(2).InfoS("Still need to wait for PodDisruptionBudget to be available", "pdb", klog.KObj(curObj)) + return manifestNotAvailableYetAction, nil +} + func trackCRDAvailability(curObj *unstructured.Unstructured) (ApplyAction, error) { var crd apiextensionsv1.CustomResourceDefinition if err := runtime.DefaultUnstructuredConverter.FromUnstructured(curObj.Object, &crd); err != nil { diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index 3a50a8439..5f997a163 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -1420,8 +1420,107 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestNotAvailableYetAction, err: nil, }, + "Test PodDisruptionBudget available": { + gvr: utils.PodDisruptionBudgetGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "policy/v1", + "kind": "PodDisruptionBudget", + "metadata": map[string]interface{}{ + "name": "test-pdb", + "namespace": "default", + "generation": 2, + }, + "spec": map[string]interface{}{ + "minAvailable": 1, + }, + "status": map[string]interface{}{ + "currentHealthy": 2, + "desiredHealthy": 1, + "observedGeneration": 2, + "disruptionsAllowed": 1, + "expectedPods": 1, + "conditions": []interface{}{ + map[string]interface{}{ + "type": "DisruptionAllowed", + "status": "True", + "reason": "SufficientPods", + "observedGeneration": 2, + }, + }, + }, + }, + }, + expected: manifestAvailableAction, + err: nil, + }, + "Test PodDisruptionBudget unavailable (condition not up-to-date)": { + gvr: utils.PodDisruptionBudgetGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "policy/v1", + "kind": "PodDisruptionBudget", + "metadata": map[string]interface{}{ + "name": "test-pdb", + "namespace": "default", + "generation": 2, + }, + "spec": map[string]interface{}{ + "minAvailable": 1, + }, + "status": map[string]interface{}{ + "currentHealthy": 2, + "desiredHealthy": 1, + "observedGeneration": 1, + "disruptionsAllowed": 1, + "expectedPods": 1, + "conditions": []interface{}{ + map[string]interface{}{ + "type": "DisruptionAllowed", + "status": "True", + "reason": "SufficientPods", + }, + }, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, + "Test PodDisruptionBudget unavailable": { + gvr: utils.PodDisruptionBudgetGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "policy/v1", + "kind": "PodDisruptionBudget", + "metadata": map[string]interface{}{ + "name": "test-pdb", + "namespace": "default", + "generation": 2, + }, + "spec": map[string]interface{}{ + "minAvailable": 1, + }, + "status": map[string]interface{}{ + "currentHealthy": 1, + "desiredHealthy": 1, + "observedGeneration": 2, + "disruptionsAllowed": 1, + "expectedPods": 1, + "conditions": []interface{}{ + map[string]interface{}{ + "type": "DisruptionAllowed", + "status": "False", + "reason": "InsufficientPods", + }, + }, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, } - for name, tt := range tests { t.Run(name, func(t *testing.T) { action, err := trackResourceAvailability(tt.gvr, tt.obj) diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 85bb5ca1f..923f35703 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -17,6 +17,7 @@ import ( batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" + policyv1 "k8s.io/api/policy/v1" rbacv1 "k8s.io/api/rbac/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -263,6 +264,12 @@ var ( Kind: "Pod", } + PodDisruptionBudgetGVR = schema.GroupVersionResource{ + Group: policyv1.GroupName, + Version: policyv1.SchemeGroupVersion.Version, + Resource: "poddisruptionbudgets", + } + RoleMetaGVK = metav1.GroupVersionKind{ Group: rbacv1.SchemeGroupVersion.Group, Version: rbacv1.SchemeGroupVersion.Version, From cab09286d9f5100bddda46212e7583716c4d8432 Mon Sep 17 00:00:00 2001 From: Britania Rodriguez Reyes Date: Tue, 7 Jan 2025 11:31:33 -0800 Subject: [PATCH 2/2] update check and tests --- pkg/controllers/work/apply_controller.go | 8 ++------ pkg/controllers/work/apply_controller_test.go | 17 +++++++++-------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 68d67ea7a..641ea6ff5 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -475,12 +475,8 @@ func trackPDBAvailability(curObj *unstructured.Unstructured) (ApplyAction, error } // Check if conditions are up-to-date if poddisruptionbudget.ConditionsAreUpToDate(&pdb) { - if cond := meta.FindStatusCondition(pdb.Status.Conditions, policyv1.DisruptionAllowedCondition); cond != nil { - if cond.Status == metav1.ConditionTrue && cond.Reason == policyv1.SufficientPodsReason { - klog.V(2).InfoS("PodDisruptionBudget is available", "pdb", klog.KObj(curObj)) - return manifestAvailableAction, nil - } - } + klog.V(2).InfoS("PodDisruptionBudget is available", "pdb", klog.KObj(curObj)) + return manifestAvailableAction, nil } klog.V(2).InfoS("Still need to wait for PodDisruptionBudget to be available", "pdb", klog.KObj(curObj)) return manifestNotAvailableYetAction, nil diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index 5f997a163..399d1efeb 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -1420,7 +1420,7 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestNotAvailableYetAction, err: nil, }, - "Test PodDisruptionBudget available": { + "Test PodDisruptionBudget available (sufficient pods)": { gvr: utils.PodDisruptionBudgetGVR, obj: &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -1487,7 +1487,7 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestNotAvailableYetAction, err: nil, }, - "Test PodDisruptionBudget unavailable": { + "Test PodDisruptionBudget available (insufficient pods)": { gvr: utils.PodDisruptionBudgetGVR, obj: &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -1502,22 +1502,23 @@ func TestTrackResourceAvailability(t *testing.T) { "minAvailable": 1, }, "status": map[string]interface{}{ - "currentHealthy": 1, + "currentHealthy": 2, "desiredHealthy": 1, "observedGeneration": 2, - "disruptionsAllowed": 1, + "disruptionsAllowed": 0, "expectedPods": 1, "conditions": []interface{}{ map[string]interface{}{ - "type": "DisruptionAllowed", - "status": "False", - "reason": "InsufficientPods", + "type": "DisruptionAllowed", + "status": "False", + "reason": "InsufficientPods", + "observedGeneration": 2, }, }, }, }, }, - expected: manifestNotAvailableYetAction, + expected: manifestAvailableAction, err: nil, }, }