From fb3d9e3ac9d244047efa411e9e6ae162692ad79e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Thu, 13 Oct 2022 11:45:42 +0300 Subject: [PATCH 1/3] rollout restart: Change error message to more descriptive `rollout restart` command calculates patches according to the `kubectl.kubernetes.io/restartedAt` annotation whose time format is RFC3339. That is sufficient for users. However, if automated scripts execute `rollout restart` in multiple times within second, commands fails by returning an error "empty patch". This PR changes error message to more descriptive format to warn users that rollout restart does not work subsequent execution within second. --- staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart.go b/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart.go index a7cc5bc762b..8a803a19b2d 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart.go @@ -18,7 +18,6 @@ package rollout import ( "fmt" - "github.com/spf13/cobra" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" @@ -183,7 +182,8 @@ func (o RestartOptions) RunRestart() error { } if string(patch.Patch) == "{}" || len(patch.Patch) == 0 { - allErrs = append(allErrs, fmt.Errorf("failed to create patch for %v: empty patch", info.Name)) + allErrs = append(allErrs, fmt.Errorf("failed to create patch for %v: if restart has already been triggered within the past second, please wait before attempting to trigger another", info.Name)) + continue } obj, err := resource.NewHelper(info.Client, info.Mapping). From 4699801d65c00ebb95d68a159b8cb2627ad783a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Fri, 18 Nov 2022 10:37:56 +0300 Subject: [PATCH 2/3] Add unit test for error case --- .../pkg/cmd/rollout/rollout_restart_test.go | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go index 5dc47d21a8f..c4425e8379e 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go @@ -23,6 +23,8 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" + appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -70,6 +72,56 @@ func TestRolloutRestartOne(t *testing.T) { } } +func TestRolloutRestartError(t *testing.T) { + deploymentName := "deployment/nginx-deployment" + ns := scheme.Codecs.WithoutConversion() + tf := cmdtesting.NewTestFactory().WithNamespace("test") + + info, _ := runtime.SerializerInfoForMediaType(ns.SupportedMediaTypes(), runtime.ContentTypeJSON) + encoder := ns.EncoderForVersion(info.Serializer, rolloutRestartGroupVersionEncoder) + tf.Client = &RolloutRestartRESTClient{ + RESTClient: &fake.RESTClient{ + GroupVersion: rolloutRestartGroupVersionEncoder, + NegotiatedSerializer: ns, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + switch p, m := req.URL.Path, req.Method; { + case p == "/namespaces/test/deployments/nginx-deployment" && (m == "GET" || m == "PATCH"): + responseDeployment := &appsv1.Deployment{} + responseDeployment.Name = deploymentName + body := ioutil.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(encoder, responseDeployment)))) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: body}, nil + default: + t.Fatalf("unexpected request: %#v\n%#v", req.URL, req) + return nil, nil + } + }), + }, + } + + streams, _, bufOut, _ := genericclioptions.NewTestIOStreams() + opt := NewRolloutRestartOptions(streams) + err := opt.Complete(tf, nil, []string{deploymentName}) + assert.NoError(t, err) + err = opt.Validate() + assert.NoError(t, err) + opt.Restarter = func(obj runtime.Object) ([]byte, error) { + return runtime.Encode(scheme.Codecs.LegacyCodec(appsv1.SchemeGroupVersion), obj) + } + + expectedErr := "failed to create patch for nginx-deployment: if restart has already been triggered within the past second, please wait before attempting to trigger another" + err = opt.RunRestart() + if err == nil { + t.Errorf("error expected but not fired") + } + if err.Error() != expectedErr { + t.Errorf("unexpected error fired %v", err) + } + + if bufOut.String() != "" { + t.Errorf("unexpected message") + } +} + // Tests that giving selectors with no matching objects shows an error func TestRolloutRestartSelectorNone(t *testing.T) { labelSelector := "app=test" From d0f558612ad6220dc1b2d007eec2cf156d7cf56a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Mon, 12 Dec 2022 09:34:35 +0300 Subject: [PATCH 3/3] use io instead ioutil --- .../src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go index c4425e8379e..504f0fc97f1 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_restart_test.go @@ -88,7 +88,7 @@ func TestRolloutRestartError(t *testing.T) { case p == "/namespaces/test/deployments/nginx-deployment" && (m == "GET" || m == "PATCH"): responseDeployment := &appsv1.Deployment{} responseDeployment.Name = deploymentName - body := ioutil.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(encoder, responseDeployment)))) + body := io.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(encoder, responseDeployment)))) return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: body}, nil default: t.Fatalf("unexpected request: %#v\n%#v", req.URL, req)