Repository: submariner-io/submariner-operator
Pull request #1875 (2022-02-24 03:59:01)

Delete ServiceDiscovery on Submariner cleanup
The `Submariner` controller creates and owns the `ServiceDiscovery` resource when service discovery is enabled so it makes sense for the controller to delete the `ServiceDiscovery` resource when the `Submariner` resource is being deleted.

The specific service discovery cleanup E2E test was removed and its parts integrated into the submariner E2E test.



submariner-bot: 🤖 Created branch: z_pr1875/tpantelis/service_disc_deletion 
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

Patch:
diff --git a/controllers/servicediscovery/cleanup.go b/controllers/servicediscovery/cleanup.go
index 04afb773..a2683683 100644
--- a/controllers/servicediscovery/cleanup.go
+++ b/controllers/servicediscovery/cleanup.go
@@ -81,7 +81,7 @@ func (r *Reconciler) doCleanup(ctx context.Context, instance *operatorv1alpha1.S
 		Log:        log,
 	}
 
-	requeue, err := uninstallInfo.Run(ctx)
+	requeue, _, err := uninstallInfo.Run(ctx)
 	if err != nil {
 		return reconcile.Result{}, err // nolint:wrapcheck // No need to wrap
 	}
diff --git a/controllers/submariner/cleanup.go b/controllers/submariner/cleanup.go
index 6a694e2c..f047b40b 100644
--- a/controllers/submariner/cleanup.go
+++ b/controllers/submariner/cleanup.go
@@ -29,6 +29,7 @@ import (
 	"github.com/submariner-io/submariner-operator/controllers/uninstall"
 	"github.com/submariner-io/submariner-operator/pkg/names"
 	appsv1 "k8s.io/api/apps/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -78,11 +79,15 @@ func (r *Reconciler) runComponentCleanup(ctx context.Context, instance *operator
 		Log:        log,
 	}
 
-	requeue, err := uninstallInfo.Run(ctx)
+	requeue, timedOut, err := uninstallInfo.Run(ctx)
 	if err != nil {
 		return reconcile.Result{}, err // nolint:wrapcheck // No need to wrap
 	}
 
+	if !timedOut && instance.Spec.ServiceDiscoveryEnabled {
+		requeue = r.ensureServiceDiscoveryDeleted(ctx, instance.Namespace) || requeue
+	}
+
 	if requeue {
 		return reconcile.Result{RequeueAfter: time.Millisecond * 100}, nil
 	}
@@ -96,6 +101,21 @@ func (r *Reconciler) removeFinalizer(ctx context.Context, instance *operatorv1al
 		instance, constants.CleanupFinalizer)
 }
 
