API for docker container and volumes #22
Conversation
There was a problem hiding this comment.
Good job! General Comments:
- Push changes in
go.modandgo.sum - Please provide documentation on the APIs and the existing test plans
- Add some comments in the code for logic that may be tricky to understand or ambigious
- Add more logging messages (i.e. if we enter error branch, we should log explicitly what is the error)
- Avoid using
panicfor code that will go into prod. but rather handle them in the caller
src/images/serve_image.go
Outdated
| } | ||
| } | ||
|
|
||
| func (mgr *ContainerMgr) stopContainer(containerID string) { |
There was a problem hiding this comment.
I think we should return status when stopContainer exits
elwincheng
force-pushed
the
elwin_dev_container_squashed
branch
from
641739c to
258ce6e
Compare
@KCui0327 Sorry I am not understanding. I am acquiring and releasing a mutex lock when calling volume/container functions. |
ah I see it now, I saw there was no diff in the review so I thought you didn't add it, I will re-review my apologies. But yeah I think for next iterations, please do not amend then force push, it makes it hard to see diffs between revisions |
src/images/README.md
Outdated
|
|
||
| --- | ||
|
|
||
| ### `func (mgr *ContainerMgr) runContainerCuda(volumeName string) (string, error)` |
There was a problem hiding this comment.
there is no runContainerCuda in the code, could you please double check
There was a problem hiding this comment.
Thanks that is a good catch
elwincheng
force-pushed
the
elwin_dev_container_squashed
branch
from
271c05d to
5f9b42f
Compare
|
I had to force push because I rebased off main cause I thought I needed Tara's commit for slog mb. |
src/images/serve_image_test.go
Outdated
| return NewContainerMgr(cli, 10, 100) | ||
| } | ||
|
|
||
| // T1: create a volume, check exists, delete, check not exists |
src/images/serve_image.go
Outdated
| return "", fmt.Errorf("volume %s does not exist", volumeName) | ||
| } | ||
|
|
||
| resp, err := cli.ContainerCreate(ctx, &container.Config{ |
| return resp.ID, err | ||
| } | ||
|
|
||
| io.Copy(os.Stdout, out) |
There was a problem hiding this comment.
don't dump container output to console - @KCui0327 should be another task to log this better? - we probably want container logs user accessible, no?
There was a problem hiding this comment.
Yes that is correct, we can add that in a separate PR
4 participants
elwin@DESKTOP-46267GC:~/dev/Mist/src/images$ go test
2025/09/15 18:52:19 WARN Volume does not exist volumeName=nonexistent_volume_t4
2025/09/15 18:52:21 ERROR Failed to remove volume volumeName=test_volume_t5 error="Error response from daemon: remove test_volume_t5: volume is in use - [b3dc76981255bcae4547ef997970146d478c2c8dcb5ed1f88f44c69a48d8412a, ec89342dfcb1a93ae63f0b0ad93b3d38cfa0b7ee1561825a7e34f8f3f53269cf]"
2025/09/15 18:52:30 ERROR Failed to remove volume volumeName=test_volume_t5 error="Error response from daemon: remove test_volume_t5: volume is in use - [b3dc76981255bcae4547ef997970146d478c2c8dcb5ed1f88f44c69a48d8412a]"
2025/09/15 18:52:53 ERROR Failed to remove volume volumeName=test_volume_t7 error="Error response from daemon: remove test_volume_t7: volume is in use - [6cb2b2cc2d799d0e89b5598a0d1df347cde053e485def4c0d7ba14e187d12acc, 1f00b4847e15778db0976b305976981b5a8d49e2821fa6af0ce1f583ab645d92, 8826fc56492f60d4f60bd3e79311dc9d317320da3e0fb3d6d682ba80694f8a19]"
2025/09/15 18:53:14 WARN Volume limit reached limit=100
2025/09/15 18:53:22 WARN Container limit reached limit=10
2025/09/15 18:54:59 ERROR Failed to remove volume volumeName=test_volume_t10 error="Error response from daemon: remove test_volume_t10: volume is in use - [da4c01ee51ae2939f5f6935f04cdb0c93ef125d5910432b81b35cd664e030243, c2cd99981648f58a85ac61857d14c1139f690ba9da335448cd58d4e4214344ec, d46c023de92057fa0f5984500be0f55185f101830f2274e7be3a38c6128bfe7d, a25e7bcac907360331392f5f1a19b4a49dff798193f1ffb64a0f116e59888313, 315aebee4f5aa3f5e9f98a21b0ca0cbca439c44bde3cd4a811c741a52561f0f6, 18a9cbbf388cfd72a83be637deeae7ddd11bbf33f7aeb3fa8acfa0b5d51ef42b, c0e8771cf2583b1187a713a114c183a78d234ec9b819d405b92a383dd91a7346, 1e7a8bb38c07e3887968490c819439778cf8ba076643316736970708697a99dc, 4e8c0a7bd18bbca04f1cd509ee575f1e26bdab78109d4970dd0a4f5cc850b2b8, 34667eb684c077cae995c5a6177c8d9687fff2c8561822cdf470c61a8cbacac2, d5245332e4b810511144700cf1c14ee5be590da05f0e79dbb101d5b81e5da54a, 2155a5d5af438b0656ca81521ade194ac3dc15df46692b67fce66a4040e3b842, 1552b023fabbcc0c7994738e375398ce6b9a26b1015833231131965b54c783bc, 6290884ae1e77e0f0e6bf88717342f18eb848395cea64fecb31d7bb5e69c51c3]"
PASS
ok mist/images 170.630s