Merge pull request #181 from adrienjt/delete-proxy-pod-when-chaperon-…

…deleted

delete proxy pod when chaperon deleted

.github/workflows/go.yml

@@ -90,5 +90,5 @@ jobs:
         if: ${{ failure() }}
         uses: actions/upload-artifact@v2
         with:
-          name: cluster-dump
+          name: cluster-dump-${{ matrix.k8s_version }}
           path: cluster-dump

pkg/controllers/feedback/controller.go

@@ -167,47 +167,53 @@ func (c *reconciler) Handle(obj interface{}) (requeueAfter *time.Duration, err e
 		}
 	}
 
-	if candidate != nil && virtualNodeName == c.target.VirtualNodeName {
-		delegate := candidate
+	if virtualNodeName == c.target.VirtualNodeName {
+		if candidate != nil {
+			delegate := candidate
 
-		mcProxyPodAnnotations, otherProxyPodAnnotations := common.SplitLabelsOrAnnotations(proxyPod.Annotations)
-		_, otherDelegatePodAnnotations := common.SplitLabelsOrAnnotations(delegate.Annotations)
+			mcProxyPodAnnotations, otherProxyPodAnnotations := common.SplitLabelsOrAnnotations(proxyPod.Annotations)
+			_, otherDelegatePodAnnotations := common.SplitLabelsOrAnnotations(delegate.Annotations)
 
-		needUpdate := !reflect.DeepEqual(otherProxyPodAnnotations, otherDelegatePodAnnotations)
-		if needUpdate {
-			for k, v := range otherDelegatePodAnnotations {
-				mcProxyPodAnnotations[k] = v
-			}
-			podCopy := proxyPod.DeepCopy()
-			podCopy.Annotations = mcProxyPodAnnotations
+			needUpdate := !reflect.DeepEqual(otherProxyPodAnnotations, otherDelegatePodAnnotations)
+			if needUpdate {
+				for k, v := range otherDelegatePodAnnotations {
+					mcProxyPodAnnotations[k] = v
+				}
+				podCopy := proxyPod.DeepCopy()
+				podCopy.Annotations = mcProxyPodAnnotations
 
-			var err error
-			if proxyPod, err = c.kubeclientset.CoreV1().Pods(namespace).Update(ctx, podCopy, metav1.UpdateOptions{}); err != nil {
-				return nil, err
+				var err error
+				if proxyPod, err = c.kubeclientset.CoreV1().Pods(namespace).Update(ctx, podCopy, metav1.UpdateOptions{}); err != nil {
+					return nil, err
+				}
 			}
-		}
 
-		// we can't group annotation and status updates into an update,
-		// because general update ignores status
+			// we can't group annotation and status updates into an update,
+			// because general update ignores status
 
-		needStatusUpdate := deep.Equal(proxyPod.Status, delegate.Status) != nil
-		if needStatusUpdate {
-			podCopy := proxyPod.DeepCopy()
-			podCopy.Status = delegate.Status
+			needStatusUpdate := deep.Equal(proxyPod.Status, delegate.Status) != nil
+			if needStatusUpdate {
+				podCopy := proxyPod.DeepCopy()
+				podCopy.Status = delegate.Status
 
-			var err error
-			if proxyPod, err = c.kubeclientset.CoreV1().Pods(namespace).UpdateStatus(ctx, podCopy, metav1.UpdateOptions{}); err != nil {
-				return nil, err
+				var err error
+				if proxyPod, err = c.kubeclientset.CoreV1().Pods(namespace).UpdateStatus(ctx, podCopy, metav1.UpdateOptions{}); err != nil {
+					return nil, err
+				}
 			}
-		}
 
-		needRemoteUpdate := delegate.Labels[common.LabelKeyParentClusterName] != c.clusterName
-		if needRemoteUpdate {
-			delegateCopy := delegate.DeepCopy()
-			delegateCopy.Labels[common.LabelKeyParentClusterName] = c.clusterName
-			var err error
-			if delegate, err = c.customclientset.MulticlusterV1alpha1().PodChaperons(namespace).Update(ctx, delegateCopy, metav1.UpdateOptions{}); err != nil {
-				return nil, fmt.Errorf("cannot update candidate pod chaperon")
+			needRemoteUpdate := delegate.Labels[common.LabelKeyParentClusterName] != c.clusterName
+			if needRemoteUpdate {
+				delegateCopy := delegate.DeepCopy()
+				delegateCopy.Labels[common.LabelKeyParentClusterName] = c.clusterName
+				var err error
+				if delegate, err = c.customclientset.MulticlusterV1alpha1().PodChaperons(namespace).Update(ctx, delegateCopy, metav1.UpdateOptions{}); err != nil {
+					return nil, fmt.Errorf("cannot update candidate pod chaperon")
+				}
+			}
+		} else {
+			if err = c.kubeclientset.CoreV1().Pods(namespace).Delete(ctx, name, metav1.DeleteOptions{}); err != nil {
+				return nil, err
 			}
 		}
 	}

pkg/controllers/follow/cm.go

@@ -169,7 +169,7 @@ func (r configMapReconciler) Handle(obj interface{}) (requeueAfter *time.Duratio
 	if err != nil {
 		if errors.IsNotFound(err) {
 			if remoteConfigMap != nil && controller.IsRemoteControlled(remoteConfigMap, r.clusterName) {
-				if err := r.remoteClient.CoreV1().Services(namespace).Delete(ctx, name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
+				if err := r.remoteClient.CoreV1().ConfigMaps(namespace).Delete(ctx, name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
 					return nil, fmt.Errorf("cannot delete orphaned configmap: %v", err)
 				}
 			}

pkg/controllers/follow/secret.go

@@ -168,7 +168,7 @@ func (r secretReconciler) Handle(obj interface{}) (requeueAfter *time.Duration,
 	if err != nil {
 		if errors.IsNotFound(err) {
 			if remoteSecret != nil && controller.IsRemoteControlled(remoteSecret, r.clusterName) {
-				if err := r.remoteClient.CoreV1().Services(namespace).Delete(ctx, name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
+				if err := r.remoteClient.CoreV1().Secrets(namespace).Delete(ctx, name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
 					return nil, fmt.Errorf("cannot delete orphaned secret: %v", err)
 				}
 			}

test/e2e/cleanup/test.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 #
-# Copyright 2022 The Multicluster-Scheduler Authors.
+# Copyright 2023 The Multicluster-Scheduler Authors.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -29,7 +29,7 @@ cleanup_test() {
   k $i delete target $target
 
   export -f cleanup_test_iteration
-  timeout --foreground 120s bash -c "until cleanup_test_iteration $i; do sleep 1; done"
+  timeout --foreground 180s bash -c "until cleanup_test_iteration $i; do sleep 1; done"
   # use --foreground to catch ctrl-c
   # https://unix.stackexchange.com/a/233685
 

test/e2e/delete-chaperon/test.sh

@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+#
+# Copyright 2023 The Multicluster-Scheduler Authors.
+#
+# 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.
+#
+
+set -euo pipefail
+
+source test/e2e/aliases.sh
+
+delete-chaperon_test() {
+  i=$1
+
+  k $i apply -f test/e2e/delete-chaperon/test.yaml
+  k $i wait pod test-delete-chaperon --for=condition=PodScheduled
+  target="$(k $i get pod test-delete-chaperon -o json | jq -er '.spec.nodeName')"
+  j="${target: -1}"
+  uid="$(k $i get pod test-delete-chaperon -o json | jq -er '.metadata.uid')"
+  k $j delete podchaperon -l multicluster.admiralty.io/parent-uid="$uid" --wait --timeout=30s
+  k $i wait pod test-delete-chaperon --for=delete
+}
+
+if [[ "${BASH_SOURCE[0]:-}" == "${0}" ]]; then
+  delete-chaperon_test "${@}"
+fi

test/e2e/delete-chaperon/test.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-delete-chaperon
+  annotations:
+    multicluster.admiralty.io/elect: ""
+spec:
+  containers:
+    - name: pause
+      image: gcr.io/google_containers/pause

test/e2e/delete-delegate/test.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 #
-# Copyright 2022 The Multicluster-Scheduler Authors.
+# Copyright 2023 The Multicluster-Scheduler Authors.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -28,10 +28,25 @@ delete-delegate_test() {
   target="$(k $i get pod -l app=delete-delegate -o json | jq -er '.items[0].metadata.finalizers[0] | split("-") | .[1]')"
   j="${target: -1}"
   k $j delete pod -l multicluster.admiralty.io/app=delete-delegate --wait --timeout=30s
+
+  export -f delete-delegate_test_iteration
+  timeout --foreground 30s bash -c "until delete-delegate_test_iteration $j; do sleep 1; done"
+  # use --foreground to catch ctrl-c
+  # https://unix.stackexchange.com/a/233685
+
   k $j wait pod -l multicluster.admiralty.io/app=delete-delegate --for=condition=PodScheduled
   k $i delete -f test/e2e/delete-delegate/test.yaml
 }
 
+delete-delegate_test_iteration() {
+  j=$1
+
+  set -euo pipefail
+  source test/e2e/aliases.sh
+
+  [ "$(k "$j" get pod -l multicluster.admiralty.io/app=delete-delegate | wc -l)" -eq 2 ] || return 1 # including header
+}
+
 if [[ "${BASH_SOURCE[0]:-}" == "${0}" ]]; then
   delete-delegate_test "${@}"
 fi

test/e2e/e2e.sh

@@ -31,6 +31,8 @@ source test/e2e/cleanup/test.sh
 source test/e2e/no-reservation/test.sh
 source test/e2e/webhook_ready.sh
 source test/e2e/no-rogue-finalizer/test.sh
+source test/e2e/delete-chaperon/test.sh
+source test/e2e/delete-delegate/test.sh
 
 argo_setup_once
 cert_manager_setup_once
@@ -75,6 +77,8 @@ exec_test 1 2
 ingress_test 1 2
 virtual-node-labels_test 1 2
 cleanup_test 1
+delete-chaperon_test 1
+delete-delegate_test 1
 no-reservation_test
 
 no-rogue-finalizer_test

test/e2e/no-reservation/test.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 #
-# Copyright 2022 The Multicluster-Scheduler Authors.
+# Copyright 2023 The Multicluster-Scheduler Authors.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -35,7 +35,7 @@ no-reservation_test() {
 
   k 1 uncordon cluster1-control-plane
 
-  k 1 rollout status deploy no-reservation --timeout=30s
+  k 1 rollout status deploy no-reservation --timeout=120s
 
   k 1 delete -f test/e2e/no-reservation/test.yaml
   k 1 uncordon admiralty-default-c2