+func (r *Reconciler) ensureServiceDiscoveryDeleted(ctx context.Context, namespace string) bool {
+	err := r.config.Client.Delete(ctx, newServiceDiscoveryCR(namespace))
+	if apierrors.IsNotFound(err) {
+		return false
+	}
+
+	if err == nil {
+		log.Info("Deleted the ServiceDiscovery resource")
+	} else {
+		log.Error(err, "Error deleting the ServiceDiscovery resource")
+	}
+
+	return true
+}
+
 func newDaemonSet(name, namespace string) *appsv1.DaemonSet {
 	return &appsv1.DaemonSet{
 		ObjectMeta: metav1.ObjectMeta{
diff --git a/controllers/uninstall/uninstall.go b/controllers/uninstall/uninstall.go
index 31a579c4..e4fdd7ba 100644
--- a/controllers/uninstall/uninstall.go
+++ b/controllers/uninstall/uninstall.go
@@ -68,24 +68,24 @@ func (c *Component) isInstalled() bool {
 	return c.CheckInstalled == nil || c.CheckInstalled()
 }
 
-func (i *Info) Run(ctx context.Context) (bool, error) {
+func (i *Info) Run(ctx context.Context) (bool, bool, error) {
 	timedOut := time.Since(i.StartTime) >= ComponentReadyTimeout
 	if timedOut {
 		i.Log.Info("Timed out waiting for components to complete - aborting")
 
 		i.cleanup(ctx)
 
-		return false, nil
+		return false, true, nil
 	}
 
 	requeue, err := i.processComponents(ctx)
 	if requeue || err != nil {
-		return requeue, err
+		return requeue, false, err
 	}
 
 	i.cleanup(ctx)
 
-	return false, nil
+	return false, false, nil
 }
 
 func (i *Info) processComponents(ctx context.Context) (bool, error) {


Test patch:
diff --git a/controllers/submariner/submariner_controller_test.go b/controllers/submariner/submariner_controller_test.go
index 13998de0..cd06598e 100644
--- a/controllers/submariner/submariner_controller_test.go
+++ b/controllers/submariner/submariner_controller_test.go
@@ -33,7 +33,9 @@ import (
 	routeagent "github.com/submariner-io/submariner/pkg/routeagent_driver/constants"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 )
 
 const (
@@ -385,4 +387,35 @@ func testDeletion() {
 			t.awaitNoFinalizer()
 		})
 	})
+
+	Context("and ServiceDiscovery is enabled", func() {
+		BeforeEach(func() {
+			t.submariner.Spec.GlobalCIDR = ""
+			t.submariner.Spec.ServiceDiscoveryEnabled = true
+
+			t.InitClientObjs = append(t.InitClientObjs,
+				&operatorv1.ServiceDiscovery{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: submarinerNamespace,
+						Name:      names.ServiceDiscoveryCrName,
+					},
+				})
+		})
+
+		It("should delete the ServiceDiscovery resource", func() {
+			t.AssertReconcileRequeue()
+
+			t.UpdateDaemonSetToReady(t.assertUninstallGatewayDaemonSet())
+			t.UpdateDaemonSetToReady(t.assertUninstallRouteAgentDaemonSet())
+
+			t.AssertReconcileSuccess()
+
+			serviceDiscovery := &operatorv1.ServiceDiscovery{}
+			err := t.Client.Get(context.TODO(), types.NamespacedName{Name: names.ServiceDiscoveryCrName, Namespace: submarinerNamespace},
+				serviceDiscovery)
+			Expect(errors.IsNotFound(err)).To(BeTrue(), "ServiceDiscovery still exists")
+
+			t.awaitNoFinalizer()
+		})
+	})
 }
diff --git a/test/e2e/cleanup/service_discovery.go b/test/e2e/cleanup/service_discovery.go
deleted file mode 100644
index 504211a5..00000000
--- a/test/e2e/cleanup/service_discovery.go
+++ /dev/null
@@ -1,168 +0,0 @@
-/*
-SPDX-License-Identifier: Apache-2.0
-
-Copyright Contributors to the Submariner project.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package cleanup
-
-import (
-	"context"
-	"fmt"
-
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
-	"github.com/submariner-io/admiral/pkg/resource"
-	"github.com/submariner-io/admiral/pkg/util"
-	"github.com/submariner-io/shipyard/test/e2e/framework"
-	operatorv1alpha1 "github.com/submariner-io/submariner-operator/api/submariner/v1alpha1"
-	"github.com/submariner-io/submariner-operator/controllers/uninstall"
-	operatorclient "github.com/submariner-io/submariner-operator/pkg/client/clientset/versioned"
-	operatorv1alpha1client "github.com/submariner-io/submariner-operator/pkg/client/clientset/versioned/typed/submariner/v1alpha1"
-	"github.com/submariner-io/submariner-operator/pkg/names"
-	"github.com/submariner-io/submariner-operator/pkg/subctl/operator/submarinercr"
-	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-)
-
-var _ = Describe("Service Discovery cleanup", func() {
-	When("the ServiceDiscovery resource is deleted", testServiceDiscoveryCleanup)
-})
-
-func testServiceDiscoveryCleanup() {
-	var (
-		serviceDiscoveryInterface operatorv1alpha1client.ServiceDiscoveryInterface
-		submarinerInterface       operatorv1alpha1client.SubmarinerInterface
-		serviceDiscovery          *operatorv1alpha1.ServiceDiscovery
-		submariner                *operatorv1alpha1.Submariner
-		lhAgentAgentPodMonitor    *podMonitor
-		stopCh                    chan struct{}
-	)
-
-	BeforeEach(func() {
-		stopCh = make(chan struct{})
-
-		operatorClient, err := operatorclient.NewForConfig(framework.RestConfigs[framework.ClusterA])
-		Expect(err).To(Succeed())
-
-		serviceDiscoveryInterface = operatorClient.SubmarinerV1alpha1().ServiceDiscoveries(framework.TestContext.SubmarinerNamespace)
-		submarinerInterface = operatorClient.SubmarinerV1alpha1().Submariners(framework.TestContext.SubmarinerNamespace)
-
-		installed := false
-
-		submariner, err = submarinerInterface.Get(context.TODO(), submarinercr.SubmarinerName, metav1.GetOptions{})
-		if apierrors.IsNotFound(err) {
-			submariner = nil
-
-			serviceDiscovery, err = serviceDiscoveryInterface.Get(context.TODO(), names.ServiceDiscoveryCrName, metav1.GetOptions{})
-			if !apierrors.IsNotFound(err) {
-				Expect(err).To(Succeed())
-				installed = true
-			}
-		} else {
-			Expect(err).To(Succeed())
-			installed = submariner.Spec.ServiceDiscoveryEnabled
-		}
-
-		if !installed {
-			framework.Skipf("ServiceDiscovery is not installed, skipping the test...")
-			return
-		}
-
-		lhAgentAgentPodMonitor = startPodMonitor(names.AppendUninstall(names.ServiceDiscoveryComponent), stopCh)
-	})
-
-	AfterEach(func() {
-		close(stopCh)
-
-		if submariner != nil {
-			Expect(util.Update(context.TODO(), resourceInterfaceForSubmariner(submarinerInterface), submariner,
-				func(existing runtime.Object) (runtime.Object, error) {
-					existing.(*operatorv1alpha1.Submariner).Spec.ServiceDiscoveryEnabled = true
-					return existing, nil
-				})).To(Succeed())
-		} else if serviceDiscovery != nil {
-			serviceDiscovery.ObjectMeta = metav1.ObjectMeta{
-				Name:        serviceDiscovery.Name,
-				Labels:      serviceDiscovery.Labels,
-				Annotations: serviceDiscovery.Annotations,
-			}
-
-			_, err := serviceDiscoveryInterface.Create(context.TODO(), serviceDiscovery, metav1.CreateOptions{})
-			if err != nil {
-				framework.Errorf("Error re-creating ServiceDiscovery: %v", err)
-			} else {
-				By("Re-created ServiceDiscovery resource")
-			}
-		}
-	})
-
-	It("should run an uninstall Deployment and cleanup coredns config", func() {
-		By(fmt.Sprintf("Deleting ServiceDiscovery resource in %q", framework.TestContext.ClusterIDs[framework.ClusterA]))
-
-		if submariner != nil {
-			Expect(util.Update(context.TODO(), resourceInterfaceForSubmariner(submarinerInterface), submariner,
-				func(existing runtime.Object) (runtime.Object, error) {
-					existing.(*operatorv1alpha1.Submariner).Spec.ServiceDiscoveryEnabled = false
-					return existing, nil
-				})).To(Succeed())
-		} else {
-			Expect(serviceDiscoveryInterface.Delete(context.TODO(), names.ServiceDiscoveryCrName, metav1.DeleteOptions{})).To(Succeed())
-		}
-
-		Eventually(func() bool {
-			_, err := serviceDiscoveryInterface.Get(context.TODO(), names.ServiceDiscoveryCrName, metav1.GetOptions{})
-			return apierrors.IsNotFound(err)
-		}, uninstall.ComponentReadyTimeout*2).Should(BeTrue(), "ServiceDiscovery resource not deleted")
-
-		By("ServiceDiscovery resource deleted")
-
-		lhAgentAgentPodMonitor.assertUninstallPodsCompleted()
-
-		_, err := framework.KubeClients[framework.ClusterA].AppsV1().Deployments(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
-			names.AppendUninstall(names.ServiceDiscoveryComponent), metav1.GetOptions{})
-		Expect(apierrors.IsNotFound(err)).To(BeTrue(), fmt.Sprintf("Unexpected Deployment %q found",
-			names.AppendUninstall(names.ServiceDiscoveryComponent)))
-
-		Expect(getCorednsConfigMap().Data["Corefile"]).ToNot(ContainSubstring("lighthouse"))
-	})
-}
-
-func getCorednsConfigMap() *corev1.ConfigMap {
-	configMap, err := framework.KubeClients[framework.ClusterA].CoreV1().ConfigMaps("kube-system").Get(context.TODO(),
-		"coredns", metav1.GetOptions{})
-	Expect(err).To(Succeed())
-
-	return configMap
-}
-
-func resourceInterfaceForSubmariner(client operatorv1alpha1client.SubmarinerInterface) resource.Interface {
-	return &resource.InterfaceFuncs{
-		GetFunc: func(ctx context.Context, name string, options metav1.GetOptions) (runtime.Object, error) {
-			return client.Get(ctx, name, options)
-		},
-		CreateFunc: func(ctx context.Context, obj runtime.Object, options metav1.CreateOptions) (runtime.Object, error) {
-			return client.Create(ctx, obj.(*operatorv1alpha1.Submariner), options)
-		},
-		UpdateFunc: func(ctx context.Context, obj runtime.Object, options metav1.UpdateOptions) (runtime.Object, error) {
-			return client.Update(ctx, obj.(*operatorv1alpha1.Submariner), options)
-		},
-		DeleteFunc: func(ctx context.Context, name string, options metav1.DeleteOptions) error {
-			return client.Delete(ctx, name, options)
-		},
-	}
-}
diff --git a/test/e2e/cleanup/submariner.go b/test/e2e/cleanup/submariner.go
index 724fc9d0..7b604368 100644
--- a/test/e2e/cleanup/submariner.go
+++ b/test/e2e/cleanup/submariner.go
@@ -51,11 +51,13 @@ var _ = Describe("Submariner cleanup", func() {
 func testSubmarinerCleanup() {
 	var (
 		submarinerInterface           operatorv1alpha1client.SubmarinerInterface
+		serviceDiscoveryInterface     operatorv1alpha1client.ServiceDiscoveryInterface
 		submariner                    *operatorv1alpha1.Submariner
 		routeAgentPodMonitor          *podMonitor
 		gatewayPodMonitor             *podMonitor
 		globalnetPodMonitor           *podMonitor
 		networkPluginSyncerPodMonitor *podMonitor
+		lhAgentAgentPodMonitor        *podMonitor
 		brokerRestConfig              *rest.Config
 		stopCh                        chan struct{}
 	)
@@ -69,6 +71,7 @@ func testSubmarinerCleanup() {
 		Expect(err).To(Succeed())
 
 		submarinerInterface = operatorClient.SubmarinerV1alpha1().Submariners(framework.TestContext.SubmarinerNamespace)
+		serviceDiscoveryInterface = operatorClient.SubmarinerV1alpha1().ServiceDiscoveries(framework.TestContext.SubmarinerNamespace)
 
 		submariner, err = submarinerInterface.Get(context.TODO(), submarinercr.SubmarinerName, metav1.GetOptions{})
 		Expect(err).To(Succeed())
@@ -88,6 +91,10 @@ func testSubmarinerCleanup() {
 		if err == nil {
 			networkPluginSyncerPodMonitor = startPodMonitor(names.AppendUninstall(names.NetworkPluginSyncerComponent), stopCh)
 		}
+
+		if submariner.Spec.ServiceDiscoveryEnabled {
+			lhAgentAgentPodMonitor = startPodMonitor(names.AppendUninstall(names.ServiceDiscoveryComponent), stopCh)
+		}
 	})
 
 	AfterEach(func() {
@@ -121,34 +128,42 @@ func testSubmarinerCleanup() {
 
 		By("Submariner resource deleted")
 
+		_, err := serviceDiscoveryInterface.Get(context.TODO(), names.ServiceDiscoveryCrName, metav1.GetOptions{})
+		assertIsNotFound(err, "ServiceDiscovery", names.ServiceDiscoveryCrName)
+
+		lhAgentAgentPodMonitor.assertUninstallPodsCompleted()
 		routeAgentPodMonitor.assertUninstallPodsCompleted()
 		gatewayPodMonitor.assertUninstallPodsCompleted()
 		globalnetPodMonitor.assertUninstallPodsCompleted()
 		networkPluginSyncerPodMonitor.assertUninstallPodsCompleted()
 
-		_, err := framework.KubeClients[framework.ClusterA].AppsV1().DaemonSets(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
+		_, err = framework.KubeClients[framework.ClusterA].AppsV1().DaemonSets(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
 			names.AppendUninstall(names.GatewayComponent), metav1.GetOptions{})
-		Expect(apierrors.IsNotFound(err)).To(BeTrue(), fmt.Sprintf("Unexpected DaemonSet %q found",
-			names.AppendUninstall(names.GatewayComponent)))
+		assertIsNotFound(err, "DaemonSet", names.AppendUninstall(names.GatewayComponent))
 
 		_, err = framework.KubeClients[framework.ClusterA].AppsV1().DaemonSets(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
 			names.AppendUninstall(names.RouteAgentComponent), metav1.GetOptions{})
-		Expect(apierrors.IsNotFound(err)).To(BeTrue(), fmt.Sprintf("Unexpected DaemonSet %q found",
-			names.AppendUninstall(names.RouteAgentComponent)))
+		assertIsNotFound(err, "DaemonSet", names.AppendUninstall(names.RouteAgentComponent))
 
 		_, err = framework.KubeClients[framework.ClusterA].AppsV1().DaemonSets(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
 			names.AppendUninstall(names.GlobalnetComponent), metav1.GetOptions{})
-		Expect(apierrors.IsNotFound(err)).To(BeTrue(), fmt.Sprintf("Unexpected DaemonSet %q found",
-			names.AppendUninstall(names.GlobalnetComponent)))
+		assertIsNotFound(err, "DaemonSet", names.AppendUninstall(names.GlobalnetComponent))
 
 		_, err = framework.KubeClients[framework.ClusterA].AppsV1().Deployments(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
 			names.AppendUninstall(names.NetworkPluginSyncerComponent), metav1.GetOptions{})
-		Expect(apierrors.IsNotFound(err)).To(BeTrue(), fmt.Sprintf("Unexpected Deployment %q found",
-			names.AppendUninstall(names.NetworkPluginSyncerComponent)))
+		assertIsNotFound(err, "Deployment", names.AppendUninstall(names.NetworkPluginSyncerComponent))
+
+		_, err = framework.KubeClients[framework.ClusterA].AppsV1().Deployments(framework.TestContext.SubmarinerNamespace).Get(context.TODO(),
+			names.AppendUninstall(names.ServiceDiscoveryComponent), metav1.GetOptions{})
+		assertIsNotFound(err, "Deployment", names.AppendUninstall(names.ServiceDiscoveryComponent))
 
 		assertNoClusterResources(brokerRestConfig, submariner.Spec.BrokerK8sRemoteNamespace)
 		assertNoEndpointResources(brokerRestConfig, submariner.Spec.BrokerK8sRemoteNamespace)
 		assertNoGlobalnetResources()
+
+		if submariner.Spec.ServiceDiscoveryEnabled {
+			Expect(getCorednsConfigMap().Data["Corefile"]).ToNot(ContainSubstring("lighthouse"))
+		}
 	})
 }
 
@@ -328,3 +343,23 @@ func getBrokerRestConfig(brokerNS string) *rest.Config {
 
 	return nil
 }
+
+func getCorednsConfigMap() *corev1.ConfigMap {
+	configMap, err := framework.KubeClients[framework.ClusterA].CoreV1().ConfigMaps("kube-system").Get(context.TODO(),
+		"coredns", metav1.GetOptions{})
+	Expect(err).To(Succeed())
+
+	return configMap
+}
+
+func assertIsNotFound(err error, typeName, objName string) {
+	if apierrors.IsNotFound(err) {
+		return
+	}
+
+	if err == nil {
+		Fail(fmt.Sprintf("Unexpected %s %q found", typeName, objName))
+	}
+
+	Expect(err).To(Succeed())
+}