engine: Handle pod crashes when there are multiple containers#1953
Conversation
5b59a49 to
4179073
Compare
| assert.Equal(t, 2, len(ms.LiveUpdatedContainerIDs)) | ||
| }) | ||
|
|
||
| // Restart one container id but not the other. |
There was a problem hiding this comment.
i don't think you mean "restart"...? maybe:
// Simulate pod event where one of the containers has been replaced
?
There was a problem hiding this comment.
AFAICT, "restart" is consistent with the terminology that Kubernetes uses
if you kill the main container process, Kubernetes creates a new container ID and increments the restartCount field to 1
There was a problem hiding this comment.
Ah. That makes sense -- but fwiw I read "restart" like restart_container, i.e. it's the same container, we just re-exec the entrypoint. So i might consider replacing cuz that word has a different meaning in our codebase. but nbd
| } | ||
| func checkForPodCrash(ctx context.Context, state *store.EngineState, ms *store.ManifestState, podInfo store.Pod) { | ||
|
|
||
| func checkForPodCrash(ctx context.Context, state *store.EngineState, mt *store.ManifestTarget) { |
There was a problem hiding this comment.
nit: we're not JUST checking for pod crash now -- it's pod-or-container-crash. idk what i'd rename it to tho
There was a problem hiding this comment.
changed to checkForContainerCrash
| } | ||
|
|
||
| // Track the container ids that have been live-updated whether the | ||
| // build succeeds or fails. |
There was a problem hiding this comment.
...why would we track a live-updated CID if the build failed?
There was a problem hiding this comment.
This is the distinction we spent a long time talking about on the other PR. If a user Run step fails on a syntax error, that is normal and expected. The container files have still diverged
There was a problem hiding this comment.
oh i see! well shoot, we don't return a result in that case :/ -- can you make a ticket for this?
There was a problem hiding this comment.
| // NOTE(maia): this is used only to populate ms.LiveUpdatedContainerID, and will | ||
| // be removed in http://bit.ly/2LFAPDb when we implement crash detection | ||
| // for multiple containers | ||
| func (set BuildResultSet) OneLiveUpdatedContainerIDForOldBehavior() container.ID { |
| f.assertAllBuildsConsumed() | ||
| } | ||
|
|
||
| func TestCrashRebuildTwoContainersOneImage(t *testing.T) { |
There was a problem hiding this comment.
is it worth testing the behavior when it's two containers on a single pod, each with a different image? should work the same, but maybe good for the sake of coverage.
There was a problem hiding this comment.
i thought that didn't work yet?
There was a problem hiding this comment.
OH! yeah that building behavior doesn't work, you're right -- was imaging that you could directly set up the pod/build-result state. but that's probably not worth it. leave a todo for that test tho, and i'll fill it in
There was a problem hiding this comment.
4179073 to
4b940a3
Compare
2 participants
Hello @maiamcc,
Please review the following commits I made in branch nicks/ch2905/crashes:
5b59a49 (2019-07-31 12:14:03 -0400)
engine: Handle pod crashes when there are multiple containers
6b9f39d (2019-07-30 18:49:10 -0400)
podbuilder: by default, use the image ref name from the YAML rather than making one up