feat: interpret all docker states with shutdown #311
Conversation
| std::process::exit(0); | ||
| } else { | ||
| std::thread::spawn(|| { |
There was a problem hiding this comment.
You are not waiting on this task, so will it actually end?
There was a problem hiding this comment.
I tested it, and it ends when the application exits normally, or it does its thing after 60s
| std::process::exit(0); | ||
| } else { | ||
| std::thread::spawn(|| { | ||
| std::thread::sleep(std::time::Duration::from_secs(60)); |
There was a problem hiding this comment.
As per above, we need ample time for the application to shutdown normally before the failsafe kicks in forcefully.
cli/src/supervisor.rs
Outdated
| self.dashboard.take(); | ||
| ctx.shutdown(); | ||
| tokio::time::sleep(Duration::from_secs(10)).await; |
There was a problem hiding this comment.
Is there an event we can wait for instead of this fixed time?
There was a problem hiding this comment.
For now, this seems to work fine. I did start a separate branch to get a tokio oneshot channel going as I could not find any events wired into here, but that will take some time to complete.
There was a problem hiding this comment.
Agree with Mike. You can get feedback that the docker process has exited, rather than wait for nnn seconds to close the app
There was a problem hiding this comment.
tbc, I'm ok to merge this behaviour now, but really need to get the proper shutdown behaviour in as a follow up
There was a problem hiding this comment.
I expanded the Docker status mapping and now properly handle ContainerState::NotFound during shutdown, which in turn enabled me to remove the sleep-when-exit statements in the launchpad front end.
| @@ -153,9 +153,14 @@ impl Default for Status { | |||
|
|
|||
| #[derive(Debug, PartialEq, Eq)] | |||
| pub enum ContainerState { | |||
| Empty, | |||
There was a problem hiding this comment.
What is the empty state? Would this be the equivalent to the previous NotFound ?
There was a problem hiding this comment.
Possibly similar to NotFound, but I wouldn't be able to say. No explanation about Empty in https://docs.rs/bollard/latest/bollard/models/enum.ContainerStateStatusEnum.html. I also did not pick it up in the log files.
There was a problem hiding this comment.
Actually, your questions made me rethink my shutdown proposal, and ContainerState::NotFound should not be removed. I expanded the Docker status mapping and now properly handle ContainerState::NotFound during shutdown. I could also remove the sleep-when-exit statements in the launchpad front end.
When the containers are running fn inspect_container will return some status, but when they are removed, for example during shutdown, it will return an error, which we then map to ContainerState::NotFound.
Added code to interpret all docker states and act accordingly. This is to prevent Docker going into undefined states with undefined behaviour.
hansieodendaal
force-pushed
the
ho_docker_states
branch
from
57ec478
to
988086c
Compare
hansieodendaal
force-pushed
the
ho_docker_states
branch
from
988086c
to
b880812
Compare
Description
Motivation and Context
All docker states were not interpreted and this caused docker containers to be left in undefined states when exiting.
How Has This Been Tested?
System-level tests on Windows and Ubuntu-WSL