feature(service): Wait on update for service to become ready. #271

rhuss · 2019-07-18T10:18:02Z

The behaviour is now the same as for kn service create. This should also fix some flakiness in the tests again.

navidshaikh · 2019-07-18T11:18:21Z

docs/cmd/kn_service_update.md

@@ -27,6 +27,7 @@ kn service update NAME [flags]
 ### Options

 ```
+      --async                    Create service and don't wait for it to become ready.


Now this should be, Create or update.

client/pkg/kn/commands/wait_flags.go

Line 36 in 0b348d7

waitUsage := fmt.Sprintf("Create %s and don't wait for it to become ready.", what)

Ah, right. Let me fix that (maybe using a switch to have different messags for create and update)

rhuss · 2019-07-18T12:22:10Z

/retest

navidshaikh · 2019-07-18T12:32:27Z

pkg/kn/commands/wait_flags.go

@@ -32,8 +32,8 @@ type WaitFlags struct {
 // Add flags which influence the sync/async behaviour when creating or updating
 // resources. Set `waitDefault` argument if the default behaviour is synchronous.
 // Use `what` for describing what is waited for.
-func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, what string) {
-	waitUsage := fmt.Sprintf("Create %s and don't wait for it to become ready.", what)
+func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, action string, what string) {


knative-prow-robot · 2019-07-18T12:32:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhuss · 2019-07-18T16:42:07Z

@maximilien @sixolet @cppforlife this is an important feature/fix which fixes a test flake introduced lately (where a kn route list comes directly after kn service update and the test expects the route to be already up).

would be cool if this could be reviewed asap as e.g. #267 integration test is constantly failing because of this.

maximilien · 2019-07-18T20:07:11Z

pkg/kn/commands/service/service.go

@@ -36,3 +43,16 @@ func NewServiceCommand(p *commands.KnParams) *cobra.Command {
 	serviceCmd.AddCommand(NewServiceUpdateCommand(p))
 	return serviceCmd
 }
+
+func waitForService(client serving_kn_v1alpha1.KnClient, serviceName string, out io.Writer, timeout int) error {
+	fmt.Fprintf(out, "Waiting for service '%s' to become ready ... ", serviceName)


And what happens if user CTRL-C out. I assume the API request continues but kn fails with user cancel? Might want to be clear. Since alternatively you could trap CTL-C and try to cancel cleanly or even undo change... Not advocating for clean up at this point but users will want to cancel and let's anticipate that and aid them.

Good point. But I would interpret CTL-C as "cancelling the waiting" (that is where it currently states). We could try to catch that and print out something like "Cancelled waiting for service to become ready". It then just turns into an async behaviour where you have to check the server state.

But a complete rollback is imo out of scope (that would mean to implement a fully transactional behaviour, recording the previous state and as you are on the client you don't have full control what's happening on the server-side. At least some events have been produced, which might trigger some other, non-rollbackable actions etc. A client-side managed transaction with possible compensation actions is not something easy.

OK on canceling the waiting. Best.

Since alternatively you could trap CTL-C and try to cancel cleanly or even undo change
this would be a very surprising behaviour to most users.

I still have to add that kind of trapping for CTRL-C and printing out something like

"Interrupt: Stop waiting on service completion. Please check the service's status yourself."

or so.

maximilien · 2019-07-18T20:09:11Z

Nice 👍 work BTW, definitely a needed feature. Cheers 🍻

sixolet · 2019-07-19T20:26:50Z

/lgtm

maximilien · 2019-07-19T23:13:05Z

/retest

maximilien · 2019-07-19T23:18:16Z

@rhuss build is failing due to:

# github.com/knative/client/pkg/kn/commands/service [github.com/knative/client/pkg/kn/commands/service.test]
pkg/kn/commands/service/service_update_test.go:94:19: undefined: serving.GetRevisionTemplate
pkg/kn/commands/service/service_update_test.go:107:18: undefined: serving.GetRevisionTemplate
?   	github.com/knative/client/cmd/kn	[no test files]
?   	github.com/knative/client/hack	[no test files]
ok  	github.com/knative/client/pkg	0.098s [no tests to run]
ok  	github.com/knative/client/pkg/kn/commands	0.020s [no tests to run]
ok  	github.com/knative/client/pkg/kn/commands/revision	0.015s [no tests to run]
ok  	github.com/knative/client/pkg/kn/commands/route	0.015s [no tests to run]
FAIL	github.com/knative/client/pkg/kn/commands/service [build failed]
...

rhuss · 2019-07-20T06:46:23Z

@maximilien that's because of the last merge of #275 which renamed that method. Going to fix that over the weekend.

The behaviour is now the same as for `kn service create`. This should also fix some flakiness in the tests again.

rhuss · 2019-07-22T08:06:49Z

@maximilien rename issue fixed.

knative-metrics-robot · 2019-07-22T08:08:55Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/service.go	100.0%	86.7%	-13.3
pkg/kn/commands/service/service_create.go	77.4%	77.5%	0.1
pkg/kn/commands/service/service_update.go	70.0%	73.7%	3.7

rhuss · 2019-07-22T14:43:55Z

@sixolet @maximilien @cppforlife PTAL, it already has been /lgtmed but had to be rebased because of an upstream merge conflict (renaming of some methods). Would be cool to lgtm it again, some other PRs are waiting for this so that the integration tests are back green again.

cppforlife · 2019-07-22T17:45:08Z

pkg/kn/commands/service/service.go

 	"github.com/knative/client/pkg/kn/commands"
+	serving_kn_v1alpha1 "github.com/knative/client/pkg/serving/v1alpha1"
+
+	"fmt"


i thought we were grouping std imports together?

We are running now "goimports" unconditionally on every build. So we take exactly what is spit out there, that there is no ambiguity anymore. Is there anything we could optimize ?

cppforlife · 2019-07-22T17:45:30Z

docs/cmd/kn_service_update.md

@@ -40,6 +41,7 @@ kn service update NAME [flags]
  -p, --port int32               The port where application listens on.
      --requests-cpu string      The requested CPU (e.g., 250m).
      --requests-memory string   The requested memory (e.g., 64Mi).
+      --wait-timeout int         Seconds to wait before giving up on waiting for service to be ready. (default 60)


shouldnt we set wait-timeout to be time.Duration. otherwise what's the unit of this flag?

As the message says, its "Seconds to wait". I think (hope) that's good enough, but we can improve on adding a unit, too (like moving to a string and parsing "1min"). personally, I think that's overkill.

cppforlife · 2019-07-22T17:46:28Z

docs/cmd/kn_service_update.md

@@ -27,6 +27,7 @@ kn service update NAME [flags]
 ### Options

 ```
