From 9fa5fe079caafd0a06baf356f9311cdc1c676408 Mon Sep 17 00:00:00 2001 From: Radim Hrazdil <32546791+rhrazdil@users.noreply.github.com> Date: Thu, 29 Apr 2021 09:35:21 +0200 Subject: [PATCH] Refactor nncp controller (#732) * Reorder nnpc controller functions The first thing that reader is presumably interested in when opening nodenetworkconfigurationpolicy_controller.go file is the Reconcile loop. In this commit, I'm moving the Reconcile loop to the top of the file, and reordering the helper functions so that the reader can see the details in the same order in which these helper functions are called in Reconcile. Signed-off-by: Radim Hrazdil * Remove unused method enactmentsCountByPolicy enactmentsCountByPolicy is not used, let's remove it. Signed-off-by: Radim Hrazdil --- ...denetworkconfigurationpolicy_controller.go | 202 ++++++++---------- 1 file changed, 95 insertions(+), 107 deletions(-) diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go index 43dede1a99..19279a28b6 100644 --- a/controllers/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/nodenetworkconfigurationpolicy_controller.go @@ -89,17 +89,6 @@ var ( } ) -func init() { - if !environment.IsHandler() { - return - } - - nodeName = environment.NodeName() - if len(nodeName) == 0 { - panic("NODE_NAME is mandatory") - } -} - // NodeNetworkConfigurationPolicyReconciler reconciles a NodeNetworkConfigurationPolicy object type NodeNetworkConfigurationPolicyReconciler struct { client.Client @@ -107,103 +96,14 @@ type NodeNetworkConfigurationPolicyReconciler struct { Scheme *runtime.Scheme } -func (r *NodeNetworkConfigurationPolicyReconciler) waitEnactmentCreated(enactmentKey types.NamespacedName) error { - var enactment nmstatev1beta1.NodeNetworkConfigurationEnactment - pollErr := wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { - err := r.Client.Get(context.TODO(), enactmentKey, &enactment) - if err != nil { - if apierrors.IsNotFound(err) { - // Let's retry after a while, sometimes it takes some time - // for enactment to be created - return false, nil - } - return false, err - } - return true, nil - }) - - return pollErr -} - -func (r *NodeNetworkConfigurationPolicyReconciler) initializeEnactment(policy nmstatev1beta1.NodeNetworkConfigurationPolicy) error { - enactmentKey := nmstateapi.EnactmentKey(nodeName, policy.Name) - log := r.Log.WithName("initializeEnactment").WithValues("policy", policy.Name, "enactment", enactmentKey.Name) - // Return if it's already initialize or we cannot retrieve it - enactment := nmstatev1beta1.NodeNetworkConfigurationEnactment{} - err := r.Client.Get(context.TODO(), enactmentKey, &enactment) - if err != nil && !apierrors.IsNotFound(err) { - return errors.Wrap(err, "failed getting enactment ") - } - if err != nil && apierrors.IsNotFound(err) { - log.Info("creating enactment") - enactment = nmstatev1beta1.NewEnactment(nodeName, policy) - err = r.Client.Create(context.TODO(), &enactment) - if err != nil { - return errors.Wrapf(err, "error creating NodeNetworkConfigurationEnactment: %+v", enactment) - } - err = r.waitEnactmentCreated(enactmentKey) - if err != nil { - return errors.Wrapf(err, "error waitting for NodeNetworkConfigurationEnactment: %+v", enactment) - } - } else { - enactmentConditions := enactmentconditions.New(r.Client, enactmentKey) - enactmentConditions.Reset() - } - - return enactmentstatus.Update(r.Client, enactmentKey, func(status *nmstateapi.NodeNetworkConfigurationEnactmentStatus) { - status.DesiredState = policy.Spec.DesiredState - status.PolicyGeneration = policy.Generation - }) -} - -func (r *NodeNetworkConfigurationPolicyReconciler) enactmentsCountByPolicy(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) (enactmentconditions.ConditionCount, error) { - enactments := nmstatev1beta1.NodeNetworkConfigurationEnactmentList{} - policyLabelFilter := client.MatchingLabels{nmstateapi.EnactmentPolicyLabel: policy.GetName()} - err := r.Client.List(context.TODO(), &enactments, policyLabelFilter) - if err != nil { - return nil, errors.Wrap(err, "getting enactment list failed") - } - enactmentCount := enactmentconditions.Count(enactments, policy.Generation) - return enactmentCount, nil -} - -func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) error { - policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()} - err := r.Client.Get(context.TODO(), policyKey, policy) - if err != nil { - return err - } - maxUnavailable, err := node.MaxUnavailableNodeCount(r.Client, policy) - if err != nil { - return err - } - if policy.Status.UnavailableNodeCount >= maxUnavailable { - return apierrors.NewConflict(schema.GroupResource{Resource: "nodenetworkconfigurationpolicies"}, policy.Name, fmt.Errorf("maximal number of %d nodes are already processing policy configuration", policy.Status.UnavailableNodeCount)) - } - policy.Status.UnavailableNodeCount += 1 - err = r.Client.Status().Update(context.TODO(), policy) - if err != nil { - return err +func init() { + if !environment.IsHandler() { + return } - return nil -} -func (r *NodeNetworkConfigurationPolicyReconciler) decrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) { - policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()} - instance := &nmstatev1beta1.NodeNetworkConfigurationPolicy{} - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - err := r.Client.Get(context.TODO(), policyKey, instance) - if err != nil { - return err - } - if instance.Status.UnavailableNodeCount <= 0 { - return fmt.Errorf("no unavailable nodes") - } - instance.Status.UnavailableNodeCount -= 1 - return r.Client.Status().Update(context.TODO(), instance) - }) - if err != nil { - r.Log.Error(err, "error decrementing unavailableNodeCount") + nodeName = environment.NodeName() + if len(nodeName) == 0 { + panic("NODE_NAME is mandatory") } } @@ -299,7 +199,6 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context } func (r *NodeNetworkConfigurationPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { - allPolicies := handler.MapFunc( func(client.Object) []reconcile.Request { log := r.Log.WithName("allPolicies") @@ -340,6 +239,95 @@ func (r *NodeNetworkConfigurationPolicyReconciler) SetupWithManager(mgr ctrl.Man return nil } +func (r *NodeNetworkConfigurationPolicyReconciler) initializeEnactment(policy nmstatev1beta1.NodeNetworkConfigurationPolicy) error { + enactmentKey := nmstateapi.EnactmentKey(nodeName, policy.Name) + log := r.Log.WithName("initializeEnactment").WithValues("policy", policy.Name, "enactment", enactmentKey.Name) + // Return if it's already initialize or we cannot retrieve it + enactment := nmstatev1beta1.NodeNetworkConfigurationEnactment{} + err := r.Client.Get(context.TODO(), enactmentKey, &enactment) + if err != nil && !apierrors.IsNotFound(err) { + return errors.Wrap(err, "failed getting enactment ") + } + if err != nil && apierrors.IsNotFound(err) { + log.Info("creating enactment") + enactment = nmstatev1beta1.NewEnactment(nodeName, policy) + err = r.Client.Create(context.TODO(), &enactment) + if err != nil { + return errors.Wrapf(err, "error creating NodeNetworkConfigurationEnactment: %+v", enactment) + } + err = r.waitEnactmentCreated(enactmentKey) + if err != nil { + return errors.Wrapf(err, "error waitting for NodeNetworkConfigurationEnactment: %+v", enactment) + } + } else { + enactmentConditions := enactmentconditions.New(r.Client, enactmentKey) + enactmentConditions.Reset() + } + + return enactmentstatus.Update(r.Client, enactmentKey, func(status *nmstateapi.NodeNetworkConfigurationEnactmentStatus) { + status.DesiredState = policy.Spec.DesiredState + status.PolicyGeneration = policy.Generation + }) +} + +func (r *NodeNetworkConfigurationPolicyReconciler) waitEnactmentCreated(enactmentKey types.NamespacedName) error { + var enactment nmstatev1beta1.NodeNetworkConfigurationEnactment + pollErr := wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { + err := r.Client.Get(context.TODO(), enactmentKey, &enactment) + if err != nil { + if apierrors.IsNotFound(err) { + // Let's retry after a while, sometimes it takes some time + // for enactment to be created + return false, nil + } + return false, err + } + return true, nil + }) + + return pollErr +} + +func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) error { + policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()} + err := r.Client.Get(context.TODO(), policyKey, policy) + if err != nil { + return err + } + maxUnavailable, err := node.MaxUnavailableNodeCount(r.Client, policy) + if err != nil { + return err + } + if policy.Status.UnavailableNodeCount >= maxUnavailable { + return apierrors.NewConflict(schema.GroupResource{Resource: "nodenetworkconfigurationpolicies"}, policy.Name, fmt.Errorf("maximal number of %d nodes are already processing policy configuration", policy.Status.UnavailableNodeCount)) + } + policy.Status.UnavailableNodeCount += 1 + err = r.Client.Status().Update(context.TODO(), policy) + if err != nil { + return err + } + return nil +} + +func (r *NodeNetworkConfigurationPolicyReconciler) decrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) { + policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()} + instance := &nmstatev1beta1.NodeNetworkConfigurationPolicy{} + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + err := r.Client.Get(context.TODO(), policyKey, instance) + if err != nil { + return err + } + if instance.Status.UnavailableNodeCount <= 0 { + return fmt.Errorf("no unavailable nodes") + } + instance.Status.UnavailableNodeCount -= 1 + return r.Client.Status().Update(context.TODO(), instance) + }) + if err != nil { + r.Log.Error(err, "error decrementing unavailableNodeCount") + } +} + func (r *NodeNetworkConfigurationPolicyReconciler) forceNNSRefresh(name string) { log := r.Log.WithName("forceNNSRefresh").WithValues("node", name) log.Info("forcing NodeNetworkState refresh after NNCP applied")