Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(service): Improvements for waiting on service readiness #431

Merged
merged 1 commit into from
Oct 10, 2019
Merged

chore(service): Improvements for waiting on service readiness #431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@
| 🎁
| Update build.sh -w to add a message when compilation succeeded
| https://github.com/knative/client/pull/432[#432]

| 🧽
| Add more progress information during service create/update
| https://github.com/knative/client/pull/431[#431]

|===

## v0.2.0 (2019-07-10)
Expand Down
2 changes: 1 addition & 1 deletion docs/cmd/kn_service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ kn service create NAME --image IMAGE [flags]
--requests-memory string The requested memory (e.g., 64Mi).
--revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
--service-account string Service account name to set. Empty service account name will result to clear the service account.
--wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60)
--wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 600)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to also have a --no-wait?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

600 seconds == 10 minutes. Is that too much for a default? I feel like one minute (60 seconds) should be good enough. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had 60s before, but that was often too short, especially when the created Pods still have to pull the image, which is often longer than one minute. In that case a "timeout error" occurs as in #423, which is confusing as the service itself is ready eventually.

As I'm assuming that as long the "Ready" condition is in the state "UNKNOWN" I rely on Knative that this eventually will be reconciled to either "TRUE" or "FALSE". If this is the case, the command will return. It will only run into a timeout when staying in UNKNOWN. So from my experience, we should rely on Knative serving to do that reconciliation and e.g. if it can't get the service up in a certain amount of time, will be put the "Ready" condition to "FALSE". Hence this large timeout, which could maybe even longer (not sure if there are image that large that it will take longer than 10 mins, but then a sync wait on the CLI without a real progress meter is tedious, too).

I added the detailed feedback on the event as a poor-men progress meter replacement, so that the user sees that something is going on even when the creation/update takes a bit longer.

That output is also helpful for scripting use cases as it helps in debugging timing issues.

Still, with a -q we should certainly not show that messages.

One can argue that one migt move these event to --verbose, but then one would lose the simple progress indicator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--no-wait is achieved with --async (that was chosen over no-wait at the time when the feature was introduced)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I can live with that. Thanks.

```

### Options inherited from parent commands
Expand Down
88 changes: 45 additions & 43 deletions pkg/kn/commands/service/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ import (
"knative.dev/serving/pkg/apis/serving"

"github.com/spf13/cobra"
serving_v1alpha1_api "knative.dev/serving/pkg/apis/serving/v1alpha1"

corev1 "k8s.io/api/core/v1"
api_errors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
serving_kn_v1alpha1 "knative.dev/client/pkg/serving/v1alpha1"
serving_v1alpha1_api "knative.dev/serving/pkg/apis/serving/v1alpha1"
)

var create_example = `
Expand Down Expand Up @@ -95,59 +95,57 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
return err
}

out := cmd.OutOrStdout()
if serviceExists {
if !editFlags.ForceCreate {
return fmt.Errorf(
"cannot create service '%s' in namespace '%s' "+
"because the service already exists and no --force option was given", name, namespace)
}
err = replaceService(client, service, namespace, cmd.OutOrStdout())
err = replaceService(client, service, waitFlags, out)
} else {
err = createService(client, service, namespace, cmd.OutOrStdout())
err = createService(client, service, waitFlags, out)
}
if err != nil {
return err
}

if !waitFlags.Async {
out := cmd.OutOrStdout()
err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
if err != nil {
return err
}
return showUrl(client, name, namespace, out)
}

return nil
},
}
commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false)
editFlags.AddCreateFlags(serviceCreateCommand)
waitFlags.AddConditionWaitFlags(serviceCreateCommand, 60, "Create", "service")
waitFlags.AddConditionWaitFlags(serviceCreateCommand, 600, "Create", "service")
return serviceCreateCommand
}

// Duck type for writers having a flush
type flusher interface {
Flush() error
}