+      --async                    Update service and don't wait for it to become ready.


we should rename this flag to --wait=bool. async is too confusing imho with serveless "async" handler configuration (no expected response).

Happy to rename when we decided to on the general direction. See #283 and #284 for voting how to continue with boolean flags (like --help=true)

sixolet · 2019-07-22T17:51:56Z

/lgtm

cppforlife · 2019-07-22T17:54:49Z

pkg/kn/commands/service/service_update_test.go

@@ -75,7 +122,7 @@ func TestServiceUpdateImage(t *testing.T) {
 	servinglib.UpdateImage(template, "gcr.io/foo/bar:baz")

 	action, updated, output, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar"})
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--async"}, false)


it seems strange that we placed non-default behaviour in bunch of tests. given that these are unit tests i dont see a good reason why we would add --async to bunch of them instead of just having a dedicated test that tests --async.

I agree, that we should update most of them. The more involved part is to get the reactor right for simulating the sync behaviour, but we can change that.

There is at least one test for the sync behaviour.

That not all tests have been changed is mostly because of lack of time. As we should refactor the create/update tests anyway (think there is even an issue open for that), I thought its ok to postpone that, too.

sixolet · 2019-07-22T17:58:25Z

Unit tests for the simpler behavior (asynchronous) make perfect sense to me. We should have tests for both; should we add more specific waiting tests you think?

…

On Jul 22, 2019, at 10:54 AM, Dmitriy Kalinin ***@***.***> wrote: @cppforlife commented on this pull request. In pkg/kn/commands/service/service_update_test.go: > @@ -75,7 +122,7 @@ func TestServiceUpdateImage(t *testing.T) { servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, output, err := fakeServiceUpdate(orig, []string{ - "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar"}) + "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--async"}, false) it seems strange that we placed non-default behaviour in bunch of tests. given that these are unit tests i dont see a good reason why we would add --async to bunch of them instead of just having a dedicated test that tests --async. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

