Add Running filter when stopping containers attached to a volume and rename functions for better clarity #93
Conversation
…rename functions for better clarity Signed-off-by: Sahil Soni <SscSPs@gmail.com>
There was a problem hiding this comment.
LGTM!
I've tested it by exporting a volume that is shared across 2 containers:
# 1. Launch a container that is running forever
docker run -d --name ctr-1 -v my-vol:/tmp alpine sleep infinity.
# 2. Launch a container that is stopped immediately after.
docker run -d --name ctr-2 -v my-vol:/tmp alpine /bin/sh
# 3. From the extension, export the volume "my-vol".
# 4. Only container "ctr-1" was stopped before exporting, then started again once the operation was completed. The container "ctr-2" remained stopped as it was originally.
vm/internal/backend/containers.go
Outdated
| var stoppedContainersByExtension []string | ||
| var timeout = 10 * time.Second | ||
|
|
||
| containerNames := GetContainersForVolume(ctx, cli, volumeName) | ||
| containerNames := GetContainersForVolume(ctx, cli, volumeName, filters.NewArgs(filters.Arg("status", "running"))) |
There was a problem hiding this comment.
nit: From the list of status, I wonder if we should also filter "restarting" containers. When restarting, and during the backup process, they could also alter the data.
And could "removing" containers also add or remove data from an attached volume?
There was a problem hiding this comment.
Yeah, I had the same thought, there's a paused state as well, i am not sure how's the behaviour of the state transitions are, i can add these 3 as well in the filters,
Possible status: created, restarting, running, removing, paused, exited, or dead
I think we can take into consideration these 4: restarting, running, removing, paused.
In case of restarting, we can start it just fine,
And in case of removing, we can wait it's removal?
For paused,I'm not sure how we should handle it, although since it's paused, there shouldn't be a risk of data corruption, so we can probably ignore it.
There was a problem hiding this comment.
In case of restarting, we can start it just fine.
I don't know if it is possible that a restarting container can write data into the volume. I guess yes, it could generate some logs that could be written into the volume.
So, when the backup is done, yes I agree, starting it seams fine.
However, if there is a container that is restarting when starting the backup, it could add data to the attached volume. So the container must be stopped.
And in case of removing, we can wait it's removal?
Looks like a good idea.
For paused,I'm not sure how we should handle it, although since it's paused, there shouldn't be a risk of data corruption, so we can probably ignore it.
I agree
There was a problem hiding this comment.
so I added changes to support restarting and removing statuses,
for restarting, we just treat it as running, stop it before cloning, and start it after cloning completes.
for removing, wait for removal to complete before we finish the StopRunningContainersAttachedToVolume function
Signed-off-by: Sahil Soni <SscSPs@gmail.com>
Signed-off-by: Sahil Soni <SscSPs@gmail.com>
|
Hi @SscSPs and thank you for stellar your contribution! In order to send you some Hacktoberfest-related swag, could you please send me an email to alba.roza@docker.com with your postal address and your t-shirt size? Thank you so much! |
closes #91
Signed-off-by: Sahil Soni SscSPs@gmail.com