func flush(out io.Writer) {
if flusher, ok := out.(flusher); ok {
flusher.Flush()
func createService(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, waitFlags commands.WaitFlags, out io.Writer) error {
err := client.CreateService(service)
if err != nil {
return err
}

return waitIfRequested(client, service, waitFlags, "Creating", "created", out)
}

func createService(client v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, namespace string, out io.Writer) error {
err := client.CreateService(service)
func replaceService(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, waitFlags commands.WaitFlags, out io.Writer) error {
err := prepareAndUpdateService(client, service)
if err != nil {
return err
}
fmt.Fprintf(out, "Service '%s' successfully created in namespace '%s'.\n", service.Name, namespace)
return nil
return waitIfRequested(client, service, waitFlags, "Replacing", "replaced", out)
}

func waitIfRequested(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, waitFlags commands.WaitFlags, verbDoing string, verbDone string, out io.Writer) error {
if waitFlags.Async {
fmt.Fprintf(out, "Service '%s' %s in namespace '%s'.\n", service.Name, verbDone, client.Namespace())
return nil
}

fmt.Fprintf(out, "%s service '%s' in namespace '%s':\n", verbDoing, service.Name, client.Namespace())
return waitForServiceToGetReady(client, service.Name, waitFlags.TimeoutInSeconds, verbDone, out)
}

func replaceService(client v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, namespace string, out io.Writer) error {
func prepareAndUpdateService(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service) error {
var retries = 0
for {
existingService, err := client.GetService(service.Name)
Expand Down Expand Up @@ -185,11 +183,29 @@ func replaceService(client v1alpha1.KnServingClient, service *serving_v1alpha1_a
}
return err
}
fmt.Fprintf(out, "Service '%s' successfully replaced in namespace '%s'.\n", service.Name, namespace)
return nil
}
}

func waitForServiceToGetReady(client serving_kn_v1alpha1.KnServingClient, name string, timeout int, verbDone string, out io.Writer) error {
err := waitForService(client, name, out, timeout)
if err != nil {
return err
}
return showUrl(client, name, "", verbDone, out)
}

// Duck type for writers having a flush
type flusher interface {
Flush() error
}

func flush(out io.Writer) {
if flusher, ok := out.(flusher); ok {
flusher.Flush()
}
}

func serviceExists(client v1alpha1.KnServingClient, name string) (bool, error) {
_, err := client.GetService(name)
if api_errors.IsNotFound(err) {
Expand Down Expand Up @@ -228,17 +244,3 @@ func constructService(cmd *cobra.Command, editFlags ConfigurationEditFlags, name
}
return &service, nil
}

func showUrl(client v1alpha1.KnServingClient, serviceName string, namespace string, out io.Writer) error {
service, err := client.GetService(serviceName)
if err != nil {
return fmt.Errorf("cannot fetch service '%s' in namespace '%s' for extracting the URL: %v", serviceName, namespace, err)
}
url := service.Status.URL.String()
if url == "" {
url = service.Status.DeprecatedDomain
}
fmt.Fprintln(out, "\nService URL:")
fmt.Fprintf(out, "%s\n", url)
return nil
}
6 changes: 4 additions & 2 deletions pkg/kn/commands/service/create_mock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package service

import (
"testing"
"time"

"gotest.tools/assert"
corev1 "k8s.io/api/core/v1"
Expand All @@ -27,6 +28,7 @@ import (

servinglib "knative.dev/client/pkg/serving"
knclient "knative.dev/client/pkg/serving/v1alpha1"
"knative.dev/client/pkg/wait"

"knative.dev/client/pkg/util"
)
Expand All @@ -43,14 +45,14 @@ func TestServiceCreateImageMock(t *testing.T) {
// Create service (don't validate given service --> "Any()" arg is allowed)
r.CreateService(knclient.Any(), nil)
// Wait for service to become ready
r.WaitForService("foo", knclient.Any(), nil)
r.WaitForService("foo", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
// Get for showing the URL
r.GetService("foo", getServiceWithUrl("foo", "http://foo.example.com"), nil)

// Testing:
output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(output, "created", "foo", "http://foo.example.com", "Waiting"))
assert.Assert(t, util.ContainsAll(output, "Creating", "foo", "http://foo.example.com", "Ready"))

// Validate that all recorded API methods have been called
r.Validate()
Expand Down
10 changes: 5 additions & 5 deletions pkg/kn/commands/service/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ func fakeServiceCreate(args []string, withExistingService bool, sync bool) (

func getServiceEvents(name string) []watch.Event {
return []watch.Event{
{watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "")},
{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "")},
{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "")},
{watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")},
{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", "msg2")},
{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
}
}

Expand Down Expand Up @@ -147,11 +147,11 @@ func TestServiceCreateImageSync(t *testing.T) {
if template.Spec.Containers[0].Image != "gcr.io/foo/bar:baz" {
t.Fatalf("wrong image set: %v", template.Spec.Containers[0].Image)
}
if !strings.Contains(output, "foo") || !strings.Contains(output, "created") ||
if !strings.Contains(output, "foo") || !strings.Contains(output, "Creating") ||
!strings.Contains(output, commands.FakeNamespace) {
t.Fatalf("wrong stdout message: %v", output)
}
if !strings.Contains(output, "OK") || !strings.Contains(output, "Waiting") {
if !strings.Contains(output, "Ready") {
t.Fatalf("not running in sync mode")
}
}
Expand Down
17 changes: 11 additions & 6 deletions pkg/kn/commands/service/list_mock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@ package service
import (
"strings"
"testing"
"time"

"gotest.tools/assert"
"k8s.io/apimachinery/pkg/api/errors"
knclient "knative.dev/client/pkg/serving/v1alpha1"
"knative.dev/client/pkg/util"
"knative.dev/client/pkg/wait"

"knative.dev/serving/pkg/apis/serving/v1alpha1"
)

Expand All @@ -32,17 +35,17 @@ func TestServiceListAllNamespaceMock(t *testing.T) {

r.GetService("svc1", nil, errors.NewNotFound(v1alpha1.Resource("service"), "svc1"))
r.CreateService(knclient.Any(), nil)
r.WaitForService("svc1", knclient.Any(), nil)
r.WaitForService("svc1", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
r.GetService("svc1", getServiceWithNamespace("svc1", "default"), nil)

r.GetService("svc2", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo"))
r.CreateService(knclient.Any(), nil)
r.WaitForService("svc2", knclient.Any(), nil)
r.WaitForService("svc2", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
r.GetService("svc2", getServiceWithNamespace("svc2", "foo"), nil)

r.GetService("svc3", nil, errors.NewNotFound(v1alpha1.Resource("service"), "svc3"))
r.CreateService(knclient.Any(), nil)
r.WaitForService("svc3", knclient.Any(), nil)
r.WaitForService("svc3", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
r.GetService("svc3", getServiceWithNamespace("svc3", "bar"), nil)

r.ListServices(knclient.Any(), &v1alpha1.ServiceList{
Expand All @@ -55,15 +58,17 @@ func TestServiceListAllNamespaceMock(t *testing.T) {

output, err := executeServiceCommand(client, "create", "svc1", "--image", "gcr.io/foo/bar:baz", "--namespace", "default")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(output, "created", "svc1", "default", "Waiting"))
assert.Assert(t, util.ContainsAll(output, "Creating", "svc1", "default", "Ready"))

r.Namespace("foo")
output, err = executeServiceCommand(client, "create", "svc2", "--image", "gcr.io/foo/bar:baz", "--namespace", "foo")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(output, "created", "svc2", "foo", "Waiting"))
assert.Assert(t, util.ContainsAll(output, "Creating", "svc2", "foo", "Ready"))

r.Namespace("bar")
output, err = executeServiceCommand(client, "create", "svc3", "--image", "gcr.io/foo/bar:baz", "--namespace", "bar")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(output, "created", "svc3", "bar", "Waiting"))
assert.Assert(t, util.ContainsAll(output, "Creating", "svc3", "bar", "Ready"))

output, err = executeServiceCommand(client, "list", "--all-namespaces")
assert.NilError(t, err)
Expand Down
28 changes: 22 additions & 6 deletions pkg/kn/commands/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"knative.dev/client/pkg/kn/commands"
serving_kn_v1alpha1 "knative.dev/client/pkg/serving/v1alpha1"
"knative.dev/client/pkg/wait"

"fmt"

Expand All @@ -45,14 +46,29 @@ func NewServiceCommand(p *commands.KnParams) *cobra.Command {
}

func waitForService(client serving_kn_v1alpha1.KnServingClient, serviceName string, out io.Writer, timeout int) error {
fmt.Fprintf(out, "Waiting for service '%s' to become ready ... ", serviceName)
flush(out)

err := client.WaitForService(serviceName, time.Duration(timeout)*time.Second)
fmt.Println("")
err, duration := client.WaitForService(serviceName, time.Duration(timeout)*time.Second, wait.SimpleMessageCallback(out))
if err != nil {
fmt.Fprintln(out)
return err
}
fmt.Fprintln(out, "OK")
fmt.Fprintf(out, "%7.3fs Ready to serve.\n", float64(duration.Round(time.Millisecond))/float64(time.Second))
return nil
}

func showUrl(client serving_kn_v1alpha1.KnServingClient, serviceName string, originalRevision string, what string, out io.Writer) error {
service, err := client.GetService(serviceName)
if err != nil {
return fmt.Errorf("cannot fetch service '%s' in namespace '%s' for extracting the URL: %v", serviceName, client.Namespace(), err)
}
url := service.Status.URL.String()
if url == "" {
url = service.Status.DeprecatedDomain
}
revisionUpdateStatus := ""
newRevision := service.Status.LatestReadyRevisionName
if originalRevision != "" && originalRevision == newRevision {
revisionUpdateStatus = " (unchanged)"
}
fmt.Fprintf(out, "\nService '%s' %s with latest revision '%s'%s and URL:\n%s\n", serviceName, what, newRevision, revisionUpdateStatus, url)
return nil
}
12 changes: 8 additions & 4 deletions pkg/kn/commands/service/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
return err
}
service = service.DeepCopy()

latestRevisionBeforeUpdate := service.Status.LatestReadyRevisionName
var baseRevision *v1alpha1.Revision
if !cmd.Flags().Changed("image") && editFlags.LockToDigest {
baseRevision, err = client.GetBaseRevision(service)
if _, ok := err.(*serving.NoBaseRevisionError); ok {
fmt.Fprintf(cmd.OutOrStdout(), "Warning: No reivision found to update image digest")
fmt.Fprintf(cmd.OutOrStdout(), "Warning: No revision found to update image digest")
}
}
err = editFlags.Apply(service, baseRevision, cmd)
Expand All @@ -112,15 +112,19 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
return err
}

out := cmd.OutOrStdout()
if !waitFlags.Async {
out := cmd.OutOrStdout()
fmt.Fprintf(cmd.OutOrStdout(), "Updating Service '%s' in namespace '%s':\n", args[0], namespace)
err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
if err != nil {
return err
}
return showUrl(client, name, latestRevisionBeforeUpdate, "updated", out)
} else {
fmt.Fprintf(out, "Service '%s' updated in namespace '%s'.\n", args[0], namespace)

}

fmt.Fprintf(cmd.OutOrStdout(), "Service '%s' updated in namespace '%s'.\n", args[0], namespace)
return nil
}
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/kn/commands/service/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ func TestServiceUpdateImageSync(t *testing.T) {
assert.NilError(t, err)

assert.Equal(t, template.Spec.Containers[0].Image, "gcr.io/foo/quux:xyzzy")
assert.Assert(t, util.ContainsAll(strings.ToLower(output), "update", "foo", "service", "namespace", "bar", "ok", "waiting"))
assert.Assert(t, util.ContainsAll(strings.ToLower(output), "updating", "foo", "service", "namespace", "bar", "ready"))
}

func TestServiceUpdateImage(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/kn/commands/testing_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func CreateTestKnCommand(cmd *cobra.Command, knParams *KnParams) (*cobra.Command
fakeServing := &fake.FakeServingV1alpha1{&client_testing.Fake{}}
knParams.Output = buf
knParams.NewClient = func(namespace string) (v1alpha1.KnServingClient, error) {
return v1alpha1.NewKnServingClient(fakeServing, namespace), nil
return v1alpha1.NewKnServingClient(fakeServing, FakeNamespace), nil
}
knParams.fixedCurrentNamespace = FakeNamespace
knCommand := NewKnTestCommand(cmd, knParams)
Expand Down
Loading