From 71aaeb471cdb89524797c73828c66ffcd56a3288 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 15:22:34 +0800 Subject: [PATCH 1/8] fix some orphan pods cleaner bugs --- cmd/controller-manager/main.go | 19 +++++++++- pkg/controller/backup/backup_controller.go | 4 -- .../backup_schedule_controller.go | 4 -- pkg/controller/restore/restore_controller.go | 4 -- .../tidbcluster/tidb_cluster_controller.go | 5 +-- pkg/manager/member/orphan_pods_cleaner.go | 38 +++++++++++++++++-- .../member/orphan_pods_cleaner_test.go | 10 +++-- 7 files changed, 59 insertions(+), 25 deletions(-) diff --git a/cmd/controller-manager/main.go b/cmd/controller-manager/main.go index d2570d6207..98f8a4cf55 100644 --- a/cmd/controller-manager/main.go +++ b/cmd/controller-manager/main.go @@ -141,8 +141,23 @@ func main() { bsController := backupschedule.NewController(kubeCli, cli, informerFactory, kubeInformerFactory) controllerCtx, cancel := context.WithCancel(context.Background()) defer cancel() - go informerFactory.Start(controllerCtx.Done()) - go kubeInformerFactory.Start(controllerCtx.Done()) + + // Start informer factories after all controller are initializated. + informerFactory.Start(controllerCtx.Done()) + kubeInformerFactory.Start(controllerCtx.Done()) + + // Wait for all started informers' cache were synced. + for v, synced := range informerFactory.WaitForCacheSync(wait.NeverStop) { + if !synced { + glog.Fatalf("error syncing informer for %v", v) + } + } + for v, synced := range kubeInformerFactory.WaitForCacheSync(wait.NeverStop) { + if !synced { + glog.Fatalf("error syncing informer for %v", v) + } + } + glog.Infof("cache of informer factories sync successfully") onStarted := func(ctx context.Context) { go wait.Forever(func() { backupController.Run(workers, ctx.Done()) }, waitDuration) diff --git a/pkg/controller/backup/backup_controller.go b/pkg/controller/backup/backup_controller.go index e100142308..37fd19e542 100644 --- a/pkg/controller/backup/backup_controller.go +++ b/pkg/controller/backup/backup_controller.go @@ -118,10 +118,6 @@ func (bkc *Controller) Run(workers int, stopCh <-chan struct{}) { glog.Info("Starting backup controller") defer glog.Info("Shutting down backup controller") - if !cache.WaitForCacheSync(stopCh, bkc.backupListerSynced) { - return - } - for i := 0; i < workers; i++ { go wait.Until(bkc.worker, time.Second, stopCh) } diff --git a/pkg/controller/backupschedule/backup_schedule_controller.go b/pkg/controller/backupschedule/backup_schedule_controller.go index b7b53414e1..e9a7952762 100644 --- a/pkg/controller/backupschedule/backup_schedule_controller.go +++ b/pkg/controller/backupschedule/backup_schedule_controller.go @@ -114,10 +114,6 @@ func (bsc *Controller) Run(workers int, stopCh <-chan struct{}) { glog.Info("Starting backup schedule controller") defer glog.Info("Shutting down backup schedule controller") - if !cache.WaitForCacheSync(stopCh, bsc.bsListerSynced) { - return - } - for i := 0; i < workers; i++ { go wait.Until(bsc.worker, time.Second, stopCh) } diff --git a/pkg/controller/restore/restore_controller.go b/pkg/controller/restore/restore_controller.go index 084382d4df..cb9fc7891a 100644 --- a/pkg/controller/restore/restore_controller.go +++ b/pkg/controller/restore/restore_controller.go @@ -117,10 +117,6 @@ func (rsc *Controller) Run(workers int, stopCh <-chan struct{}) { glog.Info("Starting restore controller") defer glog.Info("Shutting down restore controller") - if !cache.WaitForCacheSync(stopCh, rsc.restoreListerSynced) { - return - } - for i := 0; i < workers; i++ { go wait.Until(rsc.worker, time.Second, stopCh) } diff --git a/pkg/controller/tidbcluster/tidb_cluster_controller.go b/pkg/controller/tidbcluster/tidb_cluster_controller.go index 3b971b4e0e..8d23430030 100644 --- a/pkg/controller/tidbcluster/tidb_cluster_controller.go +++ b/pkg/controller/tidbcluster/tidb_cluster_controller.go @@ -170,6 +170,7 @@ func NewController( podInformer.Lister(), podControl, pvcInformer.Lister(), + kubeCli, ), recorder, ), @@ -210,10 +211,6 @@ func (tcc *Controller) Run(workers int, stopCh <-chan struct{}) { glog.Info("Starting tidbcluster controller") defer glog.Info("Shutting down tidbcluster controller") - if !cache.WaitForCacheSync(stopCh, tcc.tcListerSynced, tcc.setListerSynced) { - return - } - for i := 0; i < workers; i++ { go wait.Until(tcc.worker, time.Second, stopCh) } diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index fcd70d9678..7c068f2381 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -19,6 +19,8 @@ import ( "github.com/pingcap/tidb-operator/pkg/controller" "github.com/pingcap/tidb-operator/pkg/label" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" corelisters "k8s.io/client-go/listers/core/v1" ) @@ -29,6 +31,18 @@ const ( ) // OrphanPodsCleaner implements the logic for cleaning the orphan pods(has no pvc) +// +// In scaling out and failover, we will try to delete the old PVC to prevent it +// from being used by the new pod. However, the PVC might not be deleted +// immediately in the apiserver because of finalizers (e.g. +// kubernetes.io/pvc-protection) and the statefulset controller may not have +// received PVC delete event when it tries to create the new replica. +// Statefulset controller will create the new pod which will be pending forever +// because no PVC to use. We need to clean these orphan pods and let the +// statefulset controller to create PVC(s) for them. +// +// https://github.com/kubernetes/kubernetes/blob/84fe3db5cf58bf0fc8ff792b885465ceaf70a435/pkg/controller/statefulset/stateful_pod_control.go#L175-L199 +// type OrphanPodsCleaner interface { Clean(*v1alpha1.TidbCluster) (map[string]string, error) } @@ -37,13 +51,15 @@ type orphanPodsCleaner struct { podLister corelisters.PodLister podControl controller.PodControlInterface pvcLister corelisters.PersistentVolumeClaimLister + kubeCli kubernetes.Interface } // NewOrphanPodsCleaner returns a OrphanPodsCleaner func NewOrphanPodsCleaner(podLister corelisters.PodLister, podControl controller.PodControlInterface, - pvcLister corelisters.PersistentVolumeClaimLister) OrphanPodsCleaner { - return &orphanPodsCleaner{podLister, podControl, pvcLister} + pvcLister corelisters.PersistentVolumeClaimLister, + kubeCli kubernetes.Interface) OrphanPodsCleaner { + return &orphanPodsCleaner{podLister, podControl, pvcLister, kubeCli} } func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string, error) { @@ -68,6 +84,7 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string continue } + // TODO support multiple pvcs case? var pvcName string for _, vol := range pod.Spec.Volumes { if vol.PersistentVolumeClaim != nil { @@ -80,7 +97,19 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string continue } - _, err := opc.pvcLister.PersistentVolumeClaims(ns).Get(pvcName) + var err error + // check informer cache + _, err = opc.pvcLister.PersistentVolumeClaims(ns).Get(pvcName) + if err == nil { + skipReason[podName] = skipReasonOrphanPodsCleanerPVCIsFound + continue + } + if !errors.IsNotFound(err) { + return skipReason, err + } + + // check api server + _, err = opc.kubeCli.CoreV1().PersistentVolumeClaims(ns).Get(pvcName, metav1.GetOptions{}) if err == nil { skipReason[podName] = skipReasonOrphanPodsCleanerPVCIsFound continue @@ -89,6 +118,9 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string return skipReason, err } + // if the PVC is not found in apiserver (also informer cache) and the + // phrase of the Pod is Pending, delete it and let the stateful + // controller to create the pod and its PVC(s) again err = opc.podControl.DeletePod(tc, pod) if err != nil { glog.Errorf("orphan pods cleaner: failed to clean orphan pod: %s/%s, %v", ns, podName, err) diff --git a/pkg/manager/member/orphan_pods_cleaner_test.go b/pkg/manager/member/orphan_pods_cleaner_test.go index 7735bc17a5..3022636eb4 100644 --- a/pkg/manager/member/orphan_pods_cleaner_test.go +++ b/pkg/manager/member/orphan_pods_cleaner_test.go @@ -24,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kubeinformers "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" kubefake "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/tools/cache" ) @@ -42,7 +43,7 @@ func TestOrphanPodsCleanerClean(t *testing.T) { testFn := func(test *testcase, t *testing.T) { t.Log(test.name) - opc, podIndexer, pvcIndexer, podControl := newFakeOrphanPodsCleaner() + opc, podIndexer, pvcIndexer, client, podControl := newFakeOrphanPodsCleaner() if test.pods != nil { for _, pod := range test.pods { podIndexer.Add(pod) @@ -50,6 +51,7 @@ func TestOrphanPodsCleanerClean(t *testing.T) { } if test.pvcs != nil { for _, pvc := range test.pvcs { + client.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(pvc) pvcIndexer.Add(pvc) } } @@ -354,13 +356,13 @@ func TestOrphanPodsCleanerClean(t *testing.T) { } } -func newFakeOrphanPodsCleaner() (*orphanPodsCleaner, cache.Indexer, cache.Indexer, *controller.FakePodControl) { +func newFakeOrphanPodsCleaner() (*orphanPodsCleaner, cache.Indexer, cache.Indexer, kubernetes.Interface, *controller.FakePodControl) { kubeCli := kubefake.NewSimpleClientset() kubeInformerFactory := kubeinformers.NewSharedInformerFactory(kubeCli, 0) podInformer := kubeInformerFactory.Core().V1().Pods() pvcInformer := kubeInformerFactory.Core().V1().PersistentVolumeClaims() podControl := controller.NewFakePodControl(podInformer) - return &orphanPodsCleaner{podInformer.Lister(), podControl, pvcInformer.Lister()}, - podInformer.Informer().GetIndexer(), pvcInformer.Informer().GetIndexer(), podControl + return &orphanPodsCleaner{podInformer.Lister(), podControl, pvcInformer.Lister(), kubeCli}, + podInformer.Informer().GetIndexer(), pvcInformer.Informer().GetIndexer(), kubeCli, podControl } From 979117825df3ddd85232fd8a3f1354a67b80cfd3 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 15:54:00 +0800 Subject: [PATCH 2/8] pod pending check --- pkg/manager/member/orphan_pods_cleaner.go | 12 +++-- .../member/orphan_pods_cleaner_test.go | 54 +++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index 7c068f2381..1074e28384 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -18,6 +18,7 @@ import ( "github.com/pingcap/tidb-operator/pkg/apis/pingcap.com/v1alpha1" "github.com/pingcap/tidb-operator/pkg/controller" "github.com/pingcap/tidb-operator/pkg/label" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -25,9 +26,10 @@ import ( ) const ( - skipReasonOrphanPodsCleanerIsNotPDOrTiKV = "orphan pods cleaner: member type is not pd or tikv" - skipReasonOrphanPodsCleanerPVCNameIsEmpty = "orphan pods cleaner: pvcName is empty" - skipReasonOrphanPodsCleanerPVCIsFound = "orphan pods cleaner: pvc is found" + skipReasonOrphanPodsCleanerIsNotPDOrTiKV = "orphan pods cleaner: member type is not pd or tikv" + skipReasonOrphanPodsCleanerPVCNameIsEmpty = "orphan pods cleaner: pvcName is empty" + skipReasonOrphanPodsCleanerPVCIsFound = "orphan pods cleaner: pvc is found" + skipReasonOrphanPodsCleanerPodIsNotPending = "orphan pods cleaner: pod is not pending" ) // OrphanPodsCleaner implements the logic for cleaning the orphan pods(has no pvc) @@ -121,6 +123,10 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string // if the PVC is not found in apiserver (also informer cache) and the // phrase of the Pod is Pending, delete it and let the stateful // controller to create the pod and its PVC(s) again + if pod.Status.Phase != v1.PodPending { + skipReason[podName] = skipReasonOrphanPodsCleanerPodIsNotPending + continue + } err = opc.podControl.DeletePod(tc, pod) if err != nil { glog.Errorf("orphan pods cleaner: failed to clean orphan pod: %s/%s, %v", ns, podName, err) diff --git a/pkg/manager/member/orphan_pods_cleaner_test.go b/pkg/manager/member/orphan_pods_cleaner_test.go index 3022636eb4..ea13437221 100644 --- a/pkg/manager/member/orphan_pods_cleaner_test.go +++ b/pkg/manager/member/orphan_pods_cleaner_test.go @@ -159,6 +159,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, }, pvcs: []*corev1.PersistentVolumeClaim{ @@ -196,6 +199,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, }, pvcs: []*corev1.PersistentVolumeClaim{}, @@ -207,6 +213,39 @@ func TestOrphanPodsCleanerClean(t *testing.T) { g.Expect(strings.Contains(err.Error(), "not found")).To(BeTrue()) }, }, + { + name: "pvc is not found but pod is not pending", + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-1", + Namespace: metav1.NamespaceDefault, + Labels: label.New().Instance(tc.GetLabels()[label.InstanceLabelKey]).PD().Labels(), + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "pd", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-1", + }, + }, + }, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + }, + }, + }, + pvcs: []*corev1.PersistentVolumeClaim{}, + expectFn: func(g *GomegaWithT, skipReason map[string]string, opc *orphanPodsCleaner, err error) { + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(len(skipReason)).To(Equal(1)) + g.Expect(skipReason["pod-1"]).To(Equal(skipReasonOrphanPodsCleanerPodIsNotPending)) + }, + }, { name: "pod delete failed", pods: []*corev1.Pod{ @@ -228,6 +267,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, }, pvcs: []*corev1.PersistentVolumeClaim{}, @@ -259,6 +301,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, { ObjectMeta: metav1.ObjectMeta{ @@ -278,6 +323,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, { ObjectMeta: metav1.ObjectMeta{ @@ -297,6 +345,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, { ObjectMeta: metav1.ObjectMeta{ @@ -316,6 +367,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, }, pvcs: []*corev1.PersistentVolumeClaim{ From e83381450eae8a2655650165479f019a752ea8b8 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 16:01:14 +0800 Subject: [PATCH 3/8] reword --- pkg/manager/member/orphan_pods_cleaner.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index 1074e28384..4e82f96998 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -38,10 +38,10 @@ const ( // from being used by the new pod. However, the PVC might not be deleted // immediately in the apiserver because of finalizers (e.g. // kubernetes.io/pvc-protection) and the statefulset controller may not have -// received PVC delete event when it tries to create the new replica. -// Statefulset controller will create the new pod which will be pending forever -// because no PVC to use. We need to clean these orphan pods and let the -// statefulset controller to create PVC(s) for them. +// received PVC delete event when it tries to create the new replica and the +// new pod will be pending forever because no PVC to use. We need to clean +// these orphan pods and let the statefulset controller to create PVC(s) for +// them. // // https://github.com/kubernetes/kubernetes/blob/84fe3db5cf58bf0fc8ff792b885465ceaf70a435/pkg/controller/statefulset/stateful_pod_control.go#L175-L199 // From 931c65f895831191905297288d4ccdaa506cdb3a Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 16:08:52 +0800 Subject: [PATCH 4/8] fix typo --- cmd/controller-manager/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/controller-manager/main.go b/cmd/controller-manager/main.go index 98f8a4cf55..7558c901cc 100644 --- a/cmd/controller-manager/main.go +++ b/cmd/controller-manager/main.go @@ -142,7 +142,7 @@ func main() { controllerCtx, cancel := context.WithCancel(context.Background()) defer cancel() - // Start informer factories after all controller are initializated. + // Start informer factories after all controller are initialized. informerFactory.Start(controllerCtx.Done()) kubeInformerFactory.Start(controllerCtx.Done()) From 1a352865c2bf31f9747fb7092a4dfc0f3d28eedf Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 16:29:06 +0800 Subject: [PATCH 5/8] fast path --- pkg/manager/member/orphan_pods_cleaner.go | 11 ++++++----- pkg/manager/member/orphan_pods_cleaner_test.go | 6 ++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index 4e82f96998..401064fddc 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -86,6 +86,11 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string continue } + if pod.Status.Phase != v1.PodPending { + skipReason[podName] = skipReasonOrphanPodsCleanerPodIsNotPending + continue + } + // TODO support multiple pvcs case? var pvcName string for _, vol := range pod.Spec.Volumes { @@ -121,12 +126,8 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string } // if the PVC is not found in apiserver (also informer cache) and the - // phrase of the Pod is Pending, delete it and let the stateful + // phase of the Pod is Pending, delete it and let the stateful // controller to create the pod and its PVC(s) again - if pod.Status.Phase != v1.PodPending { - skipReason[podName] = skipReasonOrphanPodsCleanerPodIsNotPending - continue - } err = opc.podControl.DeletePod(tc, pod) if err != nil { glog.Errorf("orphan pods cleaner: failed to clean orphan pod: %s/%s, %v", ns, podName, err) diff --git a/pkg/manager/member/orphan_pods_cleaner_test.go b/pkg/manager/member/orphan_pods_cleaner_test.go index ea13437221..1d06b782ee 100644 --- a/pkg/manager/member/orphan_pods_cleaner_test.go +++ b/pkg/manager/member/orphan_pods_cleaner_test.go @@ -99,6 +99,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { Namespace: metav1.NamespaceDefault, Labels: label.New().Instance(tc.GetLabels()[label.InstanceLabelKey]).PD().Labels(), }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, }, pvcs: nil, @@ -129,6 +132,9 @@ func TestOrphanPodsCleanerClean(t *testing.T) { }, }, }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, }, }, pvcs: nil, From 7a3caf0fd6fe28106ffc5c67afad53bf438f51a1 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 16:30:43 +0800 Subject: [PATCH 6/8] address comments --- pkg/manager/member/orphan_pods_cleaner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index 401064fddc..b18ac735b9 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -115,7 +115,7 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string return skipReason, err } - // check api server + // if PVC not found in cache, re-check from apiserver directly to make sure the PVC really not exist _, err = opc.kubeCli.CoreV1().PersistentVolumeClaims(ns).Get(pvcName, metav1.GetOptions{}) if err == nil { skipReason[podName] = skipReasonOrphanPodsCleanerPVCIsFound From 017bd4e3d0406518fee6cba15b68927e82cde1e2 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 17:49:00 +0800 Subject: [PATCH 7/8] double check pod in apiserver --- pkg/controller/pod_control.go | 5 +- pkg/manager/member/orphan_pods_cleaner.go | 16 +++++ .../member/orphan_pods_cleaner_test.go | 68 +++++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/pkg/controller/pod_control.go b/pkg/controller/pod_control.go index e80f95105a..85294f0b64 100644 --- a/pkg/controller/pod_control.go +++ b/pkg/controller/pod_control.go @@ -23,6 +23,7 @@ import ( "github.com/pingcap/tidb-operator/pkg/label" "github.com/pingcap/tidb-operator/pkg/pdapi" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" coreinformers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -192,7 +193,9 @@ func (rpc *realPodControl) DeletePod(tc *v1alpha1.TidbCluster, pod *corev1.Pod) ns := tc.GetNamespace() tcName := tc.GetName() podName := pod.GetName() - err := rpc.kubeCli.CoreV1().Pods(ns).Delete(podName, nil) + preconditions := metav1.Preconditions{UID: &pod.UID} + deleteOptions := metav1.DeleteOptions{Preconditions: &preconditions} + err := rpc.kubeCli.CoreV1().Pods(ns).Delete(podName, &deleteOptions) if err != nil { glog.Errorf("failed to delete Pod: [%s/%s], TidbCluster: %s, %v", ns, podName, tcName, err) } else { diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index b18ac735b9..a4e16989a6 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -30,6 +30,8 @@ const ( skipReasonOrphanPodsCleanerPVCNameIsEmpty = "orphan pods cleaner: pvcName is empty" skipReasonOrphanPodsCleanerPVCIsFound = "orphan pods cleaner: pvc is found" skipReasonOrphanPodsCleanerPodIsNotPending = "orphan pods cleaner: pod is not pending" + skipReasonOrphanPodsCleanerPodIsNotFound = "orphan pods cleaner: pod does not exist anymore" + skipReasonOrphanPodsCleanerPodChanged = "orphan pods cleaner: pod changed before deletion" ) // OrphanPodsCleaner implements the logic for cleaning the orphan pods(has no pvc) @@ -128,6 +130,20 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string // if the PVC is not found in apiserver (also informer cache) and the // phase of the Pod is Pending, delete it and let the stateful // controller to create the pod and its PVC(s) again + apiPod, err := opc.kubeCli.CoreV1().Pods(pod.Namespace).Get(pod.Name, metav1.GetOptions{}) + if errors.IsNotFound(err) { + skipReason[podName] = skipReasonOrphanPodsCleanerPodIsNotFound + continue + } + if err != nil { + return skipReason, err + } + // try our best to avoid deleting wrong object in apiserver + // TODO upgrade to use deleteOption.Preconditions.ResourceVersion in client-go 1.14+ + if apiPod.UID != pod.UID || apiPod.ResourceVersion != pod.ResourceVersion { + skipReason[podName] = skipReasonOrphanPodsCleanerPodChanged + continue + } err = opc.podControl.DeletePod(tc, pod) if err != nil { glog.Errorf("orphan pods cleaner: failed to clean orphan pod: %s/%s, %v", ns, podName, err) diff --git a/pkg/manager/member/orphan_pods_cleaner_test.go b/pkg/manager/member/orphan_pods_cleaner_test.go index 1d06b782ee..d518237f4d 100644 --- a/pkg/manager/member/orphan_pods_cleaner_test.go +++ b/pkg/manager/member/orphan_pods_cleaner_test.go @@ -36,6 +36,7 @@ func TestOrphanPodsCleanerClean(t *testing.T) { type testcase struct { name string pods []*corev1.Pod + apiPods []*corev1.Pod pvcs []*corev1.PersistentVolumeClaim deletePodFailed bool expectFn func(*GomegaWithT, map[string]string, *orphanPodsCleaner, error) @@ -46,9 +47,15 @@ func TestOrphanPodsCleanerClean(t *testing.T) { opc, podIndexer, pvcIndexer, client, podControl := newFakeOrphanPodsCleaner() if test.pods != nil { for _, pod := range test.pods { + client.CoreV1().Pods(pod.Namespace).Create(pod) podIndexer.Add(pod) } } + if test.apiPods != nil { + for _, pod := range test.apiPods { + client.CoreV1().Pods(pod.Namespace).Update(pod) + } + } if test.pvcs != nil { for _, pvc := range test.pvcs { client.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(pvc) @@ -252,6 +259,67 @@ func TestOrphanPodsCleanerClean(t *testing.T) { g.Expect(skipReason["pod-1"]).To(Equal(skipReasonOrphanPodsCleanerPodIsNotPending)) }, }, + { + name: "pvc is not found but pod changed in apiserver", + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-1", + UID: "pod-1-uid", + ResourceVersion: "1", + Namespace: metav1.NamespaceDefault, + Labels: label.New().Instance(tc.GetLabels()[label.InstanceLabelKey]).PD().Labels(), + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "pd", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-1", + }, + }, + }, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, + }, + }, + apiPods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pod-1", + UID: "pod-1-uid", + ResourceVersion: "2", + Namespace: metav1.NamespaceDefault, + Labels: label.New().Instance(tc.GetLabels()[label.InstanceLabelKey]).PD().Labels(), + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "pd", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-1", + }, + }, + }, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + }, + }, + }, + pvcs: []*corev1.PersistentVolumeClaim{}, + expectFn: func(g *GomegaWithT, skipReason map[string]string, opc *orphanPodsCleaner, err error) { + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(len(skipReason)).To(Equal(1)) + g.Expect(skipReason["pod-1"]).To(Equal(skipReasonOrphanPodsCleanerPodChanged)) + }, + }, { name: "pod delete failed", pods: []*corev1.Pod{ From 45a0c728a6d22a029f35278453d160912cf44e57 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Thu, 5 Sep 2019 18:11:31 +0800 Subject: [PATCH 8/8] address commits --- pkg/manager/member/orphan_pods_cleaner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/manager/member/orphan_pods_cleaner.go b/pkg/manager/member/orphan_pods_cleaner.go index a4e16989a6..25bdf70420 100644 --- a/pkg/manager/member/orphan_pods_cleaner.go +++ b/pkg/manager/member/orphan_pods_cleaner.go @@ -130,7 +130,7 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string // if the PVC is not found in apiserver (also informer cache) and the // phase of the Pod is Pending, delete it and let the stateful // controller to create the pod and its PVC(s) again - apiPod, err := opc.kubeCli.CoreV1().Pods(pod.Namespace).Get(pod.Name, metav1.GetOptions{}) + apiPod, err := opc.kubeCli.CoreV1().Pods(ns).Get(podName, metav1.GetOptions{}) if errors.IsNotFound(err) { skipReason[podName] = skipReasonOrphanPodsCleanerPodIsNotFound continue