From 9b9bb82b1714006a8ef8cbbc1dcf0d638ff393ed Mon Sep 17 00:00:00 2001 From: Yanjun Zhou Date: Thu, 3 Oct 2024 15:31:37 +0800 Subject: [PATCH] Remove SubnetPort/Pod finalizer Signed-off-by: Yanjun Zhou --- pkg/controllers/pod/pod_controller.go | 95 +++++++------- .../subnetport/subnetport_controller.go | 118 ++++++++--------- .../subnetport/subnetport_controller_test.go | 121 +++++++----------- pkg/nsx/services/common/types.go | 2 - pkg/nsx/services/subnetport/builder.go | 3 - pkg/nsx/services/subnetport/store.go | 18 +++ pkg/nsx/services/subnetport/subnetport.go | 68 ++++++++-- 7 files changed, 222 insertions(+), 203 deletions(-) diff --git a/pkg/controllers/pod/pod_controller.go b/pkg/controllers/pod/pod_controller.go index c13959a71..5c8797ef6 100644 --- a/pkg/controllers/pod/pod_controller.go +++ b/pkg/controllers/pod/pod_controller.go @@ -7,18 +7,17 @@ import ( "context" "fmt" "os" + "time" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimachineryruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" "github.com/vmware-tanzu/nsx-operator/pkg/controllers/common" "github.com/vmware-tanzu/nsx-operator/pkg/logger" @@ -45,14 +44,25 @@ type PodReconciler struct { } func (r *PodReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - pod := &v1.Pod{} log.Info("reconciling pod", "pod", req.NamespacedName) + startTime := time.Now() + defer func() { + log.Info("finished reconciling Pod", "Pod", req.NamespacedName, "duration", time.Since(startTime)) + }() metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerSyncTotal, MetricResTypePod) + pod := &v1.Pod{} if err := r.Client.Get(ctx, req.NamespacedName, pod); err != nil { - log.Error(err, "unable to fetch pod", "req", req.NamespacedName) - return common.ResultNormal, client.IgnoreNotFound(err) + if apierrors.IsNotFound(err) { + if err := r.deleteSubnetPortByPodName(ctx, req.Namespace, req.Name); err != nil { + log.Error(err, "failed to delete NSX SubnetPort", "SubnetPort", req.NamespacedName) + return common.ResultRequeue, err + } + return common.ResultNormal, nil + } + log.Error(err, "unable to fetch Pod", "Pod", req.NamespacedName) + return common.ResultRequeue, err } if pod.Spec.HostNetwork { log.Info("skipping handling hostnetwork pod", "pod", req.NamespacedName) @@ -65,16 +75,6 @@ func (r *PodReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R if !podIsDeleted(pod) { metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerUpdateTotal, MetricResTypePod) - if !controllerutil.ContainsFinalizer(pod, servicecommon.PodFinalizerName) { - controllerutil.AddFinalizer(pod, servicecommon.PodFinalizerName) - if err := r.Client.Update(ctx, pod); err != nil { - log.Error(err, "add finalizer", "pod", req.NamespacedName) - updateFail(r, ctx, pod, &err) - return common.ResultRequeue, err - } - log.Info("added finalizer on pod", "pod", req.NamespacedName) - } - nsxSubnetPath, err := r.GetSubnetPathForPod(ctx, pod) if err != nil { log.Error(err, "failed to get NSX resource path from subnet", "pod.Name", pod.Name, "pod.UID", pod.UID) @@ -105,25 +105,13 @@ func (r *PodReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R } updateSuccess(r, ctx, pod) } else { - if controllerutil.ContainsFinalizer(pod, servicecommon.PodFinalizerName) { - metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypePod) - subnetPortID := r.SubnetPortService.BuildSubnetPortId(&pod.ObjectMeta) - if err := r.SubnetPortService.DeleteSubnetPort(subnetPortID); err != nil { - log.Error(err, "deletion failed, would retry exponentially", "pod", req.NamespacedName) - deleteFail(r, ctx, pod, &err) - return common.ResultRequeue, err - } - controllerutil.RemoveFinalizer(pod, servicecommon.PodFinalizerName) - if err := r.Client.Update(ctx, pod); err != nil { - log.Error(err, "deletion failed, would retry exponentially", "pod", req.NamespacedName) - deleteFail(r, ctx, pod, &err) - return common.ResultRequeue, err - } - log.Info("removed finalizer", "pod", req.NamespacedName) - deleteSuccess(r, ctx, pod) - } else { - log.Info("finalizers cannot be recognized", "pod", req.NamespacedName) + subnetPortID := r.SubnetPortService.BuildSubnetPortId(&pod.ObjectMeta) + if err := r.SubnetPortService.DeleteSubnetPortById(subnetPortID); err != nil { + log.Error(err, "deletion failed, would retry exponentially", "pod", req.NamespacedName) + deleteFail(r, ctx, pod, &err) + return common.ResultRequeue, err } + deleteSuccess(r, ctx, pod) } return ctrl.Result{}, nil } @@ -147,14 +135,6 @@ func (r *PodReconciler) GetNodeByName(nodeName string) (*model.HostTransportNode func (r *PodReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&v1.Pod{}). - WithEventFilter( - predicate.Funcs{ - DeleteFunc: func(e event.DeleteEvent) bool { - // Suppress Delete events to avoid filtering them out in the Reconcile function - return false - }, - }, - ). WithOptions( controller.Options{ MaxConcurrentReconciles: common.NumReconcile(), @@ -212,7 +192,7 @@ func (r *PodReconciler) CollectGarbage(ctx context.Context) { for elem := range diffSet { log.V(1).Info("GC collected Pod", "NSXSubnetPortID", elem) metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypePod) - err = r.SubnetPortService.DeleteSubnetPort(elem) + err = r.SubnetPortService.DeleteSubnetPortById(elem) if err != nil { metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResTypePod) } else { @@ -221,17 +201,17 @@ func (r *PodReconciler) CollectGarbage(ctx context.Context) { } } -func updateFail(r *PodReconciler, c context.Context, o *v1.Pod, e *error) { +func updateFail(r *PodReconciler, _ context.Context, o *v1.Pod, e *error) { r.Recorder.Event(o, v1.EventTypeWarning, common.ReasonFailUpdate, fmt.Sprintf("%v", *e)) metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerUpdateFailTotal, MetricResTypePod) } -func deleteFail(r *PodReconciler, c context.Context, o *v1.Pod, e *error) { +func deleteFail(r *PodReconciler, _ context.Context, o *v1.Pod, e *error) { r.Recorder.Event(o, v1.EventTypeWarning, common.ReasonFailDelete, fmt.Sprintf("%v", *e)) metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResTypePod) } -func updateSuccess(r *PodReconciler, c context.Context, o *v1.Pod) { +func updateSuccess(r *PodReconciler, _ context.Context, o *v1.Pod) { r.Recorder.Event(o, v1.EventTypeNormal, common.ReasonSuccessfulUpdate, "Pod CR has been successfully updated") metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerUpdateSuccessTotal, MetricResTypePod) } @@ -264,3 +244,26 @@ func (r *PodReconciler) GetSubnetPathForPod(ctx context.Context, pod *v1.Pod) (s func podIsDeleted(pod *v1.Pod) bool { return !pod.ObjectMeta.DeletionTimestamp.IsZero() || pod.Status.Phase == "Succeeded" || pod.Status.Phase == "Failed" } + +func (r *PodReconciler) deleteSubnetPortByPodName(ctx context.Context, ns string, name string) error { + // When deleting SubnetPort by Name and Namespace, skip the SubnetPort belonging to the existed SubnetPort CR + nsxSubnetPorts := r.SubnetPortService.ListSubnetPortByPodName(ns, name) + + crSubnetPortIDsSet, err := r.SubnetPortService.ListSubnetPortIDsFromCRs(ctx) + if err != nil { + log.Error(err, "failed to list SubnetPort CRs") + return err + } + + for _, nsxSubnetPort := range nsxSubnetPorts { + if crSubnetPortIDsSet.Has(*nsxSubnetPort.Id) { + log.Info("skipping deletion, Pod CR still exists in K8s", "ID", *nsxSubnetPort.Id) + continue + } + if err := r.SubnetPortService.DeleteSubnetPort(nsxSubnetPort); err != nil { + return err + } + } + log.Info("successfully deleted nsxSubnetPort for Pod", "namespace", ns, "name", name) + return nil +} diff --git a/pkg/controllers/subnetport/subnetport_controller.go b/pkg/controllers/subnetport/subnetport_controller.go index ee6a15cfc..468995878 100644 --- a/pkg/controllers/subnetport/subnetport_controller.go +++ b/pkg/controllers/subnetport/subnetport_controller.go @@ -10,21 +10,20 @@ import ( "os" "reflect" "strings" + "time" vmv1alpha1 "github.com/vmware-tanzu/vm-operator/api/v1alpha1" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apimachineryruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -57,18 +56,27 @@ type SubnetPortReconciler struct { // +kubebuilder:rbac:groups=nsx.vmware.com,resources=subnetports,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=nsx.vmware.com,resources=subnetports/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=nsx.vmware.com,resources=subnetports/finalizers,verbs=update func (r *SubnetPortReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - subnetPort := &v1alpha1.SubnetPort{} log.Info("reconciling subnetport CR", "subnetport", req.NamespacedName) + startTime := time.Now() + defer func() { + log.Info("finished reconciling SubnetPort", "SubnetPort", req.NamespacedName, "duration", time.Since(startTime)) + }() metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerSyncTotal, MetricResTypeSubnetPort) + subnetPort := &v1alpha1.SubnetPort{} if err := r.Client.Get(ctx, req.NamespacedName, subnetPort); err != nil { - log.Error(err, "unable to fetch subnetport CR", "req", req.NamespacedName) - return common.ResultNormal, client.IgnoreNotFound(err) + if apierrors.IsNotFound(err) { + if err := r.deleteSubnetPortByName(ctx, req.Namespace, req.Name); err != nil { + log.Error(err, "failed to delete NSX SubnetPort", "SubnetPort", req.NamespacedName) + return common.ResultRequeue, err + } + return common.ResultNormal, nil + } + log.Error(err, "unable to fetch SubnetPort CR", "SubnetPort", req.NamespacedName) + return common.ResultRequeue, err } - if len(subnetPort.Spec.SubnetSet) > 0 && len(subnetPort.Spec.Subnet) > 0 { err := errors.New("subnet and subnetset should not be configured at the same time") log.Error(err, "failed to get subnet/subnetset of the subnetport", "subnetport", req.NamespacedName) @@ -78,15 +86,6 @@ func (r *SubnetPortReconciler) Reconcile(ctx context.Context, req ctrl.Request) if subnetPort.ObjectMeta.DeletionTimestamp.IsZero() { metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerUpdateTotal, MetricResTypeSubnetPort) - if !controllerutil.ContainsFinalizer(subnetPort, servicecommon.SubnetPortFinalizerName) { - controllerutil.AddFinalizer(subnetPort, servicecommon.SubnetPortFinalizerName) - if err := r.Client.Update(ctx, subnetPort); err != nil { - log.Error(err, "add finalizer", "subnetport", req.NamespacedName) - updateFail(r, ctx, subnetPort, &err) - return common.ResultRequeue, err - } - log.Info("added finalizer on subnetport CR", "subnetport", req.NamespacedName) - } old_status := subnetPort.Status.DeepCopy() isParentResourceTerminating, nsxSubnetPath, err := r.CheckAndGetSubnetPathForSubnetPort(ctx, subnetPort) @@ -145,27 +144,16 @@ func (r *SubnetPortReconciler) Reconcile(ctx context.Context, req ctrl.Request) } updateSuccess(r, ctx, subnetPort) } else { - if controllerutil.ContainsFinalizer(subnetPort, servicecommon.SubnetPortFinalizerName) { - metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypeSubnetPort) - subnetPortID := r.SubnetPortService.BuildSubnetPortId(&subnetPort.ObjectMeta) - if err := r.SubnetPortService.DeleteSubnetPort(subnetPortID); err != nil { - log.Error(err, "deletion failed, would retry exponentially", "subnetport", req.NamespacedName) - deleteFail(r, ctx, subnetPort, &err) - return common.ResultRequeue, err - } - controllerutil.RemoveFinalizer(subnetPort, servicecommon.SubnetPortFinalizerName) - if err := r.Client.Update(ctx, subnetPort); err != nil { - log.Error(err, "deletion failed, would retry exponentially", "subnetport", req.NamespacedName) - deleteFail(r, ctx, subnetPort, &err) - return common.ResultRequeue, err - } - log.Info("removed finalizer", "subnetport", req.NamespacedName) - deleteSuccess(r, ctx, subnetPort) - } else { - log.Info("finalizers cannot be recognized", "subnetport", req.NamespacedName) + metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypeSubnetPort) + subnetPortID := r.SubnetPortService.BuildSubnetPortId(&subnetPort.ObjectMeta) + if err := r.SubnetPortService.DeleteSubnetPortById(subnetPortID); err != nil { + log.Error(err, "deletion failed, would retry exponentially", "SubnetPort", req.NamespacedName) + deleteFail(r, ctx, subnetPort, &err) + return common.ResultRequeue, err } + deleteSuccess(r, ctx, subnetPort) } - return ctrl.Result{}, nil + return common.ResultNormal, nil } func subnetPortNamespaceVMIndexFunc(obj client.Object) []string { @@ -191,6 +179,29 @@ func addressBindingNamespaceVMIndexFunc(obj client.Object) []string { } } +func (r *SubnetPortReconciler) deleteSubnetPortByName(ctx context.Context, ns string, name string) error { + // When deleting SubnetPort by Name and Namespace, skip the SubnetPort belonging to the existed SubnetPort CR + nsxSubnetPorts := r.SubnetPortService.ListSubnetPortByName(ns, name) + + crSubnetPortIDsSet, err := r.SubnetPortService.ListSubnetPortIDsFromCRs(ctx) + if err != nil { + log.Error(err, "failed to list SubnetPort CRs") + return err + } + + for _, nsxSubnetPort := range nsxSubnetPorts { + if crSubnetPortIDsSet.Has(*nsxSubnetPort.Id) { + log.Info("skipping deletion, SubnetPort CR still exists in K8s", "ID", *nsxSubnetPort.Id) + continue + } + if err := r.SubnetPortService.DeleteSubnetPort(nsxSubnetPort); err != nil { + return err + } + } + log.Info("successfully deleted nsxSubnetPort", "namespace", ns, "name", name) + return nil +} + // SetupWithManager sets up the controller with the Manager. func (r *SubnetPortReconciler) SetupWithManager(mgr ctrl.Manager) error { if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &v1alpha1.SubnetPort{}, util.SubnetPortNamespaceVMIndexKey, subnetPortNamespaceVMIndexFunc); err != nil { @@ -201,18 +212,6 @@ func (r *SubnetPortReconciler) SetupWithManager(mgr ctrl.Manager) error { } return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.SubnetPort{}). - WithEventFilter( - predicate.Funcs{ - DeleteFunc: func(e event.DeleteEvent) bool { - // Suppress Delete events to avoid filtering them out in the Reconcile function - switch e.Object.(type) { - case *v1alpha1.AddressBinding: - return true - } - return false - }, - }, - ). WithOptions( controller.Options{ MaxConcurrentReconciles: common.NumReconcile(), @@ -290,24 +289,17 @@ func (r *SubnetPortReconciler) CollectGarbage(ctx context.Context) { if len(nsxSubnetPortSet) == 0 { return } - subnetPortList := &v1alpha1.SubnetPortList{} - err := r.Client.List(ctx, subnetPortList) + + crSubnetPortIDsSet, err := r.SubnetPortService.ListSubnetPortIDsFromCRs(ctx) if err != nil { - log.Error(err, "failed to list SubnetPort CR") return } - CRSubnetPortSet := sets.New[string]() - for _, subnetPort := range subnetPortList.Items { - subnetPortID := r.SubnetPortService.BuildSubnetPortId(&subnetPort.ObjectMeta) - CRSubnetPortSet.Insert(subnetPortID) - } - - diffSet := nsxSubnetPortSet.Difference(CRSubnetPortSet) + diffSet := nsxSubnetPortSet.Difference(crSubnetPortIDsSet) for elem := range diffSet { log.V(1).Info("GC collected SubnetPort CR", "UID", elem) metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypeSubnetPort) - err = r.SubnetPortService.DeleteSubnetPort(elem) + err = r.SubnetPortService.DeleteSubnetPortById(elem) if err != nil { metrics.CounterInc(r.SubnetPortService.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResTypeSubnetPort) } else { @@ -359,7 +351,7 @@ func (r *SubnetPortReconciler) UpdateSubnetPortStatusConditions(ctx context.Cont } } -func (r *SubnetPortReconciler) mergeSubnetPortStatusCondition(ctx context.Context, subnetPort *v1alpha1.SubnetPort, newCondition *v1alpha1.Condition) bool { +func (r *SubnetPortReconciler) mergeSubnetPortStatusCondition(_ context.Context, subnetPort *v1alpha1.SubnetPort, newCondition *v1alpha1.Condition) bool { matchedCondition := getExistingConditionOfType(newCondition.Type, subnetPort.Status.Conditions) if reflect.DeepEqual(matchedCondition, newCondition) { @@ -420,7 +412,7 @@ func (r *SubnetPortReconciler) CheckAndGetSubnetPathForSubnetPort(ctx context.Co _, err = r.SubnetService.GetSubnetByPath(subnetPath) if err != nil { log.Info("previous NSX subnet is deleted, deleting the stale subnet port", "subnetPort.UID", subnetPort.UID, "subnetPath", subnetPath) - if err = r.SubnetPortService.DeleteSubnetPort(subnetPortID); err != nil { + if err = r.SubnetPortService.DeleteSubnetPortById(subnetPortID); err != nil { log.Error(err, "failed to delete the stale subnetport", "subnetport.UID", subnetPort.UID) return } @@ -439,7 +431,7 @@ func (r *SubnetPortReconciler) CheckAndGetSubnetPathForSubnetPort(ctx context.Co log.Error(err, "subnet CR not found", "subnet CR", namespacedName) return } - if subnet != nil && !subnet.DeletionTimestamp.IsZero() { + if !subnet.DeletionTimestamp.IsZero() { isStale = true err := fmt.Errorf("subnet %s is being deleted, cannot operate subnetport %s", namespacedName, subnetPort.Name) return true, "", err @@ -464,7 +456,7 @@ func (r *SubnetPortReconciler) CheckAndGetSubnetPathForSubnetPort(ctx context.Co log.Error(err, "subnetSet CR not found", "subnet CR", namespacedName) return } - if subnetSet != nil && !subnetSet.DeletionTimestamp.IsZero() { + if !subnetSet.DeletionTimestamp.IsZero() { isStale = true err = fmt.Errorf("subnetset %s is being deleted, cannot operate subnetport %s", namespacedName, subnetPort.Name) return diff --git a/pkg/controllers/subnetport/subnetport_controller_test.go b/pkg/controllers/subnetport/subnetport_controller_test.go index ec0dbc55c..3aa0d6940 100644 --- a/pkg/controllers/subnetport/subnetport_controller_test.go +++ b/pkg/controllers/subnetport/subnetport_controller_test.go @@ -9,6 +9,7 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1" @@ -77,41 +78,37 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { }) defer patchesGetSubnetByPath.Reset() - // not found - errNotFound := errors.New("not found") + // fail to get + errFailToGet := errors.New("failed to get CR") + k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errFailToGet) + _, ret := r.Reconcile(ctx, req) + assert.Equal(t, errFailToGet, ret) + + // not found and deletion success + errNotFound := apierrors.NewNotFound(v1alpha1.Resource("subnetport"), "") k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errNotFound) - _, err := r.Reconcile(ctx, req) - assert.Equal(t, err, errNotFound) - // update fails - sp := &v1alpha1.SubnetPort{} - k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( - func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { - v1sp := obj.(*v1alpha1.SubnetPort) - v1sp.Spec.Subnet = "subnet1" + patchesDeleteSubnetPortByName := gomonkey.ApplyFunc((*SubnetPortReconciler).deleteSubnetPortByName, + func(r *SubnetPortReconciler, ctx context.Context, ns string, name string) error { return nil }) - subnet := &v1alpha1.Subnet{} - k8sClient.EXPECT().Get(ctx, gomock.Any(), subnet).Return(nil).AnyTimes().Do( - func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { - s := obj.(*v1alpha1.Subnet) - s.Name = sp.Spec.Subnet - return nil - }) - err = errors.New("Update failed") - k8sClient.EXPECT().Update(ctx, gomock.Any()).Return(err) - patchesSuccess := gomonkey.ApplyFunc(updateSuccess, - func(r *SubnetPortReconciler, c context.Context, o *v1alpha1.SubnetPort) { - }) - defer patchesSuccess.Reset() - patchesUpdateFail := gomonkey.ApplyFunc(updateFail, - func(r *SubnetPortReconciler, c context.Context, o *v1alpha1.SubnetPort, e *error) { + defer patchesDeleteSubnetPortByName.Reset() + _, ret = r.Reconcile(ctx, req) + assert.Equal(t, nil, ret) + + // not found and deletion failed + err := errors.New("Deletion failed") + k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errNotFound) + patchesDeleteSubnetPortByName = gomonkey.ApplyFunc((*SubnetPortReconciler).deleteSubnetPortByName, + func(r *SubnetPortReconciler, ctx context.Context, ns string, name string) error { + return err }) - defer patchesUpdateFail.Reset() - _, ret := r.Reconcile(ctx, req) + defer patchesDeleteSubnetPortByName.Reset() + _, ret = r.Reconcile(ctx, req) assert.Equal(t, err, ret) // both subnet and subnetset are configured + sp := &v1alpha1.SubnetPort{} k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.SubnetPort) @@ -119,6 +116,10 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { v1sp.Spec.SubnetSet = "subnetset2" return nil }) + patchesUpdateFail := gomonkey.ApplyFunc(updateFail, + func(r *SubnetPortReconciler, c context.Context, o *v1alpha1.SubnetPort, e *error) { + }) + defer patchesUpdateFail.Reset() err = errors.New("subnet and subnetset should not be configured at the same time") _, ret = r.Reconcile(ctx, req) assert.Equal(t, err, ret) @@ -135,7 +136,6 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { return requests }) defer patchesVmMapFunc.Reset() - k8sClient.EXPECT().Update(ctx, gomock.Any()).Return(nil) k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.SubnetPort) @@ -155,8 +155,8 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { defer patchesCreateOrUpdateSubnetPort.Reset() _, ret = r.Reconcile(ctx, req) assert.Equal(t, err, ret) + // happy path - k8sClient.EXPECT().Update(ctx, gomock.Any()).Return(nil) k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.SubnetPort) @@ -184,6 +184,10 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { return portState, nil }) defer patchesCreateOrUpdateSubnetPort.Reset() + patchesSuccess := gomonkey.ApplyFunc(updateSuccess, + func(r *SubnetPortReconciler, c context.Context, o *v1alpha1.SubnetPort) { + }) + defer patchesSuccess.Reset() _, ret = r.Reconcile(ctx, req) assert.Equal(t, nil, ret) @@ -194,15 +198,14 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { v1sp.Spec.Subnet = "subnet1" time := metav1.Now() v1sp.ObjectMeta.DeletionTimestamp = &time - v1sp.Finalizers = []string{common.SubnetPortFinalizerName} return nil }) err = errors.New("DeleteSubnetPort failed") - patchesDeleteSubnetPort := gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPort, + patchesDeleteSubnetPortById := gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPortById, func(s *subnetport.SubnetPortService, uid string) error { return err }) - defer patchesDeleteSubnetPort.Reset() + defer patchesDeleteSubnetPortById.Reset() patchesCreateOrUpdateSubnetPort = gomonkey.ApplyFunc((*subnetport.SubnetPortService).CreateOrUpdateSubnetPort, func(s *subnetport.SubnetPortService, obj interface{}, nsxSubnet *model.VpcSubnet, contextID string, tags *map[string]string) (*model.SegmentPortState, error) { assert.FailNow(t, "should not be called") @@ -216,46 +219,7 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { _, ret = r.Reconcile(ctx, req) assert.Equal(t, err, ret) - // handle deletion event - update subnetport failed in deletion event - k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( - func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { - v1sp := obj.(*v1alpha1.SubnetPort) - v1sp.Spec.Subnet = "subnet1" - time := metav1.Now() - v1sp.ObjectMeta.DeletionTimestamp = &time - v1sp.Finalizers = []string{common.SubnetPortFinalizerName} - return nil - }) - err = errors.New("Update failed") - k8sClient.EXPECT().Update(ctx, gomock.Any()).Return(err) - patchesDeleteSubnetPort = gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPort, - func(s *subnetport.SubnetPortService, uid string) error { - return nil - }) - defer patchesDeleteSubnetPort.Reset() - _, ret = r.Reconcile(ctx, req) - assert.Equal(t, err, ret) - // handle deletion event - successfully deleted - k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( - func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { - v1sp := obj.(*v1alpha1.SubnetPort) - v1sp.Spec.Subnet = "subnet1" - time := metav1.Now() - v1sp.ObjectMeta.DeletionTimestamp = &time - v1sp.Finalizers = []string{common.SubnetPortFinalizerName} - return nil - }) - k8sClient.EXPECT().Update(ctx, gomock.Any()).Return(nil) - patchesDeleteSubnetPort = gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPort, - func(s *subnetport.SubnetPortService, uid string) error { - return nil - }) - defer patchesDeleteSubnetPort.Reset() - _, ret = r.Reconcile(ctx, req) - assert.Equal(t, nil, ret) - - // handle deletion event - unknown finalizers k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do( func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.SubnetPort) @@ -264,20 +228,22 @@ func TestSubnetPortReconciler_Reconcile(t *testing.T) { v1sp.ObjectMeta.DeletionTimestamp = &time return nil }) - patchesDeleteSubnetPort = gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPort, + patchesDeleteSubnetPortById = gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPortById, func(s *subnetport.SubnetPortService, uid string) error { - assert.FailNow(t, "should not be called") return nil }) - defer patchesDeleteSubnetPort.Reset() + defer patchesDeleteSubnetPortById.Reset() _, ret = r.Reconcile(ctx, req) assert.Equal(t, nil, ret) } func TestSubnetPortReconciler_GarbageCollector(t *testing.T) { // gc collect item "2345", local store has more item than k8s cache + mockCtl := gomock.NewController(t) + k8sClient := mock_client.NewMockClient(mockCtl) service := &subnetport.SubnetPortService{ Service: common.Service{ + Client: k8sClient, NSXConfig: &config.NSXOperatorConfig{ NsxConfig: &config.NsxConfig{ EnforcementPoint: "vmc-enforcementpoint", @@ -293,13 +259,12 @@ func TestSubnetPortReconciler_GarbageCollector(t *testing.T) { return a }) defer patchesListNSXSubnetPortIDForCR.Reset() - patchesDeleteSubnetPort := gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPort, + patchesDeleteSubnetPortById := gomonkey.ApplyFunc((*subnetport.SubnetPortService).DeleteSubnetPortById, func(s *subnetport.SubnetPortService, uid string) error { return nil }) - defer patchesDeleteSubnetPort.Reset() - mockCtl := gomock.NewController(t) - k8sClient := mock_client.NewMockClient(mockCtl) + defer patchesDeleteSubnetPortById.Reset() + r := &SubnetPortReconciler{ Client: k8sClient, Scheme: nil, diff --git a/pkg/nsx/services/common/types.go b/pkg/nsx/services/common/types.go index 02a559e67..95bc89fa9 100644 --- a/pkg/nsx/services/common/types.go +++ b/pkg/nsx/services/common/types.go @@ -105,9 +105,7 @@ const ( NSXServiceAccountFinalizerName = "nsxserviceaccount.nsx.vmware.com/finalizer" T1SecurityPolicyFinalizerName = "securitypolicy.nsx.vmware.com/finalizer" StaticRouteFinalizerName = "staticroute.crd.nsx.vmware.com/finalizer" - SubnetPortFinalizerName = "subnetport.crd.nsx.vmware.com/finalizer" NetworkInfoFinalizerName = "networkinfo.crd.nsx.vmware.com/finalizer" - PodFinalizerName = "pod.crd.nsx.vmware.com/finalizer" IPPoolFinalizerName = "ippool.crd.nsx.vmware.com/finalizer" IPAddressAllocationFinalizerName = "ipaddressallocation.crd.nsx.vmware.com/finalizer" diff --git a/pkg/nsx/services/subnetport/builder.go b/pkg/nsx/services/subnetport/builder.go index 0c8795f94..7ff5e29ce 100644 --- a/pkg/nsx/services/subnetport/builder.go +++ b/pkg/nsx/services/subnetport/builder.go @@ -51,9 +51,6 @@ func (service *SubnetPortService) buildSubnetPort(obj interface{}, nsxSubnet *mo return nil, err } nsxSubnetPortPath := fmt.Sprintf("%s/ports/%s", *nsxSubnet.Path, nsxSubnetPortID) - if err != nil { - return nil, err - } namespace := &corev1.Namespace{} namespacedName := types.NamespacedName{ Name: objNamespace, diff --git a/pkg/nsx/services/subnetport/store.go b/pkg/nsx/services/subnetport/store.go index 6a01e4d68..c5496a20e 100644 --- a/pkg/nsx/services/subnetport/store.go +++ b/pkg/nsx/services/subnetport/store.go @@ -67,6 +67,24 @@ func subnetPortIndexBySubnetID(obj interface{}) ([]string, error) { } } +func subnetPortIndexNamespace(obj interface{}) ([]string, error) { + switch o := obj.(type) { + case *model.VpcSubnetPort: + return filterTag(o.Tags, common.TagScopeVMNamespace), nil + default: + return nil, errors.New("subnetPortIndexNamespace doesn't support unknown type") + } +} + +func subnetPortIndexPodNamespace(obj interface{}) ([]string, error) { + switch o := obj.(type) { + case *model.VpcSubnetPort: + return filterTag(o.Tags, common.TagScopeNamespace), nil + default: + return nil, errors.New("subnetPortIndexPodNamespace doesn't support unknown type") + } +} + // SubnetPortStore is a store for SubnetPorts type SubnetPortStore struct { common.ResourceStore diff --git a/pkg/nsx/services/subnetport/subnetport.go b/pkg/nsx/services/subnetport/subnetport.go index 36c2d947a..8f0cd7031 100644 --- a/pkg/nsx/services/subnetport/subnetport.go +++ b/pkg/nsx/services/subnetport/subnetport.go @@ -52,6 +52,8 @@ func InitializeSubnetPort(service servicecommon.Service) (*SubnetPortService, er cache.Indexers{ servicecommon.TagScopeSubnetPortCRUID: subnetPortIndexByCRUID, servicecommon.TagScopePodUID: subnetPortIndexByPodUID, + servicecommon.TagScopeVMNamespace: subnetPortIndexNamespace, + servicecommon.TagScopeNamespace: subnetPortIndexPodNamespace, servicecommon.IndexKeySubnetID: subnetPortIndexBySubnetID, }), BindingType: model.VpcSubnetPortBindingType(), @@ -176,7 +178,7 @@ func (service *SubnetPortService) CheckSubnetPortState(obj interface{}, nsxSubne if realizestate.IsRealizeStateError(err) { log.Error(err, "the created subnet port is in error realization state, cleaning the resource", "subnetport", portID) // only recreate subnet port on RealizationErrorStateError. - if err := service.DeleteSubnetPort(portID); err != nil { + if err := service.DeleteSubnetPortById(portID); err != nil { log.Error(err, "cleanup error subnetport failed", "subnetport", portID) return nil, err } @@ -213,26 +215,30 @@ func (service *SubnetPortService) GetSubnetPortState(obj interface{}, nsxSubnetP return &nsxSubnetPortState, nil } -func (service *SubnetPortService) DeleteSubnetPort(portID string) error { - nsxSubnetPort := service.SubnetPortStore.GetByKey(portID) - if nsxSubnetPort == nil || nsxSubnetPort.Id == nil { - log.Info("NSX subnet port is not found in store, skip deleting it", "id", portID) - return nil - } +func (service *SubnetPortService) DeleteSubnetPort(nsxSubnetPort *model.VpcSubnetPort) error { nsxOrgID, nsxProjectID, nsxVPCID, nsxSubnetID := nsxutil.ParseVPCPath(*nsxSubnetPort.Path) - err := service.NSXClient.PortClient.Delete(nsxOrgID, nsxProjectID, nsxVPCID, nsxSubnetID, portID) + err := service.NSXClient.PortClient.Delete(nsxOrgID, nsxProjectID, nsxVPCID, nsxSubnetID, *nsxSubnetPort.Id) err = nsxutil.TransNSXApiError(err) if err != nil { log.Error(err, "failed to delete subnetport", "nsxSubnetPort.Path", *nsxSubnetPort.Path) return err } - if err = service.SubnetPortStore.Delete(portID); err != nil { + if err = service.SubnetPortStore.Delete(*nsxSubnetPort.Id); err != nil { return err } - log.Info("successfully deleted nsxSubnetPort", "nsxSubnetPortID", portID) + log.Info("successfully deleted nsxSubnetPort", "nsxSubnetPortID", *nsxSubnetPort.Id) return nil } +func (service *SubnetPortService) DeleteSubnetPortById(portID string) error { + nsxSubnetPort := service.SubnetPortStore.GetByKey(portID) + if nsxSubnetPort == nil || nsxSubnetPort.Id == nil { + log.Info("NSX subnet port is not found in store, skip deleting it", "id", portID) + return nil + } + return service.DeleteSubnetPort(nsxSubnetPort) +} + func (service *SubnetPortService) ListNSXSubnetPortIDForCR() sets.Set[string] { log.V(2).Info("listing subnet port CR UIDs") subnetPortSet := sets.New[string]() @@ -305,6 +311,46 @@ func (service *SubnetPortService) GetPortsOfSubnet(nsxSubnetID string) (ports [] return subnetPortList } +func (service *SubnetPortService) ListSubnetPortIDsFromCRs(ctx context.Context) (sets.Set[string], error) { + subnetPortList := &v1alpha1.SubnetPortList{} + err := service.Client.List(ctx, subnetPortList) + if err != nil { + log.Error(err, "failed to list SubnetPort CR") + return nil, err + } + + crSubnetPortIDsSet := sets.New[string]() + for _, subnetPort := range subnetPortList.Items { + subnetPortID := service.BuildSubnetPortId(&subnetPort.ObjectMeta) + crSubnetPortIDsSet.Insert(subnetPortID) + } + return crSubnetPortIDsSet, nil +} + +func (service *SubnetPortService) ListSubnetPortByName(ns string, name string) []*model.VpcSubnetPort { + var result []*model.VpcSubnetPort + subnetports := service.SubnetPortStore.GetByIndex(servicecommon.TagScopeVMNamespace, ns) + for _, subnetport := range subnetports { + tagname := nsxutil.FindTag(subnetport.Tags, servicecommon.TagScopeSubnetPortCRName) + if tagname == name { + result = append(result, subnetport) + } + } + return result +} + +func (service *SubnetPortService) ListSubnetPortByPodName(ns string, name string) []*model.VpcSubnetPort { + var result []*model.VpcSubnetPort + subnetports := service.SubnetPortStore.GetByIndex(servicecommon.TagScopeNamespace, ns) + for _, subnetport := range subnetports { + tagname := nsxutil.FindTag(subnetport.Tags, servicecommon.TagScopePodName) + if tagname == name { + result = append(result, subnetport) + } + } + return result +} + func (service *SubnetPortService) Cleanup(ctx context.Context) error { subnetPorts := service.SubnetPortStore.List() log.Info("cleanup subnetports", "count", len(subnetPorts)) @@ -314,7 +360,7 @@ func (service *SubnetPortService) Cleanup(ctx context.Context) error { case <-ctx.Done(): return errors.Join(nsxutil.TimeoutFailed, ctx.Err()) default: - err := service.DeleteSubnetPort(subnetPortID) + err := service.DeleteSubnetPortById(subnetPortID) if err != nil { log.Error(err, "cleanup subnetport failed", "subnetPortID", subnetPortID) return err