From ad3ef39be5ec5809266cfe89b2eb327be5425743 Mon Sep 17 00:00:00 2001 From: sophieliu15 Date: Wed, 1 Apr 2020 12:16:36 -0400 Subject: [PATCH] Compute NumSourceObjects in HNCConfiguration status This PR computes NumSourceObjects as a part of HNCConfiguration status. NumSourceObjects indicates the number of source objects of a specific type created by users. Tested: unit tests, GKE cluster Issue: #411 --- incubator/hnc/api/v1alpha1/hnc_config.go | 5 ++ .../hnc/api/v1alpha1/zz_generated.deepcopy.go | 5 ++ .../bases/hnc.x-k8s.io_hncconfigurations.yaml | 5 ++ incubator/hnc/pkg/forest/forest.go | 20 +++-- incubator/hnc/pkg/reconcilers/hnc_config.go | 21 ++++-- .../hnc/pkg/reconcilers/hnc_config_test.go | 74 ++++++++++++++++++- incubator/hnc/pkg/reconcilers/object.go | 12 ++- 7 files changed, 126 insertions(+), 16 deletions(-) diff --git a/incubator/hnc/api/v1alpha1/hnc_config.go b/incubator/hnc/api/v1alpha1/hnc_config.go index 4d11d680c..ffe45aa23 100644 --- a/incubator/hnc/api/v1alpha1/hnc_config.go +++ b/incubator/hnc/api/v1alpha1/hnc_config.go @@ -78,6 +78,11 @@ type TypeSynchronizationStatus struct { // +kubebuilder:validation:Minimum=0 // +optional NumPropagatedObjects *int `json:"numPropagatedObjects,omitempty"` + + // Tracks the number of objects that are created by users. + // +kubebuilder:validation:Minimum=0 + // +optional + NumSourceObjects *int `json:"numSourceObjects,omitempty"` } // +kubebuilder:object:root=true diff --git a/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go b/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go index a50745a47..7b240e69b 100644 --- a/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go +++ b/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go @@ -376,6 +376,11 @@ func (in *TypeSynchronizationStatus) DeepCopyInto(out *TypeSynchronizationStatus *out = new(int) **out = **in } + if in.NumSourceObjects != nil { + in, out := &in.NumSourceObjects, &out.NumSourceObjects + *out = new(int) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TypeSynchronizationStatus. diff --git a/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml b/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml index f973398d4..298c186ec 100644 --- a/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml +++ b/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml @@ -120,6 +120,11 @@ spec: by HNC. minimum: 0 type: integer + numSourceObjects: + description: Tracks the number of objects that are created by + users. + minimum: 0 + type: integer type: object type: array type: object diff --git a/incubator/hnc/pkg/forest/forest.go b/incubator/hnc/pkg/forest/forest.go index dce54acbe..0792232e8 100644 --- a/incubator/hnc/pkg/forest/forest.go +++ b/incubator/hnc/pkg/forest/forest.go @@ -38,10 +38,10 @@ type TypeSyncer interface { GetNumPropagatedObjects() int } -// NumPropagatedObjectsSyncer syncs the number of propagated objects. ConfigReconciler implements the -// interface so that it can be called by an ObjectReconciler if the number of propagated objects is changed. -type NumPropagatedObjectsSyncer interface { - SyncNumPropagatedObjects(logr.Logger) +// NumObjectsSyncer syncs the number of propagated and source objects. ConfigReconciler implements the +// interface so that it can be called by an ObjectReconciler if the number of propagated or source objects is changed. +type NumObjectsSyncer interface { + SyncNumObjects(logr.Logger) } // Forest defines a forest of namespaces - that is, a set of trees. It includes methods to mutate @@ -66,7 +66,7 @@ type Forest struct { // ObjectsStatusSyncer is the ConfigReconciler that an object reconciler can call if the status of the HNCConfiguration // object needs to be updated. - ObjectsStatusSyncer NumPropagatedObjectsSyncer + ObjectsStatusSyncer NumObjectsSyncer } func NewForest() *Forest { @@ -142,6 +142,11 @@ func (f *Forest) GetNamespaceNames() []string { return keys } +// GetNamespaces returns the namespaces in the forest. +func (f *Forest) GetNamespaces() namedNamespaces { + return f.namespaces +} + type namedNamespaces map[string]*Namespace // TODO Store source objects by GK in the forest - https://github.com/kubernetes-sigs/multi-tenancy/issues/281 @@ -382,6 +387,11 @@ func (ns *Namespace) GetOriginalObjects(gvk schema.GroupVersionKind) []*unstruct return o } +// GetNumOriginalObjects returns the total number of original objects of a specific GVK in the namespace. +func (ns *Namespace) GetNumOriginalObjects(gvk schema.GroupVersionKind) int { + return len(ns.originalObjects[gvk]) +} + // GetPropagatedObjects returns all original copies in the ancestors. func (ns *Namespace) GetPropagatedObjects(gvk schema.GroupVersionKind) []*unstructured.Unstructured { o := []*unstructured.Unstructured{} diff --git a/incubator/hnc/pkg/reconcilers/hnc_config.go b/incubator/hnc/pkg/reconcilers/hnc_config.go index 4c583f562..b746f8cef 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config.go @@ -399,12 +399,22 @@ func (r *ConfigReconciler) setTypeStatuses(inst *api.HNCConfiguration) { Mode: ts.GetMode(), // may be different from the spec if it's implicit } - // Only add counts if we're not ignoring this type + // Only add NumPropagatedObjects if we're not ignoring this type if ts.GetMode() != api.Ignore { numProp := ts.GetNumPropagatedObjects() status.NumPropagatedObjects = &numProp } + // Only add NumSourceObjects if we are propagating objects of this type. + if ts.GetMode() == api.Propagate { + numSrc := 0 + nms := r.Forest.GetNamespaces() + for _, ns := range nms { + numSrc += ns.GetNumOriginalObjects(gvk) + } + status.NumSourceObjects = &numSrc + } + // Record the status statuses = append(statuses, status) } @@ -447,15 +457,14 @@ func (r *ConfigReconciler) periodicTrigger() { if r.shouldReconcile == false { continue } - r.enqueueSingleton(r.Log, "Syncing NumPropagatedObjects in the status") + r.enqueueSingleton(r.Log, "Syncing NumPropagatedObjects and/or NumSourceObjects in the status") r.shouldReconcile = false } } -// SyncNumPropagatedObjects will be called by object reconcilers to signal config -// reconciler to reconcile when an object is reconciled successfully and the status of -// the `config` object might need to be updated. -func (r *ConfigReconciler) SyncNumPropagatedObjects(log logr.Logger) { +// SyncNumObjects will be called by object reconcilers to signal config +// reconciler to reconcile when the status of the `config` object might need to be updated. +func (r *ConfigReconciler) SyncNumObjects(log logr.Logger) { log.V(1).Info("Signalling config reconciler for reconciliation.") r.shouldReconcile = true } diff --git a/incubator/hnc/pkg/reconcilers/hnc_config_test.go b/incubator/hnc/pkg/reconcilers/hnc_config_test.go index 266918f1b..a1423ccea 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config_test.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config_test.go @@ -190,7 +190,7 @@ var _ = Describe("HNCConfiguration", func() { // Change to ignore and wait for reconciler updateHNCConfigSpec(ctx, "v1", "Secret", api.Ignore) - Eventually(typeHasStatus(ctx, "v1", "Secret")).Should(Equal(api.Ignore)) + Eventually(typeStatusHasMode(ctx, "v1", "Secret")).Should(Equal(api.Ignore)) bazName := createNS(ctx, "baz") setParent(ctx, bazName, fooName) @@ -266,7 +266,7 @@ var _ = Describe("HNCConfiguration", func() { // Remove from spec and wait for the reconciler to pick it up removeHNCConfigType(ctx, "v1", "Secret") - Eventually(typeHasStatus(ctx, "v1", "Secret")).Should(Equal(testModeMisssing)) + Eventually(typeStatusHasMode(ctx, "v1", "Secret")).Should(Equal(testModeMisssing)) // Give foo another secret. makeObject(ctx, "Secret", fooName, "foo-sec-2") @@ -324,6 +324,42 @@ var _ = Describe("HNCConfiguration", func() { Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), statusUpdateTime).Should(Equal(0)) }) + + It("should set NumSourceObjects for a type in propagate mode", func() { + addToHNCConfig(ctx, "v1", "LimitRange", api.Propagate) + makeObject(ctx, "LimitRange", fooName, "foo-lr") + + // There should be 2 source objects of LimitRange: 1 added in this test case, the other added in the previous test case. + Eventually(getNumSourceObjects(ctx, "v1", "LimitRange"), statusUpdateTime).Should(Equal(2)) + }) + + // If a mode is unset, it is treated as `propagate` by default, in which case we will also compute NumSourceObjects + It("should set NumSourceObjects for a type with unset mode", func() { + addToHNCConfig(ctx, "v1", "LimitRange", "") + makeObject(ctx, "LimitRange", fooName, "foo-lr") + + // There should be 3 source objects of LimitRange: 1 added in this test case, the other 2 added in the previous two test cases. + Eventually(getNumSourceObjects(ctx, "v1", "LimitRange"), statusUpdateTime).Should(Equal(3)) + }) + + It("should decrement NumSourceObjects correctly after deleting an object of a type in propagate mode", func() { + addToHNCConfig(ctx, "v1", "LimitRange", api.Propagate) + makeObject(ctx, "LimitRange", fooName, "foo-lr") + + // There should be 4 source objects of LimitRange, 1 added in this teat case, the other 3 added by the previous three test cases. + Eventually(getNumSourceObjects(ctx, "v1", "LimitRange"), statusUpdateTime).Should(Equal(4)) + + deleteObject(ctx, "LimitRange", fooName, "foo-lr") + + // There should be 3 source objects of LimitRange because we deleted the one added in this test case. + Eventually(getNumSourceObjects(ctx, "v1", "LimitRange"), statusUpdateTime).Should(Equal(3)) + }) + + It("should not set NumSourceObjects for a type not in propagate mode", func() { + addToHNCConfig(ctx, "v1", "LimitRange", api.Remove) + + Eventually(hasNumSourceObjects(ctx, "v1", "LimitRange"), statusUpdateTime).Should(BeFalse()) + }) }) func typeSpecHasMode(ctx context.Context, apiVersion, kind string) func() api.SynchronizationMode { @@ -338,7 +374,7 @@ func typeSpecHasMode(ctx context.Context, apiVersion, kind string) func() api.Sy } } -func typeHasStatus(ctx context.Context, apiVersion, kind string) func() api.SynchronizationMode { +func typeStatusHasMode(ctx context.Context, apiVersion, kind string) func() api.SynchronizationMode { return func() api.SynchronizationMode { config := getHNCConfig(ctx) for _, t := range config.Status.Types { @@ -503,3 +539,35 @@ func getNumPropagatedObjects(ctx context.Context, apiVersion, kind string) func( return -1, errors.New(fmt.Sprintf("apiversion %s, kind %s is not found in status", apiVersion, kind)) } } + +// hasNumSourceObjects returns true if NumSourceObjects is set (not nil) for a specific type and returns false +// if NumSourceObjects is not set. It returns false and an error if the type does not exist in the status. +func hasNumSourceObjects(ctx context.Context, apiVersion, kind string) func() (bool, error) { + return func() (bool, error) { + c := getHNCConfig(ctx) + for _, t := range c.Status.Types { + if t.APIVersion == apiVersion && t.Kind == kind { + return t.NumSourceObjects != nil, nil + } + } + return false, errors.New(fmt.Sprintf("apiversion %s, kind %s is not found in status", apiVersion, kind)) + } +} + +// getNumSourceObjects returns NumSourceObjects status for a given type. If NumSourceObjects is +// not set or if type does not exist in status, it returns -1 and an error. +func getNumSourceObjects(ctx context.Context, apiVersion, kind string) func() (int, error) { + return func() (int, error) { + c := getHNCConfig(ctx) + for _, t := range c.Status.Types { + if t.APIVersion == apiVersion && t.Kind == kind { + if t.NumSourceObjects != nil { + return *t.NumSourceObjects, nil + } + return -1, errors.New(fmt.Sprintf("NumSourceObjects field is not set for "+ + "apiversion %s, kind %s", apiVersion, kind)) + } + } + return -1, errors.New(fmt.Sprintf("apiversion %s, kind %s is not found in status", apiVersion, kind)) + } +} diff --git a/incubator/hnc/pkg/reconcilers/object.go b/incubator/hnc/pkg/reconcilers/object.go index ab12de61a..da2aa83ef 100644 --- a/incubator/hnc/pkg/reconcilers/object.go +++ b/incubator/hnc/pkg/reconcilers/object.go @@ -392,6 +392,10 @@ func (r *ObjectReconciler) syncSource(ctx context.Context, log logr.Logger, src // Update or create a copy of the source object in the forest ns.SetOriginalObject(src.DeepCopy()) + // Signal the config reconciler for reconciliation because it is possible that a source object is + // added to the apiserver. + r.Forest.ObjectsStatusSyncer.SyncNumObjects(log) + // Enqueue all the descendant copies r.enqueueDescendants(ctx, log, src) } @@ -590,6 +594,10 @@ func (r *ObjectReconciler) syncUnpropagatedSource(ctx context.Context, log logr. gvk := inst.GroupVersionKind() r.Forest.Get(nnm).DeleteOriginalObject(gvk, nm) + // Signal the config reconciler for reconciliation because it is possible that the source object is + // deleted on the apiserver. + r.Forest.ObjectsStatusSyncer.SyncNumObjects(log) + r.enqueueDescendants(ctx, log, inst) } @@ -640,7 +648,7 @@ func (r *ObjectReconciler) recordPropagatedObject(log logr.Logger, namespace, na Name: name, } r.propagatedObjects[nnm] = true - r.Forest.ObjectsStatusSyncer.SyncNumPropagatedObjects(log) + r.Forest.ObjectsStatusSyncer.SyncNumObjects(log) } // recordRemovedObject records the fact that this (possibly) previously propagated object no longer @@ -654,7 +662,7 @@ func (r *ObjectReconciler) recordRemovedObject(log logr.Logger, namespace, name Name: name, } delete(r.propagatedObjects, nnm) - r.Forest.ObjectsStatusSyncer.SyncNumPropagatedObjects(log) + r.Forest.ObjectsStatusSyncer.SyncNumObjects(log) } func (r *ObjectReconciler) SetupWithManager(mgr ctrl.Manager, maxReconciles int) error {