pod_watch: only emit events from pods whose specs we have deployed since startup#2439
Conversation
|
|
||
| require.Equal(f.T(), 1, len(entities), "expected only one entity. Yaml contained: %s", f.k8s.Yaml) | ||
|
|
||
| d := entities[0].Obj.(*v1.Deployment) |
There was a problem hiding this comment.
maybe d, ok := here and require.True(t, ok, "expected entity to be a deployment") -- if it's NOT a deployment for some reason, this is easier to debug than the weird nil pointer errors you'll get otherwise
There was a problem hiding this comment.
(but also why are you assuming the first entity is always a deployment?)
There was a problem hiding this comment.
maybe d, ok := here and require.True(t, ok, "expected entity to be a deployment") -- if it's NOT a deployment for some reason, this is easier to debug than the weird nil pointer errors you'll get otherwise
how about require.IsType?
also why are you assuming the first entity is always a deployment?
More like "assuming the entity is always a deployment" - this is after requiring that there's exactly one entity. All the tests that hit this code only have one entity, which is a deployment. I added a comment above the length assertion to make it easier for someone to understand what's going on if they add/modify a test such that that's no longer true.
|
|
||
| if ok { | ||
| set, ok := podTemplateSpecHashes[mn] | ||
| if ok && !hasOKPodTemplateSpecHash(pod, set) { |
There was a problem hiding this comment.
maybe worth a descriptive comment e.g. "the pod template hash doesn't correspond to the most recently deployed one--this pod is probably from an old Tilt run" or something.
There was a problem hiding this comment.
Do you think the existing comment on hasOKPodTemplateSpecHash suffices?
Or do you want to add a descriptive comment to each of its call sites?
There was a problem hiding this comment.
oh whoops, i missed that. ya that's fine.
|
what's the status of this pr? lost track of whether it's tabled, or needs review, or waiting to be merged, something else? |
When I left yesterday, it was awaiting approval. I failed to ping you for approval yesterday after addressing your comments. |
|
|
||
| if ok { | ||
| set, ok := podTemplateSpecHashes[mn] | ||
| if ok && !hasOKPodTemplateSpecHash(pod, set) { |
There was a problem hiding this comment.
oh whoops, i missed that. ya that's fine.
3 participants
Problem
When a user starts Tilt, Tilt will build a new image with a new hash, create new Deployment YAML, and apply it. If there is already a deployment running, this application will edit the existing deployment, leaving the UID unchanged. Thus, when k8s tells Tilt about the pod the deployment had before Tilt even started, Tilt says "hey, I know that deployment's UID!" and treats it as its own.
This has a few concrete minor effects:
tilt upd withkubectl applyand knows the Deployment's UID and the moment k8s has started deploying the new pod and told Tilt about it. (because the old pod matches the deployment and is healthy)Solution
Inject a hash of the pod template spec into every pod template spec we deploy. Then, similar to DeployedUIDSet, track those IDs for each manifest and only pay attention to updates for pods whose template specs we've deployed since startup.