From 13be899b422a1f68c38e3a9c9d88831db709a32d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Fri, 2 Dec 2022 16:00:22 +0300 Subject: [PATCH 1/3] kubectl scale: Use visitor only once `kubectl scale` calls visitor two times. Second call fails when the piped input is passed by returning an `error: no objects passed to scale` error. This PR uses the result of first visitor and fixes that piped input problem. In addition to that, this PR also adds new scale test to verify. --- .../src/k8s.io/kubectl/pkg/cmd/scale/scale.go | 34 ++++++++----------- test/cmd/core.sh | 9 +++++ 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go b/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go index 9571351d40e..2aaa99191a9 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go @@ -209,13 +209,10 @@ func (o *ScaleOptions) RunScale() error { return err } - infos := []*resource.Info{} - r.Visit(func(info *resource.Info, err error) error { - if err == nil { - infos = append(infos, info) - } - return nil - }) + infos, err := r.Infos() + if err != nil { + return err + } if len(o.ResourceVersion) != 0 && len(infos) > 1 { return fmt.Errorf("cannot use --resource-version with multiple resources") @@ -234,17 +231,16 @@ func (o *ScaleOptions) RunScale() error { waitForReplicas = scale.NewRetryParams(1*time.Second, o.Timeout) } - counter := 0 - err = r.Visit(func(info *resource.Info, err error) error { - if err != nil { - return err - } - counter++ + if len(infos) == 0 { + return fmt.Errorf("no objects passed to scale") + } + for _, info := range infos { mapping := info.ResourceMapping() if o.dryRunStrategy == cmdutil.DryRunClient { return o.PrintObj(info.Object, o.Out) } + if err := o.scaler.Scale(info.Namespace, info.Name, uint(o.Replicas), precondition, retry, waitForReplicas, mapping.Resource, o.dryRunStrategy == cmdutil.DryRunServer); err != nil { return err } @@ -263,14 +259,12 @@ func (o *ScaleOptions) RunScale() error { } } - return o.PrintObj(info.Object, o.Out) - }) - if err != nil { - return err - } - if counter == 0 { - return fmt.Errorf("no objects passed to scale") + err := o.PrintObj(info.Object, o.Out) + if err != nil { + return err + } } + return nil } diff --git a/test/cmd/core.sh b/test/cmd/core.sh index 1ce79d8188a..4733c9212fe 100755 --- a/test/cmd/core.sh +++ b/test/cmd/core.sh @@ -1289,6 +1289,15 @@ run_rc_tests() { # Clean-up kubectl delete deployment/nginx-deployment "${kube_flags[@]}" + ### Scale a deployment with piped input + kubectl create -f test/fixtures/doc-yaml/user-guide/deployment.yaml "${kube_flags[@]}" + # Command + kubectl get deployment/nginx-deployment -o json | kubectl scale --replicas=2 -f - + # Post-condition: 2 replica for nginx-deployment + kube::test::get_object_assert 'deployment nginx-deployment' "{{${deployment_replicas:?}}}" '2' + # Clean-up + kubectl delete deployment/nginx-deployment "${kube_flags[@]}" + ### Expose deployments by creating a service # Uses deployment selectors for created service output_message=$(kubectl expose -f test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment.yaml --port 80 2>&1 "${kube_flags[@]}") From 76ee3788ccbac9003e3f24de9000ebd91c27611f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Fri, 2 Dec 2022 16:27:18 +0300 Subject: [PATCH 2/3] kubectl scale: Add dry-run prefix to indicate result is not applied Currently, if user executes `kubectl scale --dry-run`, output has no indicator showing that this is not applied in reality. This PR adds dry run suffix to the output as well as more integration tests to verify it. --- .../src/k8s.io/kubectl/pkg/cmd/scale/scale.go | 15 ++++++++---- test/cmd/core.sh | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go b/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go index 2aaa99191a9..1f1f0a53148 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go @@ -144,16 +144,18 @@ func (o *ScaleOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st if err != nil { return err } + + o.dryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd) + if err != nil { + return err + } + cmdutil.PrintFlagsWithDryRunStrategy(o.PrintFlags, o.dryRunStrategy) printer, err := o.PrintFlags.ToPrinter() if err != nil { return err } o.PrintObj = printer.PrintObj - o.dryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd) - if err != nil { - return err - } dynamicClient, err := f.DynamicClient() if err != nil { return err @@ -238,7 +240,10 @@ func (o *ScaleOptions) RunScale() error { for _, info := range infos { mapping := info.ResourceMapping() if o.dryRunStrategy == cmdutil.DryRunClient { - return o.PrintObj(info.Object, o.Out) + if err := o.PrintObj(info.Object, o.Out); err != nil { + return err + } + continue } if err := o.scaler.Scale(info.Namespace, info.Name, uint(o.Replicas), precondition, retry, waitForReplicas, mapping.Resource, o.dryRunStrategy == cmdutil.DryRunServer); err != nil { diff --git a/test/cmd/core.sh b/test/cmd/core.sh index 4733c9212fe..ac2bb1c6404 100755 --- a/test/cmd/core.sh +++ b/test/cmd/core.sh @@ -1272,6 +1272,20 @@ run_rc_tests() { ### Scale multiple replication controllers kubectl create -f test/e2e/testing-manifests/guestbook/legacy/redis-master-controller.yaml "${kube_flags[@]}" kubectl create -f test/e2e/testing-manifests/guestbook/legacy/redis-slave-controller.yaml "${kube_flags[@]}" + # Command dry-run client + output_message=$(kubectl scale rc/redis-master rc/redis-slave --replicas=4 --dry-run=client "${kube_flags[@]}") + # Post-condition dry-run client: 1 replicas each + kube::test::if_has_string "${output_message}" 'replicationcontroller/redis-master scaled (dry run)' + kube::test::if_has_string "${output_message}" 'replicationcontroller/redis-slave scaled (dry run)' + kube::test::get_object_assert 'rc redis-master' "{{$rc_replicas_field}}" '1' + kube::test::get_object_assert 'rc redis-slave' "{{$rc_replicas_field}}" '2' + # Command dry-run server + output_message=$(kubectl scale rc/redis-master rc/redis-slave --replicas=4 --dry-run=server "${kube_flags[@]}") + # Post-condition dry-run server: 1 replicas each + kube::test::if_has_string "${output_message}" 'replicationcontroller/redis-master scaled (server dry run)' + kube::test::if_has_string "${output_message}" 'replicationcontroller/redis-slave scaled (server dry run)' + kube::test::get_object_assert 'rc redis-master' "{{$rc_replicas_field}}" '1' + kube::test::get_object_assert 'rc redis-slave' "{{$rc_replicas_field}}" '2' # Command kubectl scale rc/redis-master rc/redis-slave --replicas=4 "${kube_flags[@]}" # Post-condition: 4 replicas each @@ -1282,6 +1296,16 @@ run_rc_tests() { ### Scale a deployment kubectl create -f test/fixtures/doc-yaml/user-guide/deployment.yaml "${kube_flags[@]}" + # Command dry-run client + output_message=$(kubectl scale --current-replicas=3 --replicas=1 deployment/nginx-deployment --dry-run=client) + # Post-condition: 3 replica for nginx-deployment dry-run client + kube::test::if_has_string "${output_message}" 'nginx-deployment scaled (dry run)' + kube::test::get_object_assert 'deployment nginx-deployment' "{{${deployment_replicas:?}}}" '3' + # Command dry-run server + output_message=$(kubectl scale --current-replicas=3 --replicas=1 deployment/nginx-deployment --dry-run=server) + # Post-condition: 3 replica for nginx-deployment dry-run server + kube::test::if_has_string "${output_message}" 'nginx-deployment scaled (server dry run)' + kube::test::get_object_assert 'deployment nginx-deployment' "{{${deployment_replicas:?}}}" '3' # Command kubectl scale --current-replicas=3 --replicas=1 deployment/nginx-deployment # Post-condition: 1 replica for nginx-deployment From b84f192acc61b5fa9dc438950e6cc57f75889853 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Fri, 2 Dec 2022 17:23:44 +0300 Subject: [PATCH 3/3] kubectl scale: proceed even if there is invalid resource in multi --- staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go b/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go index 1f1f0a53148..9529aeef8db 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/scale/scale.go @@ -211,10 +211,11 @@ func (o *ScaleOptions) RunScale() error { return err } - infos, err := r.Infos() - if err != nil { - return err - } + // We don't immediately return infoErr if it is not nil. + // Because we want to proceed for other valid resources and + // at the end of the function, we'll return this + // to show invalid resources to the user. + infos, infoErr := r.Infos() if len(o.ResourceVersion) != 0 && len(infos) > 1 { return fmt.Errorf("cannot use --resource-version with multiple resources") @@ -270,7 +271,7 @@ func (o *ScaleOptions) RunScale() error { } } - return nil + return infoErr } func scaler(f cmdutil.Factory) (scale.Scaler, error) {