diff --git a/pkg/kube/client.go b/pkg/kube/client.go index ba881c560..e6cb56153 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -885,7 +885,12 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate } } - if err := patchResourceServerSide(target, updateOptions.dryRun, updateOptions.forceConflicts, updateOptions.fieldValidationDirective); err != nil { + if err := retry.OnError( + retry.DefaultRetry, + isServerSideRetryable, + func() error { + return patchResourceServerSide(target, updateOptions.dryRun, updateOptions.forceConflicts, updateOptions.fieldValidationDirective) + }); err != nil { logger.Debug("Error patching resource", slog.Any("error", err)) return err } diff --git a/pkg/kube/client_update_ssa_retry_test.go b/pkg/kube/client_update_ssa_retry_test.go new file mode 100644 index 000000000..d8da004c7 --- /dev/null +++ b/pkg/kube/client_update_ssa_retry_test.go @@ -0,0 +1,89 @@ +/* +Copyright The Helm 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. +*/ + +package kube + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "k8s.io/client-go/rest/fake" + "k8s.io/client-go/util/retry" + cmdtesting "k8s.io/kubectl/pkg/cmd/testing" +) + +// TestUpdateServerSideApplyRetriesOnResourceQuotaConflict is a regression +// test for the *update* (patch) path of server-side apply. +// +// PR #32088 (fixing issue #32087) added a retry.OnError wrapper around +// patchResourceServerSide in makeCreateApplyFunc, so that transient 409 +// conflicts on a ResourceQuota's optimistic lock (see +// https://github.com/kubernetes/kubernetes/issues/67761) are retried when +// *creating* a resource via server-side apply. +// +// The exact same patchResourceServerSide call also happens in +// makeUpdateApplyFunc (used by every `helm upgrade` that patches an +// already-existing resource), but that call site was not given the same +// retry wrapper. This test proves that gap: a resourcequota conflict on the +// update path currently fails immediately instead of retrying. +func TestUpdateServerSideApplyRetriesOnResourceQuotaConflict(t *testing.T) { + c := newTestClient(t) + + pods := newPodList("otter") + + cb := func(_ []RequestResponseAction, req *http.Request) (*http.Response, error) { + p, m := req.URL.Path, req.Method + switch { + case p == "/namespaces/default/pods/otter" && m == http.MethodGet: + return newResponse(http.StatusOK, &pods.Items[0]) + case p == "/namespaces/default/pods/otter" && m == http.MethodPatch: + return newResponseJSON(http.StatusConflict, resourceQuotaConflict) + } + t.Fatalf("unexpected request: %s %s", m, p) + return nil, nil + } + + client := NewRequestResponseLogClient(t, cb) + c.Factory.(*cmdtesting.TestFactory).UnstructuredClient = &fake.RESTClient{ + NegotiatedSerializer: unstructuredSerializer, + Client: fake.CreateHTTPClient(client.Do), + } + + originals, err := c.Build(objBody(&pods), false) + require.NoError(t, err) + targets, err := c.Build(objBody(&pods), false) + require.NoError(t, err) + + _, err = c.Update( + originals, + targets, + ClientUpdateOptionServerSideApply(true, false), + ) + require.ErrorContains(t, err, "Operation cannot be fulfilled on resourcequotas") + + patchCount := 0 + for _, action := range client.Actions { + if action.Request.URL.Path == "/namespaces/default/pods/otter" && action.Request.Method == http.MethodPatch { + patchCount++ + } + } + + assert.Equal(t, retry.DefaultRetry.Steps, patchCount, + "expected helm to retry the server-side apply PATCH on a resourcequota conflict during update, the same way it already does during create") +}