Skip to content

Commit 84d41b3

Browse files
committed
Don't use SSA in gcp-controller manager PodGC
This change in analogous to: kubernetes/kubernetes#121103. Required because of kubernetes/kubernetes#118261
1 parent 8fbe8d2 commit 84d41b3

File tree

3 files changed

+22
-44
lines changed

3 files changed

+22
-44
lines changed

cmd/gcp-controller-manager/BUILD

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ go_library(
7777
"//vendor/k8s.io/apiserver/pkg/server/options",
7878
"//vendor/k8s.io/apiserver/pkg/util/feature",
7979
"//vendor/k8s.io/apiserver/pkg/util/webhook",
80-
"//vendor/k8s.io/client-go/applyconfigurations/core/v1:core",
8180
"//vendor/k8s.io/client-go/informers",
8281
"//vendor/k8s.io/client-go/informers/core/v1:core",
8382
"//vendor/k8s.io/client-go/kubernetes",
@@ -98,6 +97,7 @@ go_library(
9897
"//vendor/k8s.io/controller-manager/pkg/clientbuilder",
9998
"//vendor/k8s.io/klog/v2:klog",
10099
"//vendor/k8s.io/kubernetes/pkg/api/legacyscheme",
100+
"//vendor/k8s.io/kubernetes/pkg/api/v1/pod",
101101
"//vendor/k8s.io/kubernetes/pkg/apis/certificates/install",
102102
"//vendor/k8s.io/kubernetes/pkg/apis/certificates/v1:certificates",
103103
"//vendor/k8s.io/kubernetes/pkg/controller",
@@ -139,7 +139,6 @@ go_test(
139139
"//vendor/k8s.io/apimachinery/pkg/runtime",
140140
"//vendor/k8s.io/apimachinery/pkg/runtime/schema",
141141
"//vendor/k8s.io/apimachinery/pkg/types",
142-
"//vendor/k8s.io/apimachinery/pkg/util/strategicpatch",
143142
"//vendor/k8s.io/client-go/kubernetes/fake",
144143
"//vendor/k8s.io/client-go/listers/core/v1:core",
145144
"//vendor/k8s.io/client-go/testing",

cmd/gcp-controller-manager/node_csr_approver.go

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,11 @@ import (
4848
utilerrors "k8s.io/apimachinery/pkg/util/errors"
4949
utilfeature "k8s.io/apiserver/pkg/util/feature"
5050
"k8s.io/apiserver/pkg/util/webhook"
51-
corev1apply "k8s.io/client-go/applyconfigurations/core/v1"
5251
"k8s.io/cloud-provider-gcp/pkg/csrmetrics"
5352
"k8s.io/cloud-provider-gcp/pkg/nodeidentity"
5453
"k8s.io/cloud-provider-gcp/pkg/tpmattest"
5554
"k8s.io/klog/v2"
55+
apipod "k8s.io/kubernetes/pkg/api/v1/pod"
5656
certutil "k8s.io/kubernetes/pkg/apis/certificates/v1"
5757
"k8s.io/kubernetes/pkg/controller/certificates"
5858
"k8s.io/kubernetes/pkg/features"
@@ -1038,22 +1038,17 @@ func markFailedAndDeletePodWithCondition(ctx *controllerContext, pod *v1.Pod) er
10381038
// is orphaned, in which case the pod would remain in the Running phase
10391039
// forever as there is no kubelet running to change the phase.
10401040
if pod.Status.Phase != v1.PodSucceeded && pod.Status.Phase != v1.PodFailed {
1041-
podApply := corev1apply.Pod(pod.Name, pod.Namespace).WithStatus(corev1apply.PodStatus())
1042-
// we don't need to extract the pod apply configuration and can send
1043-
// only phase and the DisruptionTarget condition as GCPControllerManager would not
1044-
// own other fields. If the DisruptionTarget condition is owned by
1045-
// GCPControllerManager it means that it is in the Failed phase, so sending the
1046-
// condition will not be re-attempted.
1047-
podApply.Status.WithPhase(v1.PodFailed)
1048-
podApply.Status.WithConditions(
1049-
corev1apply.PodCondition().
1050-
WithType(v1.DisruptionTarget).
1051-
WithStatus(v1.ConditionTrue).
1052-
WithReason("DeletionByGCPControllerManager").
1053-
WithMessage(fmt.Sprintf("%s: node no longer exists", fieldManager)).
1054-
WithLastTransitionTime(metav1.Now()))
1055-
1056-
if _, err := ctx.client.CoreV1().Pods(pod.Namespace).ApplyStatus(context.TODO(), podApply, metav1.ApplyOptions{FieldManager: fieldManager, Force: true}); err != nil {
1041+
newPod := pod.DeepCopy()
1042+
newPod.Status.Phase = v1.PodFailed
1043+
apipod.UpdatePodCondition(&newPod.Status, &v1.PodCondition{
1044+
Type: v1.DisruptionTarget,
1045+
Status: v1.ConditionTrue,
1046+
Reason: "DeletionByGCPControllerManager",
1047+
Message: fmt.Sprintf("%s: node no longer exists", fieldManager),
1048+
})
1049+
if _, err := ctx.client.CoreV1().Pods(pod.Namespace).UpdateStatus(context.TODO(), newPod, metav1.UpdateOptions{
1050+
FieldManager: fieldManager,
1051+
}); err != nil {
10571052
return err
10581053
}
10591054
}

cmd/gcp-controller-manager/node_csr_approver_test.go

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ import (
5050
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
5151
"k8s.io/apimachinery/pkg/runtime"
5252
"k8s.io/apimachinery/pkg/runtime/schema"
53-
"k8s.io/apimachinery/pkg/util/strategicpatch"
5453
"k8s.io/client-go/kubernetes/fake"
5554
testclient "k8s.io/client-go/testing"
5655
"k8s.io/cloud-provider-gcp/pkg/nodeidentity"
@@ -1783,16 +1782,16 @@ func TestDeleteAllPodsBoundToNode(t *testing.T) {
17831782
Status: v1.PodStatus{
17841783
Phase: v1.PodFailed,
17851784
Conditions: []v1.PodCondition{
1785+
{
1786+
Type: v1.PodReady,
1787+
Status: v1.ConditionTrue,
1788+
},
17861789
{
17871790
Type: v1.DisruptionTarget,
17881791
Status: v1.ConditionTrue,
17891792
Reason: "DeletionByGCPControllerManager",
17901793
Message: "GCPControllerManager: node no longer exists",
17911794
},
1792-
{
1793-
Type: v1.PodReady,
1794-
Status: v1.ConditionTrue,
1795-
},
17961795
},
17971796
},
17981797
},
@@ -1876,12 +1875,12 @@ func TestDeleteAllPodsBoundToNode(t *testing.T) {
18761875
t.Fatalf("Unexpected number of actions, got %v, want 3 (list pods, patch pod, delete pod)", len(actions))
18771876
}
18781877

1879-
var patchAction testclient.PatchAction
1878+
var updateAction testclient.UpdateAction
18801879
var deleteAction testclient.DeleteAction
18811880

18821881
for _, action := range actions {
1883-
if action.GetVerb() == "patch" {
1884-
patchAction = action.(testclient.PatchAction)
1882+
if action.GetVerb() == "update" {
1883+
updateAction = action.(testclient.UpdateAction)
18851884
}
18861885

18871886
if action.GetVerb() == "delete" {
@@ -1890,23 +1889,8 @@ func TestDeleteAllPodsBoundToNode(t *testing.T) {
18901889
}
18911890

18921891
if tc.expectedPatchedPod != nil {
1893-
patchedPodBytes := patchAction.GetPatch()
1894-
1895-
originalPod, err := json.Marshal(tc.pod)
1896-
if err != nil {
1897-
t.Fatalf("Failed to marshal original pod %#v: %v", originalPod, err)
1898-
}
1899-
updated, err := strategicpatch.StrategicMergePatch(originalPod, patchedPodBytes, v1.Pod{})
1900-
if err != nil {
1901-
t.Fatalf("Failed to apply strategic merge patch %q on pod %#v: %v", patchedPodBytes, originalPod, err)
1902-
}
1903-
1904-
updatedPod := &v1.Pod{}
1905-
if err := json.Unmarshal(updated, updatedPod); err != nil {
1906-
t.Fatalf("Failed to unmarshal updated pod %q: %v", updated, err)
1907-
}
1908-
1909-
if diff := cmp.Diff(tc.expectedPatchedPod, updatedPod, cmpopts.IgnoreFields(v1.Pod{}, "TypeMeta"), cmpopts.IgnoreFields(v1.PodCondition{}, "LastTransitionTime")); diff != "" {
1892+
patchedPod := updateAction.GetObject().(*v1.Pod)
1893+
if diff := cmp.Diff(tc.expectedPatchedPod, patchedPod, cmpopts.IgnoreFields(v1.Pod{}, "TypeMeta"), cmpopts.IgnoreFields(v1.PodCondition{}, "LastTransitionTime")); diff != "" {
19101894
t.Fatalf("Unexpected diff on pod (-want,+got):\n%s", diff)
19111895
}
19121896
}

0 commit comments

Comments
 (0)