From 5060e81ad745006d1f4b2c160c9d2279371e0b96 Mon Sep 17 00:00:00 2001 From: Alona Kaplan Date: Thu, 19 Aug 2021 17:13:43 +0300 Subject: [PATCH] Set the node as the owner reference of the enactment If the node is removed the enactment should be garbage collected. Currently we have a leak. Setting both the node and the policy as the owner references of the enactment means that the enactmant will be removed only when both the node and the policy are removed. Therefore, We have to choose only one of the two to be the owner ref. The removal of the other one should be handled by the controller. We don't have a centrelized controller, node specific controllers are running on each node. It doesn't make sense to handle a node removal by a controller running on the removed node. Therefore, the node was chosen to be the owner ref while the policy removal will be handled by the policy controller. Signed-off-by: Alona Kaplan --- ...nodenetworkconfigurationenactment_types.go | 6 +-- ...etworkconfigurationenactment_types_test.go | 41 +++++++++++++++++++ ...denetworkconfigurationpolicy_controller.go | 33 ++++++++------- ...workconfigurationpolicy_controller_test.go | 11 ++--- 4 files changed, 67 insertions(+), 24 deletions(-) create mode 100644 api/v1beta1/nodenetworkconfigurationenactment_types_test.go diff --git a/api/v1beta1/nodenetworkconfigurationenactment_types.go b/api/v1beta1/nodenetworkconfigurationenactment_types.go index 4632ad8c9..c7b77c309 100644 --- a/api/v1beta1/nodenetworkconfigurationenactment_types.go +++ b/api/v1beta1/nodenetworkconfigurationenactment_types.go @@ -33,12 +33,12 @@ type NodeNetworkConfigurationEnactment struct { Status shared.NodeNetworkConfigurationEnactmentStatus `json:"status,omitempty"` } -func NewEnactment(nodeName string, policy NodeNetworkConfigurationPolicy) NodeNetworkConfigurationEnactment { +func NewEnactment(node *corev1.Node, policy NodeNetworkConfigurationPolicy) NodeNetworkConfigurationEnactment { enactment := NodeNetworkConfigurationEnactment{ ObjectMeta: metav1.ObjectMeta{ - Name: shared.EnactmentKey(nodeName, policy.Name).Name, + Name: shared.EnactmentKey(node.Name, policy.Name).Name, OwnerReferences: []metav1.OwnerReference{ - {Name: policy.Name, Kind: policy.TypeMeta.Kind, APIVersion: policy.TypeMeta.APIVersion, UID: policy.UID}, + {Name: node.Name, Kind: "Node", APIVersion: "v1", UID: node.UID}, }, // Associate policy with the enactment using labels Labels: names.IncludeRelationshipLabels(map[string]string{shared.EnactmentPolicyLabel: policy.Name}), diff --git a/api/v1beta1/nodenetworkconfigurationenactment_types_test.go b/api/v1beta1/nodenetworkconfigurationenactment_types_test.go new file mode 100644 index 000000000..a57715d21 --- /dev/null +++ b/api/v1beta1/nodenetworkconfigurationenactment_types_test.go @@ -0,0 +1,41 @@ +package v1beta1 + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var _ = Describe("NodeNetworkEnactment", func() { + var ( + nncp = NodeNetworkConfigurationPolicy{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "nmstate.io/v1beta1", + Kind: "NodeNetworkConfigurationPolicy", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "policy1", + UID: "12345", + }, + } + node = corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + UID: "54321", + }, + } + ) + + Context("NewEnactment", func() { + It("should have the node as the owner reference of the created enactment", func() { + nnce := NewEnactment(&node, nncp) + desiredOnwerRefs := []metav1.OwnerReference{ + {Name: node.Name, Kind: "Node", APIVersion: "v1", UID: node.UID}, + } + Expect(nnce.OwnerReferences).To(Equal(desiredOnwerRefs)) + }) + }) + +}) diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go index 34e3e6aad..b34ff8ac2 100644 --- a/controllers/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/nodenetworkconfigurationpolicy_controller.go @@ -57,14 +57,14 @@ import ( ) var ( - nodeName string - nodeRunningUpdateRetryTime = 5 * time.Second - onCreateOrUpdateWithDifferentGeneration = predicate.Funcs{ + nodeName string + nodeRunningUpdateRetryTime = 5 * time.Second + onCreateOrUpdateWithDifferentGenerationOrDelete = predicate.Funcs{ CreateFunc: func(createEvent event.CreateEvent) bool { return true }, DeleteFunc: func(deleteEvent event.DeleteEvent) bool { - return false + return true }, UpdateFunc: func(updateEvent event.UpdateEvent) bool { // [1] https://blog.openshift.com/kubernetes-operators-best-practices/ @@ -125,10 +125,9 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context err := r.Client.Get(context.TODO(), request.NamespacedName, instance) if err != nil { if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. - // Return and don't requeue - return ctrl.Result{}, nil + log.Info("Policy is not found, removing previous enactment if any") + err = r.deleteEnactmentForPolicy(request.NamespacedName.Name) + return ctrl.Result{}, err } log.Error(err, "Error retrieving policy") // Error reading the object - requeue the request. @@ -150,8 +149,8 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context } if len(unmatchingNodeLabels) > 0 { - log.Info("Policy node selectors does not match node, removing previous enactments if any") - err = r.deleteEnactmentForPolicy(instance) + log.Info("Policy node selectors does not match node, removing previous enactment if any") + err = r.deleteEnactmentForPolicy(request.NamespacedName.Name) return ctrl.Result{}, err } @@ -227,7 +226,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) SetupWithManager(mgr ctrl.Man // Reconcile NNCP if they are created or updated err := ctrl.NewControllerManagedBy(mgr). For(&nmstatev1beta1.NodeNetworkConfigurationPolicy{}). - WithEventFilter(onCreateOrUpdateWithDifferentGeneration). + WithEventFilter(onCreateOrUpdateWithDifferentGenerationOrDelete). Complete(r) if err != nil { return errors.Wrap(err, "failed to add controller to NNCP Reconciler listening NNCP events") @@ -256,7 +255,13 @@ func (r *NodeNetworkConfigurationPolicyReconciler) initializeEnactment(policy nm } if err != nil && apierrors.IsNotFound(err) { log.Info("creating enactment") - enactment = nmstatev1beta1.NewEnactment(nodeName, policy) + // Fetch the Node instance + nodeInstance := &corev1.Node{} + err = r.APIClient.Get(context.TODO(), types.NamespacedName{Name: nodeName}, nodeInstance) + if err != nil { + return nil, errors.Wrap(err, "failed getting node") + } + enactment = nmstatev1beta1.NewEnactment(nodeInstance, policy) err = r.APIClient.Create(context.TODO(), &enactment) if err != nil { return nil, errors.Wrapf(err, "error creating NodeNetworkConfigurationEnactment: %+v", enactment) @@ -294,8 +299,8 @@ func (r *NodeNetworkConfigurationPolicyReconciler) waitEnactmentCreated(enactmen return pollErr } -func (r *NodeNetworkConfigurationPolicyReconciler) deleteEnactmentForPolicy(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) error { - enactmentKey := nmstateapi.EnactmentKey(nodeName, policy.Name) +func (r *NodeNetworkConfigurationPolicyReconciler) deleteEnactmentForPolicy(policyName string) error { + enactmentKey := nmstateapi.EnactmentKey(nodeName, policyName) enactment := nmstatev1beta1.NodeNetworkConfigurationEnactment{ ObjectMeta: metav1.ObjectMeta{ Name: enactmentKey.Name, diff --git a/controllers/nodenetworkconfigurationpolicy_controller_test.go b/controllers/nodenetworkconfigurationpolicy_controller_test.go index a88f32570..4a5978d0b 100644 --- a/controllers/nodenetworkconfigurationpolicy_controller_test.go +++ b/controllers/nodenetworkconfigurationpolicy_controller_test.go @@ -24,7 +24,6 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func() type predicateCase struct { GenerationOld int64 GenerationNew int64 - ReconcileCreate bool ReconcileUpdate bool } DescribeTable("testing predicates", @@ -40,12 +39,12 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func() }, } - predicate := onCreateOrUpdateWithDifferentGeneration + predicate := onCreateOrUpdateWithDifferentGenerationOrDelete Expect(predicate. CreateFunc(event.CreateEvent{ Object: &newNNCP, - })).To(Equal(c.ReconcileCreate)) + })).To(BeTrue()) Expect(predicate. UpdateFunc(event.UpdateEvent{ ObjectOld: &oldNNCP, @@ -53,21 +52,19 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func() })).To(Equal(c.ReconcileUpdate)) Expect(predicate. DeleteFunc(event.DeleteEvent{ - Object: &newNNCP, - })).To(BeFalse()) + Object: &oldNNCP, + })).To(BeTrue()) }, Entry("generation remains the same", predicateCase{ GenerationOld: 1, GenerationNew: 1, - ReconcileCreate: true, ReconcileUpdate: false, }), Entry("generation is different", predicateCase{ GenerationOld: 1, GenerationNew: 2, - ReconcileCreate: true, ReconcileUpdate: true, }), )