Repository: spidernet-io/spiderpool (go)

Issue:
third-party controller unrecognized
**Describe the version**
version about:
1. spiderpool `v0.7.0`

**Describe the bug**
Once I deployed the `redis-operator` and found the `RedisCluster` third-party controller controls kubernetes `StatefulSet`, but the spiderpool regard the `StatefulSet` pod's Top controller is `StatefulSet` and missed `RedisCluster` type.

So the `PodManager.GetTopController` method is not very concise. 

Pull request:
improve GetTopController method
In the SpiderSubnet feature, I noticed a situation that spiderpool can't handle with it well.
The RedisCluster -> StatefulSet -> Pod. If we set SpiderSubnet auto pool annotations for it, the pod would not up successfully. With what #2349  describes.

All in all, the `GetTopController` method need to be improved. So I commit a PR #2347 
But I noticed that in the RedisCluster situation, the pod will lose the StatefulSet static IP feature. So I abandon that PR.

Finally, I made a decision. If a third-party controller controls the pod directly, I'll regard the third-party controller is the top controller no matter any other controllers controls the third-party controller.
And If some third-party controller controls the native kubernetes controller like StatefulSet (RedisCluster situation), I'll regard the StatefulSet is the pod's top controller.

With this, the pod will inherit the StatefulSet pod static IP feature and be able to use SpiderSubnet feature auto pool dynamic IP numbers feature.(In the past, the third-party controller use the auto-pool must in fixed IP number).


Signed-off-by: Icarus9913 [icaruswu66@qq.com](mailto:icaruswu66@qq.com)


**What this PR does / why we need it**:
fix bug

**Which issue(s) this PR fixes**:
close #2349 


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() {