Skip to content

engine: add timeout for service watch#1181

Merged
jazzdan merged 7 commits into
masterfrom
dmiller/add_timeout_for_service_watch
Feb 28, 2019
Merged

engine: add timeout for service watch#1181
jazzdan merged 7 commits into
masterfrom
dmiller/add_timeout_for_service_watch

Conversation

@jazzdan

@jazzdan jazzdan commented Feb 19, 2019

Copy link
Copy Markdown
Contributor

This may fix a problem Frank saw with Tilt hanging forever upon start when his Kube cluster wasn't ready

@jazzdan
jazzdan requested review from landism and maiamcc February 19, 2019 18:46

@maiamcc maiamcc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⌚️ 👀

defer cancel()
ch, err := w.kCli.WatchServices(ctx2, []model.LabelPair{k8s.TiltRunLabel()})
if err != nil {
err = errors.Wrap(err, "Error watching services. Are you connected to kubernetes?\n")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Are you connected to Kubernetes" is not a helpful error message if we timed out, I might remove that bit and trust the err to contain the relevant context.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fwiw, i thought we added that precisely because we got user reports that they didn't understand the error message. there are a wide variety of weird error messages you can get from this function

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, but if we get this error due to a timeout, it doesn't imply that we're not connected to k8s, right, but rather that something ELSE is wrong? in that case, the hint "are you connected to k8s" sends the user down a rabbit hole.

defer cancel()
ch, err := w.kCli.WatchServices(ctx2, []model.LabelPair{k8s.TiltRunLabel()})
if err != nil {
err = errors.Wrap(err, "Error watching services. Are you connected to kubernetes?\n")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fwiw, i thought we added that precisely because we got user reports that they didn't understand the error message. there are a wide variety of weird error messages you can get from this function

ch, err := w.kCli.WatchServices(ctx, []model.LabelPair{k8s.TiltRunLabel()})
ctx2, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
ch, err := w.kCli.WatchServices(ctx2, []model.LabelPair{k8s.TiltRunLabel()})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you also update WatchPods? there's no guarantee which one will run first

@jazzdan
jazzdan force-pushed the dmiller/add_timeout_for_service_watch branch 2 times, most recently from b3ee398 to 9d68f2e Compare February 25, 2019 23:31
@jazzdan
jazzdan force-pushed the dmiller/add_timeout_for_service_watch branch from 9d68f2e to b49e29c Compare February 26, 2019 15:17
@jazzdan
jazzdan force-pushed the dmiller/add_timeout_for_service_watch branch from cb3dd78 to 8c65ed4 Compare February 28, 2019 03:16
@jazzdan
jazzdan merged commit 4eab45d into master Feb 28, 2019
@nicks
nicks deleted the dmiller/add_timeout_for_service_watch branch March 31, 2021 16:59
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.

3 participants