From 56a83464312d578ff5ad570f7813cef579a187a4 Mon Sep 17 00:00:00 2001 From: Ido Heyvi Date: Tue, 22 Oct 2024 09:57:57 +0300 Subject: [PATCH] Fixing some more PR comments --- .../cleanup.go | 66 ++++---- .../cleanup_test.go | 152 ++++++++++++++++-- .../main.go | 8 +- .../suite_test.go | 3 +- controllers/sriovoperatorconfig_controller.go | 54 ++++--- .../templates/pre-delete-webooks.yaml | 2 +- 6 files changed, 213 insertions(+), 72 deletions(-) diff --git a/cmd/sriov-network-operator-config-cleanup/cleanup.go b/cmd/sriov-network-operator-config-cleanup/cleanup.go index c6c6995555..93cb04c4e6 100644 --- a/cmd/sriov-network-operator-config-cleanup/cleanup.go +++ b/cmd/sriov-network-operator-config-cleanup/cleanup.go @@ -2,67 +2,75 @@ package main import ( "context" - "os" + "time" snolog "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/log" "github.com/spf13/cobra" - "k8s.io/client-go/dynamic" - - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/log" sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/client/clientset/versioned/typed/sriovnetwork/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "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/watch" ) var ( namespace string + watchTO int ) func init() { rootCmd.Flags().StringVarP(&namespace, "namespace", "n", "", "designated SriovOperatorConfig namespace") -} - -func DynamicClientFor(g schema.GroupVersionKind, obj *unstructured.Unstructured, namespace string) (dynamic.ResourceInterface, error) { - return nil, nil + rootCmd.Flags().IntVarP(&watchTO, "watch-timeout", "w", 10, "sriov-operator config post-delete watch timeout ") } func runCleanupCmd(cmd *cobra.Command, args []string) error { - var ( - config *rest.Config - err error - ) + // init logger snolog.InitLog() setupLog := log.Log.WithName("sriov-network-operator-config-cleanup") - setupLog.Info("Run sriov-network-operator-config-cleanup") - // creates the in-cluster config - kubeconfig := os.Getenv("KUBECONFIG") - if kubeconfig != "" { - config, err = clientcmd.BuildConfigFromFlags("", kubeconfig) - } else { - config, err = rest.InClusterConfig() - } - if err != nil { - setupLog.Error(err, "failed initialization k8s rest config") - } + // adding context timeout although client-go Delete should be non-blocking by default + ctx, timeoutFunc := context.WithTimeout(context.Background(), time.Second*time.Duration(watchTO)) + defer timeoutFunc() - sriovcs, err := sriovnetworkv1.NewForConfig(config) + restConfig := ctrl.GetConfigOrDie() + sriovcs, err := sriovnetworkv1.NewForConfig(restConfig) if err != nil { setupLog.Error(err, "failed to create 'sriovnetworkv1' clientset") } - sriovcs.SriovOperatorConfigs(namespace).Delete(context.Background(), "default", metav1.DeleteOptions{}) + err = sriovcs.SriovOperatorConfigs(namespace).Delete(context.Background(), "default", metav1.DeleteOptions{}) if err != nil { + if errors.IsNotFound(err) { + return nil + } setupLog.Error(err, "failed to delete SriovOperatorConfig") return err } - return nil + // watching 'default' config deletion with context timeout, in case sriov-operator fails to delete 'default' config + watcher, err := sriovcs.SriovOperatorConfigs(namespace).Watch(ctx, metav1.ListOptions{Watch: true}) + defer watcher.Stop() + if err != nil { + setupLog.Error(err, "failed creating 'default' SriovOperatorConfig object watcher") + return err + } + for { + select { + case event := <-watcher.ResultChan(): + if event.Type == watch.Deleted { + setupLog.Info("'default' SriovOperatorConfig is deleted") + return nil + } + + case <-ctx.Done(): + err = ctx.Err() + setupLog.Error(err, "timeout has occured for 'default' SriovOperatorConfig deletion") + return err + } + } } diff --git a/cmd/sriov-network-operator-config-cleanup/cleanup_test.go b/cmd/sriov-network-operator-config-cleanup/cleanup_test.go index 96b4f0ba0f..95c87fda27 100644 --- a/cmd/sriov-network-operator-config-cleanup/cleanup_test.go +++ b/cmd/sriov-network-operator-config-cleanup/cleanup_test.go @@ -2,17 +2,34 @@ package main import ( "context" + "sync" + "github.com/golang/mock/gomock" sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" + "github.com/k8snetworkplumbingwg/sriov-network-operator/controllers" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/featuregate" + mock_platforms "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/mock" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/openshift" "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/manager" ) +type configController struct { + k8sManager manager.Manager + ctx context.Context + cancel context.CancelFunc + wg *sync.WaitGroup +} + var ( + controller *configController testNamespace string = "sriov-network-operator" defaultSriovOperatorSpec = sriovnetworkv1.SriovOperatorConfigSpec{ EnableInjector: true, @@ -24,25 +41,138 @@ var ( var _ = Describe("cleanup", Ordered, func() { - defaultSriovOperatorConfig := &sriovnetworkv1.SriovOperatorConfig{ - ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: testNamespace}, - Spec: defaultSriovOperatorSpec, - } + BeforeAll(func() { + By("Create SriovOperatorConfig controller k8s objs") + config := getDefaultSriovOperatorConfig() + Expect(k8sClient.Create(context.Background(), config)).Should(Succeed()) + + somePolicy := &sriovnetworkv1.SriovNetworkNodePolicy{} + somePolicy.SetNamespace(testNamespace) + somePolicy.SetName("some-policy") + somePolicy.Spec = sriovnetworkv1.SriovNetworkNodePolicySpec{ + NumVfs: 5, + NodeSelector: map[string]string{"foo": "bar"}, + NicSelector: sriovnetworkv1.SriovNetworkNicSelector{}, + Priority: 20, + } + Expect(k8sClient.Create(context.Background(), somePolicy)).ToNot(HaveOccurred()) + DeferCleanup(func() { + err := k8sClient.Delete(context.Background(), somePolicy) + Expect(err).ToNot(HaveOccurred()) + }) + + controller = newConfigController() - BeforeEach(func() { - Expect(k8sClient.Create(context.Background(), defaultSriovOperatorConfig)).NotTo(HaveOccurred()) - err := util.WaitForNamespacedObject(defaultSriovOperatorConfig, k8sClient, testNamespace, "default", util.RetryInterval, util.APITimeout*3) - Expect(err).NotTo(HaveOccurred()) }) It("test webhook cleanup flow", func() { + controller.start() + defer controller.stop() + cmd := &cobra.Command{} namespace = testNamespace + // verify that finalizer has been added, by controller, upon object creation + config := &sriovnetworkv1.SriovOperatorConfig{} + Eventually(func() []string { + // wait for SriovOperatorConfig flags to get updated + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "default", Namespace: testNamespace}, config) + if err != nil { + return nil + } + return config.Finalizers + }, util.APITimeout, util.RetryInterval).Should(Equal([]string{sriovnetworkv1.OPERATORCONFIGFINALIZERNAME})) + Expect(runCleanupCmd(cmd, []string{})).Should(Succeed()) + config = &sriovnetworkv1.SriovOperatorConfig{} + err := util.WaitForNamespacedObjectDeleted(config, k8sClient, testNamespace, "default", util.RetryInterval, util.APITimeout) + Expect(err).NotTo(HaveOccurred()) - config := &sriovnetworkv1.SriovOperatorConfig{} - Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: "default", Namespace: testNamespace}, config)).To(MatchError( - ContainSubstring("sriovoperatorconfigs.sriovnetwork.openshift.io \"default\" not found"))) + }) + It("test 'default' config cleanup timeout", func() { + // in this test case sriov-operator controller has been scaled down. + // we are testing returned ctx timeout error, for not being able to delete 'default' config object + config := getDefaultSriovOperatorConfig() + config.Finalizers = []string{sriovnetworkv1.OPERATORCONFIGFINALIZERNAME} + Expect(k8sClient.Create(context.Background(), config)).Should(Succeed()) + + cmd := &cobra.Command{} + namespace = testNamespace + // verify that finalizer has been added, by controller, upon object creation + config = &sriovnetworkv1.SriovOperatorConfig{} + Eventually(func() []string { + // wait for SriovOperatorConfig flags to get updated + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "default", Namespace: testNamespace}, config) + if err != nil { + return nil + } + return config.Finalizers + }, util.APITimeout, util.RetryInterval).Should(Equal([]string{sriovnetworkv1.OPERATORCONFIGFINALIZERNAME})) + + watchTO = 1 + err := runCleanupCmd(cmd, []string{}) + Expect(err.Error()).To(ContainSubstring("context deadline exceeded")) }) }) + +func getDefaultSriovOperatorConfig() *sriovnetworkv1.SriovOperatorConfig { + return &sriovnetworkv1.SriovOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: testNamespace, + }, + Spec: defaultSriovOperatorSpec, + } +} + +func newConfigController() *configController { + // setup controller manager + By("Setup controller manager") + k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + }) + Expect(err).ToNot(HaveOccurred()) + + t := GinkgoT() + mockCtrl := gomock.NewController(t) + platformHelper := mock_platforms.NewMockInterface(mockCtrl) + platformHelper.EXPECT().GetFlavor().Return(openshift.OpenshiftFlavorDefault).AnyTimes() + platformHelper.EXPECT().IsOpenshiftCluster().Return(false).AnyTimes() + platformHelper.EXPECT().IsHypershift().Return(false).AnyTimes() + + err = (&controllers.SriovOperatorConfigReconciler{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + PlatformHelper: platformHelper, + FeatureGate: featuregate.New(), + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) + + ctx, cancel := context.WithCancel(context.Background()) + wg := sync.WaitGroup{} + controller = &configController{ + k8sManager: k8sManager, + ctx: ctx, + cancel: cancel, + wg: &wg, + } + + return controller +} + +func (c *configController) start() { + + c.wg.Add(1) + go func() { + defer c.wg.Done() + defer GinkgoRecover() + By("Start controller manager") + err := c.k8sManager.Start(c.ctx) + Expect(err).ToNot(HaveOccurred()) + }() +} + +func (c *configController) stop() { + c.cancel() + c.wg.Wait() +} diff --git a/cmd/sriov-network-operator-config-cleanup/main.go b/cmd/sriov-network-operator-config-cleanup/main.go index 8988da17b7..867313c2c0 100644 --- a/cmd/sriov-network-operator-config-cleanup/main.go +++ b/cmd/sriov-network-operator-config-cleanup/main.go @@ -4,11 +4,10 @@ import ( "flag" "os" + snolog "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/log" "github.com/spf13/cobra" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/log" - - snolog "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/log" ) const ( @@ -26,13 +25,12 @@ Example: sriov-network-operator-config-cleanup -n `, } ) -func init() { +func main() { + klog.InitFlags(nil) snolog.BindFlags(flag.CommandLine) rootCmd.PersistentFlags().AddGoFlagSet(flag.CommandLine) -} -func main() { if err := rootCmd.Execute(); err != nil { log.Log.Error(err, "Error executing", componentName) os.Exit(1) diff --git a/cmd/sriov-network-operator-config-cleanup/suite_test.go b/cmd/sriov-network-operator-config-cleanup/suite_test.go index 9040ef49df..ee1815ff70 100644 --- a/cmd/sriov-network-operator-config-cleanup/suite_test.go +++ b/cmd/sriov-network-operator-config-cleanup/suite_test.go @@ -45,6 +45,7 @@ var _ = BeforeSuite(func() { // Go to project root directory err := os.Chdir("../..") + Expect(err).NotTo(HaveOccurred()) By("bootstrapping test environment") testEnv = &envtest.Environment{ @@ -58,9 +59,7 @@ var _ = BeforeSuite(func() { Expect(cfg).NotTo(BeNil()) apiserverDir := testEnv.ControlPlane.GetAPIServer().CertDir - println("aprserver path:", apiserverDir) kubecfgPath = findKubecfg(apiserverDir, ".kubecfg") - println("got kubecfg:", kubecfgPath) err = os.Setenv("KUBECONFIG", kubecfgPath) Expect(err).NotTo(HaveOccurred()) diff --git a/controllers/sriovoperatorconfig_controller.go b/controllers/sriovoperatorconfig_controller.go index 2cce637f34..c8ef0ccd58 100644 --- a/controllers/sriovoperatorconfig_controller.go +++ b/controllers/sriovoperatorconfig_controller.go @@ -40,6 +40,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/go-logr/logr" machinev1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" @@ -83,8 +84,6 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { if apierrors.IsNotFound(err) { logger.Info("default SriovOperatorConfig object not found. waiting for creation.") - - err := r.deleteAllWebhooks(ctx) return reconcile.Result{}, err } // Error reading the object - requeue the request. @@ -95,8 +94,10 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl. snolog.SetLogLevel(defaultConfig.Spec.LogLevel) // examine DeletionTimestamp to determine if object is under deletion - if defaultConfig.ObjectMeta.DeletionTimestamp.IsZero() { - + if !defaultConfig.ObjectMeta.DeletionTimestamp.IsZero() { + // The object is being deleted + return r.handleSriovOperatorConfigDeletion(ctx, defaultConfig, logger) + } else { if !sriovnetworkv1.StringInArray(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) { defaultConfig.ObjectMeta.Finalizers = append(defaultConfig.ObjectMeta.Finalizers, sriovnetworkv1.OPERATORCONFIGFINALIZERNAME) if err := r.Update(ctx, defaultConfig); err != nil { @@ -157,26 +158,6 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl. return reconcile.Result{}, err } } - } else { - // The object is being deleted - if sriovnetworkv1.StringInArray(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) { - // our finalizer is present, so lets handle any external dependency - logger.Info("delete SriovOperatorConfig CR", "Namespace", defaultConfig.Namespace, "Name", defaultConfig.Name) - // make sure webhooks objects are deleted prior of removing finalizer - err := r.deleteAllWebhooks(ctx) - if err != nil { - return reconcile.Result{}, err - } - // remove our finalizer from the list and update it. - var found bool - defaultConfig.ObjectMeta.Finalizers, found = sriovnetworkv1.RemoveString(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) - if found { - if err := r.Update(ctx, defaultConfig); err != nil { - return reconcile.Result{}, err - } - } - } - return reconcile.Result{}, err } logger.Info("Reconcile SriovOperatorConfig completed successfully") @@ -465,6 +446,31 @@ func (r *SriovOperatorConfigReconciler) syncOpenShiftSystemdService(ctx context. return r.setLabelInsideObject(ctx, cr, objs) } +func (r *SriovOperatorConfigReconciler) handleSriovOperatorConfigDeletion(ctx context.Context, + defaultConfig *sriovnetworkv1.SriovOperatorConfig, logger logr.Logger) (ctrl.Result, error) { + + var err error + if sriovnetworkv1.StringInArray(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) { + // our finalizer is present, so lets handle any external dependency + logger.Info("delete SriovOperatorConfig CR", "Namespace", defaultConfig.Namespace, "Name", defaultConfig.Name) + // make sure webhooks objects are deleted prior of removing finalizer + err = r.deleteAllWebhooks(ctx) + if err != nil { + return reconcile.Result{}, err + } + // remove our finalizer from the list and update it. + var found bool + defaultConfig.ObjectMeta.Finalizers, found = sriovnetworkv1.RemoveString(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) + if found { + if err := r.Update(ctx, defaultConfig); err != nil { + return reconcile.Result{}, err + } + } + } + + return reconcile.Result{}, err +} + func (r SriovOperatorConfigReconciler) setLabelInsideObject(ctx context.Context, cr *sriovnetworkv1.SriovOperatorConfig, objs []*uns.Unstructured) error { logger := log.Log.WithName("setLabelInsideObject") for _, obj := range objs { diff --git a/deployment/sriov-network-operator-chart/templates/pre-delete-webooks.yaml b/deployment/sriov-network-operator-chart/templates/pre-delete-webooks.yaml index 02cdd0151c..6eed0f267b 100644 --- a/deployment/sriov-network-operator-chart/templates/pre-delete-webooks.yaml +++ b/deployment/sriov-network-operator-chart/templates/pre-delete-webooks.yaml @@ -22,4 +22,4 @@ spec: args: - --namespace - {{ .Release.Namespace }} - restartPolicy: Never \ No newline at end of file + restartPolicy: Never