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..d0d5780a4 100644 --- a/incubator/hnc/pkg/forest/forest.go +++ b/incubator/hnc/pkg/forest/forest.go @@ -32,6 +32,16 @@ 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 + // GetMode gets the propagation mode of objects that are handled by the reconciler who implements the interface. + GetMode() api.SynchronizationMode + // GetNumPropagatedObjects returns the number of propagated objects on the apiserver. + 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) } // Forest defines a forest of namespaces - that is, a set of trees. It includes methods to mutate @@ -53,6 +63,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 NumPropagatedObjectsSyncer } func NewForest() *Forest { diff --git a/incubator/hnc/pkg/reconcilers/hnc_config.go b/incubator/hnc/pkg/reconcilers/hnc_config.go index 17a0761d6..ccb04bfd7 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config.go @@ -3,6 +3,9 @@ package reconcilers import ( "context" "fmt" + "sort" + "sync" + "time" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -33,19 +36,35 @@ 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 + + // shouldReconcileLock is used to prevent reconcilers trying to access shouldReconcile at the same time. + shouldReconcileLock sync.Mutex + + // shouldReconcile is used by object reconcilers to signal config reconciler to reconcile. + // Object reconcilers will set shouldReconcile to be true when an object is successfully reconciled + // and the status of the `config` singleton might need to be updated. + shouldReconcile bool } +// gvkSet keeps track of a group of unique GVKs. type gvkSet map[schema.GroupVersionKind]bool +// checkPeriod is the period that the config reconciler checks if it needs to reconcile the +// `config` singleton. +const checkPeriod = 3 * time.Second + // 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) { @@ -63,18 +82,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 +243,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 +251,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 @@ -311,9 +331,10 @@ func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind, m Log: ctrl.Log.WithName("reconcilers").WithName(gvk.Kind), Forest: r.Forest, GVK: gvk, - Mode: mode, + Mode: GetValidateMode(mode, r.Log), 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 +383,89 @@ 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. Only the status of +// types in `propagate` and `remove` modes will be recorded. The Status.Types +// is sorted in alphabetical order based on APIVersion. +func (r *ConfigReconciler) addTypeStatus(inst *api.HNCConfiguration) { + for _, ts := range r.Forest.GetTypeSyncers() { + if r.activeGVKs[ts.GetGVK()] && ts.GetMode() != api.Ignore { + r.addNumPropagatedObjects(ts.GetGVK(), ts.GetNumPropagatedObjects(), inst) + } + } + sort.Slice(inst.Status.Types, func(i, j int) bool { + return inst.Status.Types[i].APIVersion < inst.Status.Types[j].APIVersion + }) +} + +// addNumPropagatedObjects adds the NumPropagatedObjects field for a given GVK in the status. +func (r *ConfigReconciler) addNumPropagatedObjects(gvk schema.GroupVersionKind, num int, + inst *api.HNCConfiguration) { + apiVersion, kind := gvk.ToAPIVersionAndKind() + n := int32(num) + inst.Status.Types = append(inst.Status.Types, api.TypeSynchronizationStatus{ + APIVersion: apiVersion, + Kind: kind, + NumPropagatedObjects: &n, + }) +} + +// 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} }() } +func (r *ConfigReconciler) getShouldReconcile() bool { + r.shouldReconcileLock.Lock() + defer r.shouldReconcileLock.Unlock() + + return r.shouldReconcile +} + +func (r *ConfigReconciler) unsetShouldReconcile() { + r.shouldReconcileLock.Lock() + defer r.shouldReconcileLock.Unlock() + + r.shouldReconcile = false +} + +// periodicTrigger periodically checks if the `config` singleton needs to be reconciled and +// enqueues the `config` singleton for reconciliation, if needed. +// Object reconcilers signal the config reconciler to reconcile when the status needs to +// be updated. The config reconciler only reconciles periodically so that the reconciliation +// won't be triggered too frequently. +func (r *ConfigReconciler) periodicTrigger() { + // run forever + for { + time.Sleep(checkPeriod) + if r.getShouldReconcile() == false { + continue + } + msg := fmt.Sprintf("Syncing NumPropagatedObjects in the status") + r.enqueueSingleton(r.Log, msg) + r.unsetShouldReconcile() + } +} + +// 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) { + r.shouldReconcileLock.Lock() + defer r.shouldReconcileLock.Unlock() + + log.Info("Signalling config reconciler for reconciliation.") + r.shouldReconcile = true +} + // 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 +481,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 +497,18 @@ 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") + + // Inform the forest about the config reconciler so that object reconcilers can + // signal the config reconciler to reconcile. + r.Forest.Config = r + + // Periodically checks if the config reconciler needs to reconcile and trigger the + // reconciliation if needed, in case the status needs to be updated. This occurs + // in a goroutine so the caller doesn't block; since the reconciler is never + // garbage-collected, this is safe. + go r.periodicTrigger() + return nil } diff --git a/incubator/hnc/pkg/reconcilers/hnc_config_test.go b/incubator/hnc/pkg/reconcilers/hnc_config_test.go index 02e57384c..6bd4f69f8 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,16 @@ 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 + ctx := context.Background() var ( @@ -28,6 +31,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 +43,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() { @@ -99,22 +106,15 @@ var _ = Describe("HNCConfiguration", func() { 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" + It("should unset ObjectReconcilerCreationFailed condition if an object reconciler creation later succeeds", func() { + // API version of ConfigMap should be "v1" addToHNCConfig(ctx, "v2", "ConfigMap", api.Propagate) Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=ConfigMap")).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) + updateHNCConfigSpec(ctx, "v2", "v1", "ConfigMap", "ConfigMap", api.Propagate, api.Propagate) - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=LimitRange")).Should(BeTrue()) - - updateHNCConfigSpec(ctx, "v2", "v1", "LimitRange", "LimitRange", api.Propagate, api.Propagate) - - Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=LimitRange")).Should(BeFalse()) + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=ConfigMap")).Should(BeFalse()) }) It("should set MultipleConfigurationsForOneType if there are multiple configurations for one type", func() { @@ -148,7 +148,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 +178,11 @@ var _ = Describe("HNCConfiguration", func() { Expect(objectInheritedFrom(ctx, "Secret", barName, "foo-sec")).Should(Equal(fooName)) updateHNCConfigSpec(ctx, "v1", "v1", "Secret", "Secret", api.Propagate, api.Ignore) + 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 +195,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 +231,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()) @@ -257,7 +258,7 @@ var _ = Describe("HNCConfiguration", func() { // 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 +269,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")).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")).Should(BeFalse()) // Give foo a CronTab object. setParent(ctx, barName, fooName) @@ -284,6 +287,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"), 4*time.Second).Should(Equal(int32(1))) + + deleteObject(ctx, "LimitRange", fooName, "foo-lr") + + Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), 4*time.Second).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"), 4*time.Second).Should(Equal(int32(1))) + + updateHNCConfigSpec(ctx, "v1", "v1", "LimitRange", "LimitRange", api.Propagate, api.Remove) + + Eventually(getNumPropagatedObjects(ctx, "v1", "LimitRange"), 4*time.Second).Should(Equal(int32(0))) + }) }) func hasTypeWithMode(ctx context.Context, apiVersion, kind string, mode api.SynchronizationMode) func() bool { @@ -429,3 +456,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..4ae33dcb1 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,6 +53,10 @@ const ( ignore ) +// namespacedNameSet is used to keep track of existing propagated objects of +// a specific GVK in the cluster. +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 { @@ -74,6 +80,13 @@ type ObjectReconciler struct { // AffectedNamespace is a channel of events used to update namespaces. AffectedNamespace chan event.GenericEvent + + // propagatedObjectsLock is used to prevent the race condition between concurrent reconciliation threads + // trying to update propagatedObjects at the same time. + propagatedObjectsLock sync.Mutex + + // 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 @@ -100,11 +113,33 @@ func (r *ObjectReconciler) GetGVK() schema.GroupVersionKind { return r.GVK } +// GetMode provides the mode of objects that are handled by this reconciler. +func (r *ObjectReconciler) GetMode() api.SynchronizationMode { + return r.Mode +} + +// GetValidateMode returns a valid api.SynchronizationMode based on the given mode. Please +// see the comments of api.SynchronizationMode for currently supported modes. +// If mode is not set, it will be api.Propagate by default. Any unrecognized mode is +// treated as api.Ignore. +func GetValidateMode(mode api.SynchronizationMode, log logr.Logger) api.SynchronizationMode { + switch mode { + case api.Propagate, api.Ignore, api.Remove: + return mode + case "": + log.Info("Unset mode; using 'propagate'") + return api.Propagate + default: + log.Info("Unrecognized mode; using 'ignore'", "mode", mode) + return api.Ignore + } +} + // SetMode sets the Mode field of an object reconciler and syncs objects in the cluster if needed. // The method will return an error if syncs fail. func (r *ObjectReconciler) SetMode(ctx context.Context, mode api.SynchronizationMode, log logr.Logger) error { log = log.WithValues("gvk", r.GVK) - newMode := r.getValidateMode(mode, log) + newMode := GetValidateMode(mode, log) oldMode := r.Mode if newMode == oldMode { return nil @@ -122,21 +157,14 @@ func (r *ObjectReconciler) SetMode(ctx context.Context, mode api.Synchronization return nil } -// getValidateMode returns a valid api.SynchronizationMode based on the given mode. Please -// see the comments of api.SynchronizationMode for currently supported modes. -// If mode is not set, it will be api.Propagate by default. Any unrecognized mode is -// treated as api.Ignore. -func (r *ObjectReconciler) getValidateMode(mode api.SynchronizationMode, log logr.Logger) api.SynchronizationMode { - switch mode { - case api.Propagate, api.Ignore, api.Remove: - return mode - case "": - log.Info("Unset mode; using 'propagate'") - return api.Propagate - default: - log.Info("Unrecognized mode; using 'ignore'", "mode", mode) - return api.Ignore - } +// GetNumPropagatedObjects returns the number of propagated objects of the GVK handled by this object reconciler. +func (r *ObjectReconciler) GetNumPropagatedObjects() int { + // We need to lock the map because there can be concurrent reconciliation threads reading/writing + // the map. + r.propagatedObjectsLock.Lock() + defer r.propagatedObjectsLock.Unlock() + + return len(r.propagatedObjects) } // enqueueAllObjects enqueues all the current objects in all namespaces. @@ -158,7 +186,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 } @@ -180,9 +207,19 @@ func (r *ObjectReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { return resp, err } } + 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 succeeds. Otherwise, the + // NumPropagatedObjects is unchanged. + r.Forest.Config.SyncNumPropagatedObjects(log) + return resp, nil } // syncWithForest syncs the object instance with the in-memory forest. It returns the action to take on @@ -290,6 +327,10 @@ func (r *ObjectReconciler) syncPropagated(ctx context.Context, log logr.Logger, return write, srcInst } + // Add the propagated object to the map if it is not in the map because at this point we are confident that + // this is a propagated object that is already on the apiserver, if this is the first time we reconcile this + // object, we should include it in the map. + r.addToNumPropagated(log, inst.GetNamespace(), inst.GetName()) return ignore, nil } @@ -408,6 +449,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 +477,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 +587,32 @@ 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) { + r.propagatedObjectsLock.Lock() + defer r.propagatedObjectsLock.Unlock() + + nnm := types.NamespacedName{ + Namespace: namespace, + Name: name, + } + if !r.propagatedObjects[nnm] { + r.propagatedObjects[nnm] = true + } +} + +// removeFromNumPropagated removes a propagated object to the PropagatedObjects map. +func (r *ObjectReconciler) removeFromNumPropagated(log logr.Logger, namespace, name string) { + r.propagatedObjectsLock.Lock() + defer r.propagatedObjectsLock.Unlock() + + nnm := types.NamespacedName{ + Namespace: namespace, + Name: name, + } + delete(r.propagatedObjects, nnm) +} + func (r *ObjectReconciler) SetupWithManager(mgr ctrl.Manager, maxReconciles int) error { target := &unstructured.Unstructured{} target.SetGroupVersionKind(r.GVK) 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 {