Don't use SSA in gcp-controller manager PodGC

This change in analogous to: kubernetes/kubernetes#121103. Required because of kubernetes/kubernetes#118261
mimowo · Oct 11, 2023 · 7c252f8 · 7c252f8
1 parent 8fbe8d2
commit 7c252f8
Showing 2 changed files with 21 additions and 42 deletions.
diff --git a/cmd/gcp-controller-manager/node_csr_approver.go b/cmd/gcp-controller-manager/node_csr_approver.go
@@ -48,11 +48,11 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/apiserver/pkg/util/webhook"
-	corev1apply "k8s.io/client-go/applyconfigurations/core/v1"
 	"k8s.io/cloud-provider-gcp/pkg/csrmetrics"
 	"k8s.io/cloud-provider-gcp/pkg/nodeidentity"
 	"k8s.io/cloud-provider-gcp/pkg/tpmattest"
 	"k8s.io/klog/v2"
+	apipod "k8s.io/kubernetes/pkg/api/v1/pod"
 	certutil "k8s.io/kubernetes/pkg/apis/certificates/v1"
 	"k8s.io/kubernetes/pkg/controller/certificates"
 	"k8s.io/kubernetes/pkg/features"
@@ -1038,22 +1038,17 @@ func markFailedAndDeletePodWithCondition(ctx *controllerContext, pod *v1.Pod) er
 		// is orphaned, in which case the pod would remain in the Running phase
 		// forever as there is no kubelet running to change the phase.
 		if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed {
-			podApply := corev1apply.Pod(pod.Name, pod.Namespace).WithStatus(corev1apply.PodStatus())
-			// we don't need to extract the pod apply configuration and can send
-			// only phase and the DisruptionTarget condition as GCPControllerManager would not
-			// own other fields. If the DisruptionTarget condition is owned by
-			// GCPControllerManager it means that it is in the Failed phase, so sending the
-			// condition will not be re-attempted.
-			podApply.Status.WithPhase(v1.PodFailed)
-			podApply.Status.WithConditions(
-				corev1apply.PodCondition().
-					WithType(v1.DisruptionTarget).
-					WithStatus(v1.ConditionTrue).
-					WithReason("DeletionByGCPControllerManager").
-					WithMessage(fmt.Sprintf("%s: node no longer exists", fieldManager)).
-					WithLastTransitionTime(metav1.Now()))
-
-			if _, err := ctx.client.CoreV1().Pods(pod.Namespace).ApplyStatus(context.TODO(), podApply, metav1.ApplyOptions{FieldManager: fieldManager, Force: true}); err != nil {
+			newPod := pod.DeepCopy()
+			newPod.Status.Phase = v1.PodFailed
+			apipod.UpdatePodCondition(&newPod.Status, &v1.PodCondition{
+				Type:    v1.DisruptionTarget,
+				Status:  v1.ConditionTrue,
+				Reason:  "DeletionByGCPControllerManager",
+				Message: fmt.Sprintf("%s: node no longer exists", fieldManager),
+			})
+			if _, err := ctx.client.CoreV1().Pods(pod.Namespace).UpdateStatus(context.TODO(), newPod, metav1.UpdateOptions{
+				FieldManager: fieldManager,
+			}); err != nil {
 				return err
 			}
 		}

diff --git a/cmd/gcp-controller-manager/node_csr_approver_test.go b/cmd/gcp-controller-manager/node_csr_approver_test.go
@@ -50,7 +50,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/client-go/kubernetes/fake"
 	testclient "k8s.io/client-go/testing"
 	"k8s.io/cloud-provider-gcp/pkg/nodeidentity"
@@ -1783,16 +1782,16 @@ func TestDeleteAllPodsBoundToNode(t *testing.T) {
 				Status: v1.PodStatus{
 					Phase: v1.PodFailed,
 					Conditions: []v1.PodCondition{
+						{
+							Type:   v1.PodReady,
+							Status: v1.ConditionTrue,
+						},
 						{
 							Type:    v1.DisruptionTarget,
 							Status:  v1.ConditionTrue,
 							Reason:  "DeletionByGCPControllerManager",
 							Message: "GCPControllerManager: node no longer exists",
 						},
-						{
-							Type:   v1.PodReady,
-							Status: v1.ConditionTrue,
-						},
 					},
 				},
 			},
@@ -1876,12 +1875,12 @@ func TestDeleteAllPodsBoundToNode(t *testing.T) {
 				t.Fatalf("Unexpected number of actions, got %v, want 3 (list pods, patch pod, delete pod)", len(actions))
 			}
 
-			var patchAction testclient.PatchAction
+			var updateAction testclient.UpdateAction
 			var deleteAction testclient.DeleteAction
 
 			for _, action := range actions {
-				if action.GetVerb() == "patch" {
-					patchAction = action.(testclient.PatchAction)
+				if action.GetVerb() == "update" {
+					updateAction = action.(testclient.UpdateAction)
 				}
 
 				if action.GetVerb() == "delete" {
@@ -1890,23 +1889,8 @@ func TestDeleteAllPodsBoundToNode(t *testing.T) {
 			}
 
 			if tc.expectedPatchedPod != nil {
-				patchedPodBytes := patchAction.GetPatch()
-
-				originalPod, err := json.Marshal(tc.pod)
-				if err != nil {
-					t.Fatalf("Failed to marshal original pod %#v: %v", originalPod, err)
-				}
-				updated, err := strategicpatch.StrategicMergePatch(originalPod, patchedPodBytes, v1.Pod{})
-				if err != nil {
-					t.Fatalf("Failed to apply strategic merge patch %q on pod %#v: %v", patchedPodBytes, originalPod, err)
-				}
-
-				updatedPod := &v1.Pod{}
-				if err := json.Unmarshal(updated, updatedPod); err != nil {
-					t.Fatalf("Failed to unmarshal updated pod %q: %v", updated, err)
-				}
-
-				if diff := cmp.Diff(tc.expectedPatchedPod, updatedPod, cmpopts.IgnoreFields(v1.Pod{}, "TypeMeta"), cmpopts.IgnoreFields(v1.PodCondition{}, "LastTransitionTime")); diff != "" {
+				patchedPod := updateAction.GetObject().(*v1.Pod)
+				if diff := cmp.Diff(tc.expectedPatchedPod, patchedPod, cmpopts.IgnoreFields(v1.Pod{}, "TypeMeta"), cmpopts.IgnoreFields(v1.PodCondition{}, "LastTransitionTime")); diff != "" {
 					t.Fatalf("Unexpected diff on pod (-want,+got):\n%s", diff)
 				}
 			}