From ec2699578840e02443c1536db0846639bebd607b Mon Sep 17 00:00:00 2001 From: sophieliu15 Date: Tue, 24 Mar 2020 12:57:00 -0400 Subject: [PATCH] Compute NumPropagatedObjects in status This PR computes NumPropagatedObjects as a part of HNCConfiguration status. NumPropagatedObjects indicates the number of propagated objects of a specific type created by HNC. Tested: unit tests, GKE cluster Issue: #411 --- incubator/hnc/api/v1alpha1/hnc_config.go | 5 +- .../hnc/api/v1alpha1/zz_generated.deepcopy.go | 4 +- .../bases/hnc.x-k8s.io_hncconfigurations.yaml | 7 +- incubator/hnc/pkg/forest/forest.go | 20 ++++ incubator/hnc/pkg/reconcilers/hnc_config.go | 76 +++++++++--- .../hnc/pkg/reconcilers/hnc_config_test.go | 109 ++++++++++++++---- incubator/hnc/pkg/reconcilers/object.go | 95 ++++++++++++--- incubator/hnc/pkg/reconcilers/object_test.go | 8 +- incubator/hnc/pkg/reconcilers/setup.go | 2 +- 9 files changed, 262 insertions(+), 64 deletions(-) diff --git a/incubator/hnc/api/v1alpha1/hnc_config.go b/incubator/hnc/api/v1alpha1/hnc_config.go index 3b1f1d677..9c248ff9b 100644 --- a/incubator/hnc/api/v1alpha1/hnc_config.go +++ b/incubator/hnc/api/v1alpha1/hnc_config.go @@ -69,10 +69,11 @@ type TypeSynchronizationStatus struct { // Kind to be configured. Kind string `json:"kind,omitempty"` - // Tracks the number of original objects that are being propagated to descendant namespaces. + // Tracks the number of objects that are being propagated to descendant namespaces. The propagated + // objects are created by HNC. // +kubebuilder:validation:Minimum=0 // +optional - NumPropagated *int32 `json:"numPropagated,omitempty"` + NumPropagatedObjects *int32 `json:"numPropagatedObjects,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 bb4d37d81..29783d9a1 100644 --- a/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go +++ b/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go @@ -376,8 +376,8 @@ func (in *TypeSynchronizationSpec) DeepCopy() *TypeSynchronizationSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TypeSynchronizationStatus) DeepCopyInto(out *TypeSynchronizationStatus) { *out = *in - if in.NumPropagated != nil { - in, out := &in.NumPropagated, &out.NumPropagated + if in.NumPropagatedObjects != nil { + in, out := &in.NumPropagatedObjects, &out.NumPropagatedObjects *out = new(int32) **out = **in } 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 2ac140dfe..18dd88175 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 @@ -108,9 +108,10 @@ spec: kind: description: Kind to be configured. type: string - numPropagated: - description: Tracks the number of original objects that are being - propagated to descendant namespaces. + numPropagatedObjects: + description: Tracks the number of objects that are being propagated + to descendant namespaces. The propagated objects are created + by HNC. format: int32 minimum: 0 type: integer diff --git a/incubator/hnc/pkg/forest/forest.go b/incubator/hnc/pkg/forest/forest.go index 40e8ba942..27f254202 100644 --- a/incubator/hnc/pkg/forest/forest.go +++ b/incubator/hnc/pkg/forest/forest.go @@ -32,6 +32,14 @@ type TypeSyncer interface { // SetMode sets the propagation mode of objects that are handled by the reconciler who implements the interface. // The method also syncs objects in the cluster for the type handled by the reconciler if necessary. SetMode(context.Context, api.SynchronizationMode, logr.Logger) error + // GetNumPropagatedObjects returns the number of propagated objects on the apiserver. + GetNumPropagatedObjects() int32 +} + +// ConfigSyncer syncs the status of the HNCConfiguration object. ConfigReconciler implements the interface so that +// it can be called by an ObjectReconciler if the status needs to be updated. +type ConfigSyncer interface { + SyncHNCConfigStatus(logr.Logger) } // Forest defines a forest of namespaces - that is, a set of trees. It includes methods to mutate @@ -53,6 +61,10 @@ type Forest struct { // We can also move the lock out of the forest and pass it to all reconcilers that need the lock. // In that way, we don't need to put the list in the forest. types []TypeSyncer + + // config is the ConfigReconciler that an object reconciler can call if the status of the HNCConfiguration + // object needs to be updated. + config ConfigSyncer } func NewForest() *Forest { @@ -86,6 +98,14 @@ func (f *Forest) GetTypeSyncer(gvk schema.GroupVersionKind) TypeSyncer { return nil } +func (f *Forest) AddConfigSyncer(c ConfigSyncer) { + f.config = c +} + +func (f *Forest) GetConfigSyncer() ConfigSyncer { + return f.config +} + // GetTypeSyncers returns the types list. // Retuns a copy here so that the caller does not need to hold the mutex while accessing the returned value and can modify the // returned value without fear of corrupting the original types list. diff --git a/incubator/hnc/pkg/reconcilers/hnc_config.go b/incubator/hnc/pkg/reconcilers/hnc_config.go index 17a0761d6..bb6ac0dcd 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config.go @@ -3,6 +3,7 @@ package reconcilers import ( "context" "fmt" + "time" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -33,15 +34,18 @@ type ConfigReconciler struct { // Forest is the in-memory data structure that is shared with all other reconcilers. Forest *forest.Forest - // Igniter is a channel of event.GenericEvent (see "Watching Channels" in + // Trigger is a channel of event.GenericEvent (see "Watching Channels" in // https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) - // that is used to enqueue the singleton for initial reconciliation. - Igniter chan event.GenericEvent + // that is used to enqueue the singleton to trigger reconciliation. + Trigger chan event.GenericEvent // HierarchyConfigUpdates is a channel of events used to update hierarchy configuration changes performed by // ObjectReconcilers. It is passed on to ObjectReconcilers for the updates. The ConfigReconciler itself does // not use it. HierarchyConfigUpdates chan event.GenericEvent + + // activeGVKs contains GVKs that are configured in the Spec. + activeGVKs gvkSet } type gvkSet map[schema.GroupVersionKind]bool @@ -49,6 +53,12 @@ type gvkSet map[schema.GroupVersionKind]bool // Reconcile sets up some basic variable and logs the Spec. // TODO: Updates the comment above when adding more logic to the Reconcile method. func (r *ConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { + // Object reconcilers will trigger the config reconciler at the end of each object + // reconciliation for updating the status of the `config` singleton. Sleep here so + // that a batch of reconciliation requests issued by object reconcilers can be + // treated as one request to avoid invoking the config reconciler very frequently. + time.Sleep(3 * time.Second) + ctx := context.Background() // Validate the singleton name. @@ -63,18 +73,17 @@ func (r *ConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { return ctrl.Result{}, err } - // TODO: Modify this and other reconcilers (e.g., hierarchy and object reconcilers) to - // achieve the reconciliation. - r.Log.Info("Reconciling cluster-wide HNC configuration") - - // Clear the existing conditions because we will reconstruct the latest conditions. + // Clear the existing the status because we will reconstruct the latest status. inst.Status.Conditions = nil + inst.Status.Types = nil // Create or sync corresponding ObjectReconcilers, if needed. syncErr := r.syncObjectReconcilers(ctx, inst) + // Add the status for each type. + r.addTypeStatus(inst) + // Write back to the apiserver. - // TODO: Update HNCConfiguration.Status before writing the singleton back to the apiserver. if err := r.writeSingleton(ctx, inst); err != nil { r.Log.Error(err, "Couldn't write singleton") return ctrl.Result{}, err @@ -225,6 +234,7 @@ func (r *ConfigReconciler) syncObjectReconcilers(ctx context.Context, inst *api. func (r *ConfigReconciler) syncActiveReconcilers(ctx context.Context, inst *api.HNCConfiguration) error { // exist keeps track of existing types in the `config` singleton. exist := gvkSet{} + r.activeGVKs = gvkSet{} for _, t := range inst.Spec.Types { // If there are multiple configurations of the same type, we will follow the first // configuration and ignore the rest. @@ -232,6 +242,7 @@ func (r *ConfigReconciler) syncActiveReconcilers(ctx context.Context, inst *api. continue } gvk := schema.FromAPIVersionAndKind(t.APIVersion, t.Kind) + r.activeGVKs[gvk] = true if ts := r.Forest.GetTypeSyncer(gvk); ts != nil { if err := ts.SetMode(ctx, t.Mode, r.Log); err != nil { return err // retry the reconciliation @@ -314,6 +325,7 @@ func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind, m Mode: mode, Affected: make(chan event.GenericEvent), AffectedNamespace: r.HierarchyConfigUpdates, + PropagatedObjects: namespacedNameSet{}, } // TODO: figure out MaxConcurrentReconciles option - https://github.com/kubernetes-sigs/multi-tenancy/issues/291 @@ -362,21 +374,46 @@ func (r *ConfigReconciler) validateSingletonName(ctx context.Context, nm string) return fmt.Errorf("Error while validating singleton name: %s", msg) } -// forceInitialReconcile forces reconciliation to start after setting up the -// controller with the manager. This is used to create a default singleton if -// there is no singleton in the cluster. This occurs in a goroutine so the +// addTypeStatus adds Status.Types for types configured in the spec. +func (r *ConfigReconciler) addTypeStatus(inst *api.HNCConfiguration) { + for _, ts := range r.Forest.GetTypeSyncers() { + if r.activeGVKs[ts.GetGVK()] { + r.addNumPropagatedObjects(ts.GetGVK(), ts.GetNumPropagatedObjects(), inst) + } + } +} + +// addNumPropagatedObjects adds the NumPropagatedObjects field for a given GVK in the status. +func (r *ConfigReconciler) addNumPropagatedObjects(gvk schema.GroupVersionKind, num int32, + inst *api.HNCConfiguration) { + apiVersion, kind := gvk.ToAPIVersionAndKind() + inst.Status.Types = append(inst.Status.Types, api.TypeSynchronizationStatus{ + APIVersion: apiVersion, + Kind: kind, + NumPropagatedObjects: &num, + }) +} + +// enqueueSingleton enqueues the `config` singleton to trigger the reconciliation +// of the singleton for a given reason . This occurs in a goroutine so the // caller doesn't block; since the reconciler is never garbage-collected, // this is safe. -func (r *ConfigReconciler) forceInitialReconcile(log logr.Logger, reason string) { +func (r *ConfigReconciler) enqueueSingleton(log logr.Logger, reason string) { go func() { log.Info("Enqueuing for reconciliation", "reason", reason) // The watch handler doesn't care about anything except the metadata. inst := &api.HNCConfiguration{} inst.ObjectMeta.Name = api.HNCConfigSingleton - r.Igniter <- event.GenericEvent{Meta: inst} + r.Trigger <- event.GenericEvent{Meta: inst} }() } +// SyncHNCConfigStatus is called by an object reconciler to trigger reconciliation of +// the 'config' singleton for updating the status. +func (r *ConfigReconciler) SyncHNCConfigStatus(log logr.Logger) { + r.enqueueSingleton(log, "Sync NumPropagatedObjects in the status") +} + // SetupWithManager builds a controller with the reconciler. func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { // Whenever a CRD is created/updated, we will send a request to reconcile the @@ -392,14 +429,15 @@ func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { }) err := ctrl.NewControllerManagedBy(mgr). For(&api.HNCConfiguration{}). - Watches(&source.Channel{Source: r.Igniter}, &handler.EnqueueRequestForObject{}). + Watches(&source.Channel{Source: r.Trigger}, &handler.EnqueueRequestForObject{}). Watches(&source.Kind{Type: &v1beta1.CustomResourceDefinition{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: crdMapFn}). Complete(r) if err != nil { return err } - // Create a default singleton if there is no singleton in the cluster. + // Create a default singleton if there is no singleton in the cluster by forcing + // reconciliation to start. // // The cache used by the client to retrieve objects might not be populated // at this point. As a result, we cannot use r.Get() to determine the existence @@ -407,7 +445,11 @@ func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { // it does not exist. As a workaround, we decide to enforce reconciliation. The // cache is populated at the reconciliation stage. A default singleton will be // created during the reconciliation if there is no singleton in the cluster. - r.forceInitialReconcile(r.Log, "Enforce reconciliation to create a default"+ + r.enqueueSingleton(r.Log, "Enforce reconciliation to create a default"+ "HNCConfiguration singleton if it does not exist") + + // Informs the forest about the config reconciler so that it can be triggered + // by object reconcilers for updating the status. + r.Forest.AddConfigSyncer(r) return nil } diff --git a/incubator/hnc/pkg/reconcilers/hnc_config_test.go b/incubator/hnc/pkg/reconcilers/hnc_config_test.go index 02e57384c..ab1b546fb 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config_test.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config_test.go @@ -2,6 +2,7 @@ package reconcilers_test import ( "context" + "errors" "fmt" "strings" "time" @@ -12,14 +13,24 @@ import ( v1 "k8s.io/api/rbac/v1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) var _ = Describe("HNCConfiguration", func() { - // sleepTime is the time to sleep for objects propagation to take effect. + // objectPropagationTime is the time to sleep for objects propagation to take effect. // From experiment it takes ~0.015s for HNC to propagate an object. Setting // the sleep time to 1s should be long enough. // We may need to increase the sleep time in future if HNC takes longer to propagate objects. - const sleepTime = 1 * time.Second + const objectPropagationTime = 1 * time.Second + // hncConfigReconciliationTime is the time to wait for reconciliation of the config + // object happens. It is the same as the sleep time at the beginning of the Reconcile method + // of HNCConfiguration. + const hncConfigReconciliationTime = 3 * time.Second + // hncConfigStatusUpdateTime is the time to wait for updating HNCConfiguration status on + // the apiserver. From experiment, this is the minimal integer seconds needed for the + // status to be updated successfully on the apiserver. + // We may need to increase the time in future if it takes longer to update status. + const hncConfigStatusUpdateTime = 7 * time.Second ctx := context.Background() var ( @@ -28,6 +39,9 @@ var _ = Describe("HNCConfiguration", func() { ) BeforeEach(func() { + // Increase the timeout of `Eventually` for test cases in this file because the `Reconcile` + // method of ConfigReconciler sleeps at the very beginning of the method. + SetDefaultEventuallyTimeout(4 * time.Second) fooName = createNS(ctx, "foo") barName = createNS(ctx, "bar") }) @@ -37,6 +51,7 @@ var _ = Describe("HNCConfiguration", func() { Eventually(func() error { return resetHNCConfigToDefault(ctx) }).Should(Succeed()) + SetDefaultEventuallyTimeout(2 * time.Second) }) It("should set mode of Roles and RoleBindings as propagate by default", func() { @@ -96,25 +111,18 @@ var _ = Describe("HNCConfiguration", func() { return updateHNCConfig(ctx, config) }).Should(Succeed()) - Eventually(hasHNCConfigurationConditionWithName(ctx, api.CritSingletonNameInvalid, nm)).Should(BeTrue()) - }) - - It("should set ObjectReconcilerCreationFailed condition if an object reconciler creation fails", func() { - // API version of Secret should be "v1" - addToHNCConfig(ctx, "v2", "ConfigMap", api.Propagate) - - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=ConfigMap")).Should(BeTrue()) + Eventually(hasHNCConfigurationConditionWithName(ctx, api.CritSingletonNameInvalid, nm), hncConfigStatusUpdateTime).Should(BeTrue()) }) It("should unset ObjectReconcilerCreationFailed condition if an object reconciler creation later succeeds", func() { - // API version of LimitRange should be "v1" - addToHNCConfig(ctx, "v2", "LimitRange", api.Propagate) + // API version of ConfigMap should be "v1" + addToHNCConfig(ctx, "v2", "ConfigMap", api.Propagate) - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=LimitRange")).Should(BeTrue()) + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=ConfigMap"), hncConfigStatusUpdateTime).Should(BeTrue()) - updateHNCConfigSpec(ctx, "v2", "v1", "LimitRange", "LimitRange", api.Propagate, api.Propagate) + updateHNCConfigSpec(ctx, "v2", "v1", "ConfigMap", "ConfigMap", api.Propagate, api.Propagate) - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=LimitRange")).Should(BeFalse()) + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=ConfigMap"), hncConfigStatusUpdateTime).Should(BeFalse()) }) It("should set MultipleConfigurationsForOneType if there are multiple configurations for one type", func() { @@ -148,7 +156,7 @@ var _ = Describe("HNCConfiguration", func() { // Foo should have "foo-resource-quota" since we created there. Eventually(hasObject(ctx, "ResourceQuota", fooName, "foo-resource-quota")).Should(BeTrue()) // Sleep to give "foo-resource-quota" a chance to propagate from foo to bar, if it could. - time.Sleep(sleepTime) + time.Sleep(objectPropagationTime) Expect(hasObject(ctx, "ResourceQuota", barName, "foo-resource-quota")()).Should(BeFalse()) }) @@ -178,10 +186,12 @@ var _ = Describe("HNCConfiguration", func() { Expect(objectInheritedFrom(ctx, "Secret", barName, "foo-sec")).Should(Equal(fooName)) updateHNCConfigSpec(ctx, "v1", "v1", "Secret", "Secret", api.Propagate, api.Ignore) + time.Sleep(hncConfigReconciliationTime) + bazName := createNS(ctx, "baz") setParent(ctx, bazName, fooName) // Sleep to give "foo-sec" a chance to propagate from foo to baz, if it could. - time.Sleep(sleepTime) + time.Sleep(objectPropagationTime) Expect(hasObject(ctx, "Secret", bazName, "foo-sec")()).Should(BeFalse()) }) @@ -194,7 +204,7 @@ var _ = Describe("HNCConfiguration", func() { // Foo should have "foo-resource-quota" since we created there. Eventually(hasObject(ctx, "ResourceQuota", fooName, "foo-resource-quota")).Should(BeTrue()) // Sleep to give "foo-resource-quota" a chance to propagate from foo to bar, if it could. - time.Sleep(sleepTime) + time.Sleep(objectPropagationTime) Expect(hasObject(ctx, "ResourceQuota", barName, "foo-resource-quota")()).Should(BeFalse()) updateHNCConfigSpec(ctx, "v1", "v1", "ResourceQuota", "ResourceQuota", api.Ignore, api.Propagate) @@ -230,7 +240,7 @@ var _ = Describe("HNCConfiguration", func() { // Foo should have "foo-resource-quota" because it is a source object, which will not be removed. Eventually(hasObject(ctx, "ResourceQuota", fooName, "foo-resource-quota")).Should(BeTrue()) // Sleep to give "foo-resource-quota" a chance to propagate from foo to bar, if it could. - time.Sleep(sleepTime) + time.Sleep(objectPropagationTime) // "foo-resource-quota" should not be propagated from foo to bar. Expect(hasObject(ctx, "ResourceQuota", barName, "foo-resource-quota")()).Should(BeFalse()) @@ -251,13 +261,14 @@ var _ = Describe("HNCConfiguration", func() { Expect(objectInheritedFrom(ctx, "Secret", barName, "foo-sec")).Should(Equal(fooName)) removeHNCConfigType(ctx, "v1", "Secret") + time.Sleep(hncConfigReconciliationTime) // Give foo another secret. makeObject(ctx, "Secret", fooName, "foo-sec-2") // Foo should have "foo-sec-2" because we created there. Eventually(hasObject(ctx, "Secret", fooName, "foo-sec-2")).Should(BeTrue()) // Sleep to give "foo-sec-2" a chance to propagate from foo to bar, if it could. - time.Sleep(sleepTime) + time.Sleep(objectPropagationTime) // "foo-role-2" should not propagate from foo to bar because the reconciliation request is ignored. Expect(hasObject(ctx, "Secret", barName, "foo-sec-2")()).Should(BeFalse()) @@ -268,13 +279,15 @@ var _ = Describe("HNCConfiguration", func() { addToHNCConfig(ctx, "stable.example.com/v1", "CronTab", api.Propagate) // The corresponding object reconciler should not be created because the type does not exist. - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "stable.example.com/v1, Kind=CronTab")).Should(BeTrue()) + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, + "stable.example.com/v1, Kind=CronTab"), hncConfigStatusUpdateTime).Should(BeTrue()) // Add the CRD for CronTab to the apiserver. createCronTabCRD(ctx) // The object reconciler for CronTab should be created successfully. - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "stable.example.com/v1, Kind=CronTab")).Should(BeFalse()) + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, + "stable.example.com/v1, Kind=CronTab"), hncConfigStatusUpdateTime).Should(BeFalse()) // Give foo a CronTab object. setParent(ctx, barName, fooName) @@ -284,6 +297,30 @@ var _ = Describe("HNCConfiguration", func() { Eventually(hasObject(ctx, "CronTab", barName, "foo-crontab")).Should(BeTrue()) Expect(objectInheritedFrom(ctx, "CronTab", barName, "foo-crontab")).Should(Equal(fooName)) }) + + It("should set NumPropagatedObjects back to 0 after deleting the source object in propagate mode", func() { + addToHNCConfig(ctx, "v1", "LimitRange", api.Propagate) + setParent(ctx, barName, fooName) + makeObject(ctx, "LimitRange", fooName, "foo-lr") + + Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), hncConfigStatusUpdateTime).Should(Equal(int32(1))) + + deleteObject(ctx, "LimitRange", fooName, "foo-lr") + + Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), hncConfigStatusUpdateTime).Should(Equal(int32(0))) + }) + + It("should set NumPropagatedObjects back to 0 after switching from propagate to remove mode", func() { + addToHNCConfig(ctx, "v1", "LimitRange", api.Propagate) + setParent(ctx, barName, fooName) + makeObject(ctx, "LimitRange", fooName, "foo-lr") + + Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), hncConfigStatusUpdateTime).Should(Equal(int32(1))) + + updateHNCConfigSpec(ctx, "v1", "v1", "LimitRange", "LimitRange", api.Propagate, api.Remove) + + Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), hncConfigStatusUpdateTime).Should(Equal(int32(0))) + }) }) func hasTypeWithMode(ctx context.Context, apiVersion, kind string, mode api.SynchronizationMode) func() bool { @@ -429,3 +466,31 @@ func createCronTabCRD(ctx context.Context) { return k8sClient.Create(ctx, &crontab) }).Should(Succeed()) } + +// getNumPropagatedObjects returns NumPropagatedObjects status for a given type. If NumPropagatedObjects is +// not set or if type does not exist in status, it returns -1 and an error. +func getNumPropagatedObjects(ctx context.Context, apiVersion, kind string) func() (int32, error) { + return func() (int32, error) { + c := getHNCConfig(ctx) + for _, t := range c.Status.Types { + if t.APIVersion == apiVersion && t.Kind == kind { + if t.NumPropagatedObjects != nil { + return *t.NumPropagatedObjects, nil + } + return -1, errors.New(fmt.Sprintf("NumPropagatedObjects 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)) + } +} + +// deleteObject deletes an object of the given kind in a specific namespace. The kind and +// its corresponding GVK should be included in the GVKs map. +func deleteObject(ctx context.Context, kind string, nsName, name string) { + inst := &unstructured.Unstructured{} + inst.SetGroupVersionKind(GVKs[kind]) + inst.SetNamespace(nsName) + inst.SetName(name) + ExpectWithOffset(1, k8sClient.Delete(ctx, inst)).Should(Succeed()) +} diff --git a/incubator/hnc/pkg/reconcilers/object.go b/incubator/hnc/pkg/reconcilers/object.go index a5f3b1a40..43d5ce2ae 100644 --- a/incubator/hnc/pkg/reconcilers/object.go +++ b/incubator/hnc/pkg/reconcilers/object.go @@ -19,6 +19,7 @@ import ( "context" "fmt" "reflect" + "sync" "github.com/go-logr/logr" "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/metadata" @@ -28,6 +29,7 @@ import ( v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -51,10 +53,14 @@ const ( ignore ) +type namespacedNameSet map[types.NamespacedName]bool + // ObjectReconciler reconciles generic propagated objects. You must create one for each // group/version/kind that needs to be propagated and set its `GVK` field appropriately. type ObjectReconciler struct { client.Client + lock sync.Mutex + Log logr.Logger // Forest is the in-memory forest managed by the HierarchyConfigReconciler. @@ -74,6 +80,9 @@ type ObjectReconciler struct { // AffectedNamespace is a channel of events used to update namespaces. AffectedNamespace chan event.GenericEvent + + // PropagatedObjects contains all propagated objects of the GVK handled by this reconciler. + PropagatedObjects namespacedNameSet } // +kubebuilder:rbac:groups=*,resources=*,verbs=get;list;watch;create;update;patch;delete @@ -111,15 +120,22 @@ func (r *ObjectReconciler) SetMode(ctx context.Context, mode api.Synchronization } r.Log.Info("Changing mode of the object reconciler", "old", oldMode, "new", newMode) r.Mode = newMode - // If the new mode is not "ignore", we need to update objects in the cluster - // (e.g., propagate or remove existing objects). - if newMode != api.Ignore { - err := r.enqueueAllObjects(ctx, r.Log) - if err != nil { - return err - } - } - return nil + // For propagate and remove mode, we enqueue all objects to update objects in the cluster (e.g., propagate or + // remove existing objects). For ignore mode, we enqueue objects so that reconciliations of objects can trigger + // the config reconciler to update the status of the `config` singleton. Triggering object reconciliations in the + // ignore mode is necessary because only an object reconciler can compute NumPropagatedObjects for a given type. + err := r.enqueueAllObjects(ctx, r.Log) + return err +} + +// GetNumPropagatedObjects returns the number of propagated objects of the GVK handled by this object reconciler. +func (r *ObjectReconciler) GetNumPropagatedObjects() int32 { + // We need to lock the map because there can be concurrent reconciliation threads reading/writing + // the map. + r.lock.Lock() + n := len(r.PropagatedObjects) + r.lock.Unlock() + return int32(n) } // getValidateMode returns a valid api.SynchronizationMode based on the given mode. Please @@ -159,9 +175,6 @@ func (r *ObjectReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { log := r.Log.WithValues("trigger", req.NamespacedName) r.Mode = r.getValidateMode(r.Mode, log) - if r.Mode == api.Ignore { - return ctrl.Result{}, nil - } stats.StartObjReconcile(r.GVK) defer stats.StopObjReconcile(r.GVK) @@ -180,9 +193,29 @@ func (r *ObjectReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { return resp, err } } + + // We need to compute NumPropagatedObjects and show it in the status of the `config` + // singleton even in ignore mode. + if r.Mode == api.Ignore { + if hasPropagatedLabel(inst) { + r.addToNumPropagated(log, inst.GetNamespace(), inst.GetName()) + r.Forest.GetConfigSyncer().SyncHNCConfigStatus(log) + } + return ctrl.Result{}, nil + } + act, srcInst := r.syncWithForest(ctx, log, inst) - return resp, r.operate(ctx, log, act, inst, srcInst) + err := r.operate(ctx, log, act, inst, srcInst) + if err != nil { + return resp, err + } + + // Only trigger the config reconciler to update NumPropagatedObjects status of the + // `config` singleton when operations on the apiserver succeeded. Otherwise, the + // NumPropagatedObjects is unchanged. + r.Forest.GetConfigSyncer().SyncHNCConfigStatus(log) + return resp, nil } // syncWithForest syncs the object instance with the in-memory forest. It returns the action to take on @@ -290,6 +323,7 @@ func (r *ObjectReconciler) syncPropagated(ctx context.Context, log logr.Logger, return write, srcInst } + r.addToNumPropagated(log, inst.GetNamespace(), inst.GetName()) return ignore, nil } @@ -408,6 +442,9 @@ func (r *ObjectReconciler) delete(ctx context.Context, log logr.Logger, inst *un return err } + // Remove the propagated object from the map because we are confident that the object was successfully deleted + // on the apiserver. + r.removeFromNumPropagated(log, inst.GetNamespace(), inst.GetName()) return nil } @@ -433,6 +470,10 @@ func (r *ObjectReconciler) write(ctx context.Context, log logr.Logger, inst, src if err != nil { r.setErrorConditions(log, srcInst, inst, op, err) log.Error(err, "Couldn't write", "object", inst) + } else { + // Add the object to the map if it does not exist because we are confident that the object was updated/created + // successfully on the apiserver. + r.addToNumPropagated(log, inst.GetNamespace(), inst.GetName()) } return err } @@ -539,6 +580,34 @@ func (r *ObjectReconciler) exclude(log logr.Logger, inst *unstructured.Unstructu } } +// addToNumPropagated adds a propagated object to the PropagatedObjects map if it does not exist. +func (r *ObjectReconciler) addToNumPropagated(log logr.Logger, namespace, name string) { + // We need to lock the map because there can be concurrent reconciliation threads reading/writing + // the map. + r.lock.Lock() + nnm := types.NamespacedName{ + Namespace: namespace, + Name: name, + } + if !r.PropagatedObjects[nnm] { + r.PropagatedObjects[nnm] = true + } + r.lock.Unlock() +} + +// removeFromNumPropagated removes a propagated object to the PropagatedObjects map. +func (r *ObjectReconciler) removeFromNumPropagated(log logr.Logger, namespace, name string) { + // We need to lock the map because there can be concurrent reconciliation threads reading/writing + // the map. + r.lock.Lock() + nnm := types.NamespacedName{ + Namespace: namespace, + Name: name, + } + delete(r.PropagatedObjects, nnm) + r.lock.Unlock() +} + func (r *ObjectReconciler) SetupWithManager(mgr ctrl.Manager, maxReconciles int) error { target := &unstructured.Unstructured{} target.SetGroupVersionKind(r.GVK) diff --git a/incubator/hnc/pkg/reconcilers/object_test.go b/incubator/hnc/pkg/reconcilers/object_test.go index 54fec9a3f..e46559fe5 100644 --- a/incubator/hnc/pkg/reconcilers/object_test.go +++ b/incubator/hnc/pkg/reconcilers/object_test.go @@ -44,13 +44,13 @@ var _ = Describe("Secret", func() { setParent(ctx, barName, fooName) setParent(ctx, bazName, barName) - Eventually(hasObject(ctx, "Role", barName, "foo-role")).Should(BeTrue()) + Eventually(hasObject(ctx, "Role", barName, "foo-role"), 4*time.Second).Should(BeTrue()) Expect(objectInheritedFrom(ctx, "Role", barName, "foo-role")).Should(Equal(fooName)) - Eventually(hasObject(ctx, "Role", bazName, "foo-role")).Should(BeTrue()) + Eventually(hasObject(ctx, "Role", bazName, "foo-role"), 4*time.Second).Should(BeTrue()) Expect(objectInheritedFrom(ctx, "Role", bazName, "foo-role")).Should(Equal(fooName)) - Eventually(hasObject(ctx, "Role", bazName, "bar-role")).Should(BeTrue()) + Eventually(hasObject(ctx, "Role", bazName, "bar-role"), 4*time.Second).Should(BeTrue()) Expect(objectInheritedFrom(ctx, "Role", bazName, "bar-role")).Should(Equal(barName)) }) @@ -62,7 +62,7 @@ var _ = Describe("Secret", func() { addToHNCConfig(ctx, "v1", "ConfigMap", api.Propagate) // "foo-config" should now be propagated from foo to bar. - Eventually(hasObject(ctx, "ConfigMap", barName, "foo-config")).Should(BeTrue()) + Eventually(hasObject(ctx, "ConfigMap", barName, "foo-config"), 4*time.Second).Should(BeTrue()) Expect(objectInheritedFrom(ctx, "ConfigMap", barName, "foo-config")).Should(Equal(fooName)) }) diff --git a/incubator/hnc/pkg/reconcilers/setup.go b/incubator/hnc/pkg/reconcilers/setup.go index 2070b3267..0cebaa4dd 100644 --- a/incubator/hnc/pkg/reconcilers/setup.go +++ b/incubator/hnc/pkg/reconcilers/setup.go @@ -74,7 +74,7 @@ func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int, enableHNSReco Log: ctrl.Log.WithName("reconcilers").WithName("HNCConfiguration"), Manager: mgr, Forest: f, - Igniter: make(chan event.GenericEvent), + Trigger: make(chan event.GenericEvent), HierarchyConfigUpdates: hcChan, } if err := cr.SetupWithManager(mgr); err != nil {