Skip to content

Wait timeout fix#237

Merged
dramich merged 2 commits intorancher:masterfrom
rmweir:wait-timeout-fix
Jul 25, 2019
Merged

Wait timeout fix#237
dramich merged 2 commits intorancher:masterfrom
rmweir:wait-timeout-fix

Conversation

@rmweir
Copy link
Contributor

@rmweir rmweir commented Jul 18, 2019

Problem:
Some apps require wait and timeout flags to be passed to helm. CLI does not support these flags for apps.

Solution:
Accept flags for wait and timeout and then pass them to helm.

Depends on following PRs:
types - rancher/types#911
rancher - rancher/rancher#21604

intended merge order:
types -> rancher -> cli

Issue:
rancher/rancher#17471

@rmweir rmweir force-pushed the wait-timeout-fix branch 6 times, most recently from 1c30d58 to 56ace39 Compare July 19, 2019 18:26
@rmweir rmweir changed the title WIP Wait timeout fix Wait timeout fix Jul 19, 2019
@rmweir rmweir force-pushed the wait-timeout-fix branch from 56ace39 to 2e83a29 Compare July 20, 2019 00:40
Copy link
Contributor

@cbron cbron left a comment

Choose a reason for hiding this comment

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

Seems fine to me, show-notes worked.
Did an install with 1-second timeout (./rancher app install --wait --timeout 1 redis) and got:

[kube] 2019/07/22 13:24:20 creating 5 resource(s)
[kube] 2019/07/22 13:24:20 beginning wait for 5 resources with timeout of 1s
[kube] 2019/07/22 13:24:21 Pod is not ready: redis-0f4gh/a-svj2n-redis-master-0
[tiller] 2019/07/22 13:24:21 warning: Release "a-svj2n" failed: timed out waiting for the condition
[storage] 2019/07/22 13:24:21 updating release "a-svj2n.v1"
[tiller] 2019/07/22 13:24:21 failed install perform step: release a-svj2n failed: timed out waiting for the condition
2019/07/22 13:24:21 [ERROR] AppController p-tnfj6/a-svj2n [helm-controller] failed with : failed to install app a-svj2n. Error: release a-svj2n failed: timed out waiting for the condition

@@ -177,6 +177,15 @@ func AppCommand() cli.Command {
Name: "no-prompt",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add an example in description that includes these flags

@rmweir rmweir force-pushed the wait-timeout-fix branch from 2e83a29 to 5bf671b Compare July 22, 2019 23:28
cmd/app.go Outdated
break
}
}
fmt.Printf("run \"app show-notes %s\" to view app notes once app is ready", madeApp.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested and it works, looks good except that I think it should have newline at end. If there is ever another print statement below it then both would be on same line. Current output:
run "app show-notes a-gjlhj" to view app notes once app is ready%

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

@rmweir rmweir force-pushed the wait-timeout-fix branch from 5bf671b to 005371e Compare July 23, 2019 16:01
app.TargetNamespace = namespace
}

app.Wait = ctx.Bool("wait")
Copy link
Contributor

Choose a reason for hiding this comment

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

should wait boolflag have a default of true? false? what happens if you set app.Wait to nil or whatever comes through from the not setting BoolFlag Value?

Copy link
Contributor Author

@rmweir rmweir Jul 23, 2019

Choose a reason for hiding this comment

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

default on bool flags is false, which is intended. I tested without using wait.

Copy link
Contributor

Choose a reason for hiding this comment

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

then we're good

@luthermonson
Copy link
Contributor

lgtm

Added wait and timeout flags. Added show-notes flag. Prior, there
was no way to tell helm to wait longer than the default of 300
seconds. This caused installs of certain apps to fail. Now, users
can configure the amount of time helm should wait before failing.
CLI no longer hangs while waiting for helm. User can use the
existing wait command for hanging behavior. Notes may not be ready
immediately, so user can now use the show-notes flag to print app
notes at any time after app creation.
@rmweir rmweir force-pushed the wait-timeout-fix branch from 005371e to 05449f4 Compare July 24, 2019 22:46
@dramich
Copy link
Contributor

dramich commented Jul 25, 2019

lgtm

@dramich dramich merged commit 6100ce4 into rancher:master Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants