feat(service create): Wait for a service to be ready when its created

[rhuss]

• By default wait for a service to be in condition "Ready"
• Use "--async" for returning immediately (which is the current behaviour)
• --wait-timeout to specify how long to wait for Ready condition (in seconds, default: 60s)
• In sync mode, print out the service URL as a last line (so that it can be used together with tail -1) to extract the service URL after the service is created.
• Refactored service create code to use smaller methods which are easier to understand

Current tests has been adapted to work with async behaviour but no
new tests has been added for the new sync mode.
This is coming in a second commit (and put into an extra test file).

Fixes #54

[knative-prow-robot]

Hi @rhuss. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rhuss]

/ok-to-test

[rhuss]

/assign sixolet

[knative-prow-robot]

@rhuss: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rhuss]

Some implementation notes:

• This implementation uses a plain watch on the Services endpoint instead of a fully fledged Informer for simplicities reasons and because I don't consider the breakage of that watch connection as fatal. The service has been created anyway, its just a convenient method to wait until its ready.
• Printing out the URL at the end might be or might be not a good idea. On the one hand is nice to allow something like curl $(kn service create --image bla | tail -1) but in the case of an error, this usage pattern probably doesn't make so much sense. I'm undecided here, I probably would like a kn service show --url (or kn service describe --url) better.
• Blocking for a kn service create --force doesn't work probably now when the service already exists as the first status for the Ready condition will be true-true-unknown-true (as the first events are still about the update of the existing service). So because the code currently sees the true first, it already thinks that the service is ready, not knowing that it becomes unknown two events later. Not sure how to solve this problem easily. Any ideas ?

[adrcunha]

/retest

[cppforlife]

/ok-to-test

[rhuss]

hmm, the integration test failure doesn't seem to be related to this PR.

[rhuss]

hmm, the integration test failure doesn't seem to be related to this PR.

Nope, it's an issue with this PR. Going to fix it now.

[rhuss]

It looks like that something changes on latest in Knative serving. With 0.6.0 locally the e2etest runs smoothly.

I can fix that, but this, in general, raises the question against which version of knative serving we should test. And whether we should strive for testing against multiple Knative (0.5, 0.6, ...) versions. This becomes especially important when the internal structure changes.

I'm going to fix this for latest now and test it locally against 0.6.0, too (which is not covered then by the integration test).

[rhuss]

Integration test should be fixed if #155 is merged and this PR rebased on top of it

[rhuss]

@sixolet rebased and resolved conflicts. Please let me know whether I'm on the right track because then I would add more tests for the sync/async behaviour and also adapt the e2e test cases for the sync creation of the service.

[maximilien]

• Printing out the URL at the end might be or might be not a good idea. On the one hand is nice to allow something like curl $(kn service create --image bla | tail -1) but in the case of an error, this usage pattern probably doesn't make so much sense. I'm undecided here, I probably would like a kn service show --url (or kn service describe --url) better.

What would you print out if success? Same as before “Service Xyz created”? Then adding the URL on the next line for me is a great info to the user. This also does not prevent adding: kn service describe --url which is a great programmatic access to the URL.

• Blocking for a kn service create --force doesn't work probably now when the service already exists as the first status for the Ready condition will be true-true-unknown-true (as the first events are still about the update of the existing service). So because the code currently sees the true first, it already thinks that the service is ready, not knowing that it becomes unknown two events later. Not sure how to solve this problem easily. Any ideas ?

One idea if I understand the problem correctly is some kind of a wait limit/timeout should be applied at each Ready stage so that when unknown is reached it would have to timeout but hopefully a long enough that the next Ready == true stage is reached.

[maximilien]

I can fix that, but this, in general, raises the question against which version of knative serving we should test. And whether we should strive for testing against multiple Knative (0.5, 0.6, ...) versions. This becomes especially important when the internal structure changes.

I’d vote to stay with latest since we are still in pre-release mode.

[rhuss]

I extracted now the wait logic in a service-independent and also cobra-independent package (top-level wait).

The option is now renamed to --no-wait to fit nicely to the common pattern for gnu getopts where --no- prefix is used for negating binary options.

Also, one can switch the default mode, so when the default mode would be async, then an option --wait is added. This default behaviour could be a candidate to be configured in a ~/.kn/config.yml.

For now only --no-wait and --wait-timeout is enabled.

---

@sixolet I added the tested of configured generation vs. observed generation, but I never managed to get an event where those two values differ. I tried to find out, in which case these values can be different, and whether these values are already consolidated before the first event is sent. Do you have a pointer for me where I could learn a bit more about the usage of these generation values ?

Also, for extracting the generation values I used unstructured to work typeless on any resource. If this is not acceptable I can change that to an extractor callback based method like I do for getting to the conditions in a general fashion.

Otherwise, I added quite some tests, but I haven't touched the e2e tests yet, which at least could be optimized to remove the sleep values (which are not needed in because waiting is the default)

[maximilien]

Otherwise, I added quite some tests, but I haven't touched the e2e tests yet, which at least could be optimized to remove the sleep values (which are not needed in because waiting is the default)

And soon enough you should be able to do it in Golang :)

[cppforlife]

i would prefer if we change it to --wait=bool with a desired default.

[rhuss]

I change this. This --no-wait style was a reminiscence to GNU getopts where long boolean options are used to have --with- and --no- prefixes. But guess that's not so common anymore.

[rhuss]

@cppforlife @sixolet Just thought about the naming of the option again. If we make waiting the default value, then a --wait=true doesn't make much sense and doesn't provide any extra value. Also, having an option with a string parameter allows for additional errors like providing a non-standard value (like no, off, ....). So I'd really prefer a boolean option here.

My suggestion would be to fall back to the original suggestion of --async, or any other boolean toggle like --no-wait, --background, --apply, ...

[navidshaikh]

IMO, --no-wait reads simple and should be easy to use with default value of false.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

[knative-metrics-robot]

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_create.go	66.7%	69.0%	2.3
pkg/kn/commands/service/wait_args.go	Do not exist	80.0%
pkg/kn/commands/wait_flags.go	Do not exist	100.0%
pkg/wait/test_wait_helper.go	Do not exist	100.0%
pkg/wait/wait_for_ready.go	Do not exist	81.0%

[rhuss]

@cppforlife @sixolet I think we are ready to go and that I adressed all open change requests:

• Fixed the help message
• Added a retry mechanism when the list watch stops
• Changed option from --no-wait to --async (and removed --wait option)
• Removed the switchable default mode (e.g. that one could have used also an async behaviour as default)

One minor issue which I would adress after a merge of this PR is, whether and how we should show a service URL. I think its need that you see that URL when commands returns (and it can show the URL now as the route is then alreaday in place).

However, I'm not so sure about the how. I'd like that it would be easy to use the URL also in scripting so I though about a combination of --url and --quiet (from #182 )

• kn service create ... would not show the URL, just a success message.
• kn service create ... --url would should the URL as it is now
• kn service create ... --quiet would show nothing, but return with an error code
• kn service create ... --quiet --url would only show the URL.

The last command would make it super easy to use the URL in scripts so that you can do things like curl $(kn service create greeter --image greeter --quiet --url)

Alternative to this suggestion could be

• --url shows only the URL
• --quiet shows allways nothing
• Default would showing the URL as it is now

I think I like the second option better, but as mentioned I would address this in a seperate issue/PR as this PR already has grown quite a bit.

[maximilien]

/lgtm