Skip to content

rollout: make clear when tilt is attaching to an existing pod rather than rolling out a new one#4570

Merged
landism merged 3 commits into
masterfrom
matt/4566
May 26, 2021
Merged

rollout: make clear when tilt is attaching to an existing pod rather than rolling out a new one#4570
landism merged 3 commits into
masterfrom
matt/4566

Conversation

@landism

@landism landism commented May 19, 2021

Copy link
Copy Markdown
Member

Fixes #4566

Problems

  1. When you run tilt up and the existing pod suffices, the tilt log still makes it look like it's rolling out a new pod, which is confusing:
Tracking new pod rollout (matt-fortune-6b8dc4dc74-24llm):
     ┊ Scheduled       - <1s
     ┊ Initialized     - <1s
     ┊ Ready           - 2s
  1. On subsequent builds where the existing pod suffices, tilt will just omit that section entirely and not log anything in its place, which feels kind of weird (e.g, "why hasn't it rolled the pod out yet?"), and also leaves the user in the position of seeing runtime logs from the pod without seeing any of the startup logs they might expect, with no explanation
     2b7fb36704d4: Layer already exists 
     c1df871adf38: Layer already exists 
     503e53e365f3: Layer already exists 

STEP 3/3 — Deploying
     Injecting images into Kubernetes YAML
     Applying via kubectl:
     → myprefix-matt-vigoda:deployment

     Step 1 - 0.76s (Building Dockerfile: [vigoda])
     Step 2 - 0.12s (Pushing localhost:50380/vigoda:tilt-bfed53112d6c0507)
     Step 3 - 0.01s (Deploying)
     DONE IN: 0.89s 

2021/05/19 20:47:20 Server status: All good
2021/05/19 20:47:32 Server status: All good

Solution

Have the rollout logger explicitly log when Tilt is using an existing pod instead of rolling out a new one, and do it on every build, e.g.:

STEP 3/3 — Deploying
     Injecting images into Kubernetes YAML
     Applying via kubectl:
     → myprefix-matt-vigoda:deployment

     Step 1 - 0.67s (Building Dockerfile: [vigoda])
     Step 2 - 0.09s (Pushing localhost:50380/vigoda:tilt-bfed53112d6c0507)
     Step 3 - 0.01s (Deploying)
     DONE IN: 0.77s 


Using existing pod that matches spec (myprefix-matt-vigoda-5c75bf9577-mb47n)
2021/05/19 21:02:32 Server status: All good

note this also affects live update, which is probably fine? (also this means the update log now has the pod id, which I think is nice)

STEP 1/1 — updating image localhost:50380/fortune
     Updating container: c796b906b3
Will copy 1 file(s) to container: c796b906b3
- '/Users/matt/go/src/github.com/tilt-dev/servantes/fortune/main.go' --> '/go/src/github.com/tilt-dev/servantes/fortune/main.go'
[CMD 1/3] sh -c cd src/github.com/tilt-dev/servantes/fortune && make proto
protoc --go_out=. -I. api/fortune.proto
[CMD 2/3] sh -c go install github.com/tilt-dev/servantes/fortune
[CMD 3/3] sh -c date > /tmp/.restart-proc
  → Container c796b906b3 updated!

     Step 1 - 0.59s (updating image localhost:50380/fortune)
     DONE IN: 0.59s 


Using existing pod that matches spec (myprefix-matt-fortune-6b8dc4dc74-7zq9p)
2021/05/19 21:03:28 Starting Fortune Service on :8082!!
  1. It feels a bit weird to have this line hanging out here by itself and it'd probably be better to put in the build log, but this was cheap and is probably better than status quo? (and implementation-wise it's nice to have this logic in the same place for rollout vs attach)
  2. I don't think "Using existing pod that matches spec" will do a great job of getting the point across, but I've struggled to come up with anything better, and would appreciate suggestions!

@landism
landism requested review from milas and nicks May 19, 2021 21:10
Comment thread internal/engine/k8srollout/podmonitor.go Outdated
Comment thread internal/engine/k8srollout/podmonitor.go Outdated
@hyu

hyu commented May 24, 2021

Copy link
Copy Markdown
Contributor

Just noting here for posterity that Matt and I discussed the wording of "Using existing pod that matches spec" on Slack, and I couldn't really come up with anything definitively better.

  • I wanted more similarity to the related message of "Tracking new pod rollout", but that was tricky to do since we're no longer tracking a new rollout but using an existing pod.
  • My proposal of "Found existing pod" doesn't make it clear whether Tilt is making use of or replacing it or what
  • I did like Matt's idea of "Existing pod still matches build (podid)" but by this point I'm not sure if I can even judge objectively anymore

Naming is hard 😞

@landism
landism merged commit 93c8908 into master May 26, 2021
@landism
landism deleted the matt/4566 branch May 26, 2021 19:07
landism pushed a commit that referenced this pull request May 28, 2021
… rather than rolling out a new one (#4570)"

This reverts commit 93c8908.
landism added a commit that referenced this pull request May 28, 2021
… rather than rolling out a new one (#4570)" (#4599)

This reverts commit 93c8908.
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.

confusing tilt output when attaching to an existing pod

3 participants