- We got synchronous service update operation, ensuring the requested config change is reconciled before service update operation returns. knative#271 - Traffic splitting e2e tests, do sync service update to generate the revisions. - With every service update, we need to grab the revision name generated with this update (for subsequent operations), which we can grab using `latestReadyRevisionName` since sync service updated returned.

let the defaults spin

Due to tektoncd/pipeline#253

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 18, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2019

knative-prow-robot requested review from evankanderson and sixolet July 18, 2019 10:18

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 18, 2019

navidshaikh reviewed Jul 18, 2019

View reviewed changes

navidshaikh approved these changes Jul 18, 2019

View reviewed changes

rhuss mentioned this pull request Jul 18, 2019

chore(revision): Remove unwanted log message in test #267

Merged

maximilien suggested changes Jul 18, 2019

View reviewed changes

knative-prow-robot assigned sixolet Jul 19, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2019

rhuss added 4 commits July 22, 2019 10:02

feature(service): Wait on update for service to become ready.

a229976

The behaviour is now the same as for `kn service create`. This should also fix some flakiness in the tests again.

chore: Fixed changelog

4075fa0

chore(service): Adapt help message for update

d85f841

fix(test): Updated renamed method

c6a7997

rhuss force-pushed the pr/wait-for-update branch from c08d747 to c6a7997 Compare July 22, 2019 08:06

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2019

rhuss mentioned this pull request Jul 22, 2019

Improves(e2e): improves integration tests and to better run locally #274

Merged

cppforlife reviewed Jul 22, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2019

knative-prow-robot merged commit 875e8da into knative:master Jul 22, 2019

cppforlife reviewed Jul 22, 2019

View reviewed changes

rhuss mentioned this pull request Jul 25, 2019

Add E2E tests for Service, Revision, Route #291

Merged

navidshaikh mentioned this pull request Nov 15, 2019

With sync update, get revision name with latestReadyRevisionName #505

Closed

navidshaikh added a commit to navidshaikh/client that referenced this pull request Feb 28, 2020

Remove -parallel=1 flag from go test (knative#271)

7bb2bb5

let the defaults spin

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Temporarily remove ci-knative-build-pipeline-release (knative#271)

1aa31db

Due to tektoncd/pipeline#253

dsimansk added a commit to dsimansk/client that referenced this pull request Oct 3, 2023

[release-v1.10] Add kn-plugin-source-kafka v1.10.0 (knative#271)

ea605ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(service): Wait on update for service to become ready. #271

feature(service): Wait on update for service to become ready. #271

rhuss commented Jul 18, 2019

navidshaikh Jul 18, 2019

rhuss Jul 18, 2019

rhuss commented Jul 18, 2019

navidshaikh Jul 18, 2019

knative-prow-robot commented Jul 18, 2019

rhuss commented Jul 18, 2019

maximilien Jul 18, 2019 •

edited

Loading

rhuss Jul 18, 2019

maximilien Jul 19, 2019

cppforlife Jul 22, 2019

rhuss Jul 22, 2019

maximilien commented Jul 18, 2019

sixolet commented Jul 19, 2019

maximilien commented Jul 19, 2019

maximilien commented Jul 19, 2019

rhuss commented Jul 20, 2019

rhuss commented Jul 22, 2019

knative-metrics-robot commented Jul 22, 2019

rhuss commented Jul 22, 2019

cppforlife Jul 22, 2019

rhuss Jul 22, 2019

cppforlife Jul 22, 2019

rhuss Jul 22, 2019

cppforlife Jul 22, 2019

rhuss Jul 22, 2019

sixolet commented Jul 22, 2019

cppforlife Jul 22, 2019

rhuss Jul 22, 2019

sixolet commented Jul 22, 2019 via email

feature(service): Wait on update for service to become ready. #271

feature(service): Wait on update for service to become ready. #271

Conversation

rhuss commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Jul 18, 2019

Choose a reason for hiding this comment

knative-prow-robot commented Jul 18, 2019

rhuss commented Jul 18, 2019

maximilien Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien commented Jul 18, 2019

sixolet commented Jul 19, 2019

maximilien commented Jul 19, 2019

maximilien commented Jul 19, 2019

rhuss commented Jul 20, 2019

rhuss commented Jul 22, 2019

knative-metrics-robot commented Jul 22, 2019

rhuss commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixolet commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixolet commented Jul 22, 2019 via email

maximilien Jul 18, 2019 •

edited

Loading