Repository: spidernet-io/spiderpool
Pull request #2380 (2023-10-07 03:27:04)

improve GetTopController method
robot cherry pick pr <https://github.com/spidernet-io/spiderpool/pull/2370> to branch release-v0.7,  action <https://github.com/spidernet-io/spiderpool/actions/runs/6438758893>  , commits 9cf02c49f1fd08291ea7718f6c0b64ac369d0399 


Patch:
diff --git a/pkg/applicationcontroller/app_controller.go b/pkg/applicationcontroller/app_controller.go
index 30514457e..d5fcd4954 100644
--- a/pkg/applicationcontroller/app_controller.go
+++ b/pkg/applicationcontroller/app_controller.go
@@ -228,13 +228,6 @@ func (sac *SubnetAppController) controllerAddOrUpdateHandler() applicationinform
 				return nil
 			}
 
-			// check the app whether is the top controller or not
-			owner := metav1.GetControllerOf(newObject)
-			if owner != nil {
-				log.Sugar().Debugf("app has a owner '%s/%s', we would not create or scale IPPool for it", owner.Kind, owner.Name)
-				return nil
-			}
-
 			newAppReplicas = applicationinformers.GetAppReplicas(newObject.Spec.Replicas)
 			newSubnetConfig, err = applicationinformers.GetSubnetAnnoConfig(newObject.Spec.Template.Annotations, log)
 			if nil != err {
@@ -270,7 +263,7 @@ func (sac *SubnetAppController) controllerAddOrUpdateHandler() applicationinform
 
 			// check the app whether is the top controller or not
 			owner := metav1.GetControllerOf(newObject)
-			if owner != nil {
+			if owner != nil && owner.APIVersion == appsv1.SchemeGroupVersion.String() && owner.Kind == constant.KindDeployment {
 				log.Sugar().Debugf("app has a owner '%s/%s', we would not create or scale IPPool for it", owner.Kind, owner.Name)
 				return nil
 			}
@@ -308,13 +301,6 @@ func (sac *SubnetAppController) controllerAddOrUpdateHandler() applicationinform
 				return nil
 			}
 
-			// check the app whether is the top controller or not
-			owner := metav1.GetControllerOf(newObject)
-			if owner != nil {
-				log.Sugar().Debugf("app has a owner '%s/%s', we would not create or scale IPPool for it", owner.Kind, owner.Name)
-				return nil
-			}
-
 			newAppReplicas = applicationinformers.GetAppReplicas(newObject.Spec.Replicas)
 			newSubnetConfig, err = applicationinformers.GetSubnetAnnoConfig(newObject.Spec.Template.Annotations, log)
 			if nil != err {
@@ -350,7 +336,7 @@ func (sac *SubnetAppController) controllerAddOrUpdateHandler() applicationinform
 
 			// check the app whether is the top controller or not
 			owner := metav1.GetControllerOf(newObject)
-			if owner != nil {
+			if owner != nil && owner.APIVersion == batchv1.SchemeGroupVersion.String() && owner.Kind == constant.KindCronJob {
 				log.Sugar().Debugf("app has a owner '%s/%s', we would not create or scale IPPool for it", owner.Kind, owner.Name)
 				return nil
 			}
@@ -388,13 +374,6 @@ func (sac *SubnetAppController) controllerAddOrUpdateHandler() applicationinform
 				return nil
 			}
 
-			// check the app whether is the top controller or not
-			owner := metav1.GetControllerOf(newObject)
-			if owner != nil {
-				log.Sugar().Debugf("app has a owner '%s/%s', we would not create or scale IPPool for it", owner.Kind, owner.Name)
-				return nil
-			}
-
 			newAppReplicas = applicationinformers.CalculateJobPodNum(newObject.Spec.JobTemplate.Spec.Parallelism, newObject.Spec.JobTemplate.Spec.Completions)
 			newSubnetConfig, err = applicationinformers.GetSubnetAnnoConfig(newObject.Spec.JobTemplate.Spec.Template.Annotations, log)
 			if nil != err {
@@ -428,13 +407,6 @@ func (sac *SubnetAppController) controllerAddOrUpdateHandler() applicationinform
 				return nil
 			}
 
-			// check the app whether is the top controller or not
-			owner := metav1.GetControllerOf(newObject)
-			if owner != nil {
-				log.Sugar().Debugf("app has a owner '%s/%s', we would not create or scale IPPool for it", owner.Kind, owner.Name)
-				return nil
-			}
-
 			newAppReplicas = int(newObject.Status.DesiredNumberScheduled)
 			newSubnetConfig, err = applicationinformers.GetSubnetAnnoConfig(newObject.Spec.Template.Annotations, log)
 			if nil != err {
diff --git a/pkg/podmanager/pod_manager.go b/pkg/podmanager/pod_manager.go
index cf2b7aeba..016e864b3 100644
--- a/pkg/podmanager/pod_manager.go
+++ b/pkg/podmanager/pod_manager.go
@@ -124,37 +124,25 @@ func (pm *podManager) GetPodTopController(ctx context.Context, pod *corev1.Pod)
 		}
 
 		replicasetOwner := metav1.GetControllerOf(&replicaset)
-		if replicasetOwner != nil {
-			if replicasetOwner.APIVersion == appsv1.SchemeGroupVersion.String() && replicasetOwner.Kind == constant.KindDeployment {
-				var deployment appsv1.Deployment
-				err = pm.client.Get(ctx, apitypes.NamespacedName{Namespace: replicaset.Namespace, Name: replicasetOwner.Name}, &deployment)
-				if nil != err {
-					return types.PodTopController{}, fmt.Errorf("%w: %v", ownerErr, err)
-				}
-				return types.PodTopController{
-					AppNamespacedName: types.AppNamespacedName{
-						// deployment.APIVersion is empty string
-						APIVersion: appsv1.SchemeGroupVersion.String(),
-						Kind:       constant.KindDeployment,
-						Namespace:  deployment.Namespace,
-						Name:       deployment.Name,
-					},
-					UID: deployment.UID,
-					APP: &deployment,
-				}, nil
+		if replicasetOwner != nil && replicasetOwner.APIVersion == appsv1.SchemeGroupVersion.String() && replicasetOwner.Kind == constant.KindDeployment {
+			var deployment appsv1.Deployment
+			err = pm.client.Get(ctx, apitypes.NamespacedName{Namespace: replicaset.Namespace, Name: replicasetOwner.Name}, &deployment)
+			if nil != err {
+				return types.PodTopController{}, fmt.Errorf("%w: %v", ownerErr, err)
 			}
-
-			logger.Sugar().Warnf("the controller type '%s' of pod '%s/%s' is unknown", replicasetOwner.Kind, pod.Namespace, pod.Name)
 			return types.PodTopController{
 				AppNamespacedName: types.AppNamespacedName{
-					APIVersion: replicasetOwner.APIVersion,
-					Kind:       replicasetOwner.Kind,
-					Namespace:  pod.Namespace,
-					Name:       replicasetOwner.Name,
+					// deployment.APIVersion is empty string
+					APIVersion: appsv1.SchemeGroupVersion.String(),
+					Kind:       constant.KindDeployment,
+					Namespace:  deployment.Namespace,
+					Name:       deployment.Name,
 				},
-				UID: replicasetOwner.UID,
+				UID: deployment.UID,
+				APP: &deployment,
 			}, nil
 		}
+
 		return types.PodTopController{
 			AppNamespacedName: types.AppNamespacedName{
 				APIVersion: appsv1.SchemeGroupVersion.String(),
@@ -173,36 +161,24 @@ func (pm *podManager) GetPodTopController(ctx context.Context, pod *corev1.Pod)
 			return types.PodTopController{}, fmt.Errorf("%w: %v", ownerErr, err)
 		}
 		jobOwner := metav1.GetControllerOf(&job)
-		if jobOwner != nil {
-			if jobOwner.APIVersion == batchv1.SchemeGroupVersion.String() && jobOwner.Kind == constant.KindCronJob {
-				var cronJob batchv1.CronJob
-				err = pm.client.Get(ctx, apitypes.NamespacedName{Namespace: job.Namespace, Name: jobOwner.Name}, &cronJob)
-				if nil != err {
-					return types.PodTopController{}, fmt.Errorf("%w: %v", ownerErr, err)
-				}
-				return types.PodTopController{
-					AppNamespacedName: types.AppNamespacedName{
-						APIVersion: batchv1.SchemeGroupVersion.String(),
-						Kind:       constant.KindCronJob,
-						Namespace:  cronJob.Namespace,
-						Name:       cronJob.Name,
-					},
-					UID: cronJob.UID,
-					APP: &cronJob,
-				}, nil
+		if jobOwner != nil && jobOwner.APIVersion == batchv1.SchemeGroupVersion.String() && jobOwner.Kind == constant.KindCronJob {
+			var cronJob batchv1.CronJob
+			err = pm.client.Get(ctx, apitypes.NamespacedName{Namespace: job.Namespace, Name: jobOwner.Name}, &cronJob)
+			if nil != err {
+				return types.PodTopController{}, fmt.Errorf("%w: %v", ownerErr, err)
 			}
-
-			logger.Sugar().Warnf("the controller type '%s' of pod '%s/%s' is unknown", jobOwner.Kind, pod.Namespace, pod.Name)
 			return types.PodTopController{
 				AppNamespacedName: types.AppNamespacedName{
-					APIVersion: jobOwner.APIVersion,
-					Kind:       jobOwner.Kind,
-					Namespace:  job.Namespace,
-					Name:       jobOwner.Name,
+					APIVersion: batchv1.SchemeGroupVersion.String(),
+					Kind:       constant.KindCronJob,
+					Namespace:  cronJob.Namespace,
+					Name:       cronJob.Name,
 				},
-				UID: jobOwner.UID,
+				UID: cronJob.UID,
+				APP: &cronJob,
 			}, nil
 		}
+
 		return types.PodTopController{
 			AppNamespacedName: types.AppNamespacedName{
 				APIVersion: batchv1.SchemeGroupVersion.String(),


Test patch:
diff --git a/pkg/applicationcontroller/app_controller_test.go b/pkg/applicationcontroller/app_controller_test.go
index ae2ae0bcb..2136e2271 100644
--- a/pkg/applicationcontroller/app_controller_test.go
+++ b/pkg/applicationcontroller/app_controller_test.go
@@ -560,12 +560,19 @@ var _ = Describe("AppController", Label("app_controller_test"), func() {
 				Expect(err).NotTo(HaveOccurred())
 			})
 
-			It("the replicaSet has owner", func() {
+			It("the replicaSet has third-party controller owner", func() {
 				err := controllerutil.SetControllerReference(cloneSet, replicaSet1, scheme)
 				Expect(err).NotTo(HaveOccurred())
 				err = reconcileFunc(ctx, nil, replicaSet1)
 				Expect(err).NotTo(HaveOccurred())
 			})
+
+			It("the replicaSet has deployment controller owner", func() {
+				err := controllerutil.SetControllerReference(deployment1, replicaSet1, scheme)
+				Expect(err).NotTo(HaveOccurred())
+				err = reconcileFunc(ctx, nil, replicaSet1)
+				Expect(err).NotTo(HaveOccurred())
+			})
 		})
 
 		Context("daemonset", func() {
@@ -680,12 +687,19 @@ var _ = Describe("AppController", Label("app_controller_test"), func() {
 				Expect(err).NotTo(HaveOccurred())
 			})
 
-			It("the job has owner", func() {
+			It("the job has third-party controller owner", func() {
 				err := controllerutil.SetControllerReference(cloneSet, job1, scheme)
 				Expect(err).NotTo(HaveOccurred())
 				err = reconcileFunc(ctx, nil, job1)
 				Expect(err).NotTo(HaveOccurred())
 			})
+
+			It("the job has cronJob controller owner", func() {
+				err := controllerutil.SetControllerReference(cronJob1, job1, scheme)
+				Expect(err).NotTo(HaveOccurred())
+				err = reconcileFunc(ctx, nil, job1)
+				Expect(err).NotTo(HaveOccurred())
+			})
 		})
 
 		Context("cronJob", func() {
@@ -728,6 +742,12 @@ var _ = Describe("AppController", Label("app_controller_test"), func() {
 			})
 		})
 
+		Context("unrecognized controller", func() {
+			It("do not support third-party controller", func() {
+				err := reconcileFunc(ctx, nil, cloneSet)
+				Expect(err).To(HaveOccurred())
+			})
+		})
 	})
 
 	Describe("test application delete event hook handler", func() {
@@ -828,6 +848,12 @@ var _ = Describe("AppController", Label("app_controller_test"), func() {
 			})
 		})
 
+		Context("unrecognized controller", func() {
+			It("do not support third-party controller", func() {
+				err := cleanupFunc(ctx, cloneSet)
+				Expect(err).To(HaveOccurred())
+			})
+		})
 	})
 
 	Describe("test deleteAutoPools", func() {
diff --git a/pkg/applicationcontroller/applicationcontroller_suite_test.go b/pkg/applicationcontroller/applicationcontroller_suite_test.go
index c2b51256e..aca6ce3a3 100644
--- a/pkg/applicationcontroller/applicationcontroller_suite_test.go
+++ b/pkg/applicationcontroller/applicationcontroller_suite_test.go
@@ -12,6 +12,7 @@ import (
 
 	kruiseapi "github.com/openkruise/kruise-api"
 	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -40,7 +41,9 @@ type subnetApplicationController struct {
 
 var _ = BeforeSuite(func() {
 	scheme = runtime.NewScheme()
-	err := spiderpoolv2beta1.AddToScheme(scheme)
+	err := clientgoscheme.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+	err = spiderpoolv2beta1.AddToScheme(scheme)
 	Expect(err).NotTo(HaveOccurred())
 	err = kruiseapi.AddToScheme(scheme)
 	Expect(err).NotTo(HaveOccurred())
diff --git a/pkg/podmanager/pod_manager_test.go b/pkg/podmanager/pod_manager_test.go
index 437902e82..46d3f75a1 100644
--- a/pkg/podmanager/pod_manager_test.go
+++ b/pkg/podmanager/pod_manager_test.go
@@ -391,7 +391,7 @@ var _ = Describe("PodManager", Label("pod_manager_test"), func() {
 
 				podTopController, err := podManager.GetPodTopController(ctx, podT)
 				Expect(err).NotTo(HaveOccurred())
-				Expect(slices.Contains(constant.K8sKinds, podTopController.Kind)).To(BeFalse())
+				Expect(podTopController.Kind).Should(Equal(constant.KindReplicaSet))
 			})
 
 			It("Pod with Job controller", func() {
@@ -464,7 +464,7 @@ var _ = Describe("PodManager", Label("pod_manager_test"), func() {
 
 				podTopController, err := podManager.GetPodTopController(ctx, podT)
 				Expect(err).NotTo(HaveOccurred())
-				Expect(slices.Contains(constant.K8sKinds, podTopController.Kind)).To(BeFalse())
+				Expect(podTopController.Kind).Should(Equal(constant.KindJob))
 			})
 
 			It("Pod with CronJob controller", func() {