imageservice: Add context to various methods#44365
Conversation
vvoland
added
status/2-code-review
area/images
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
Being a context plumber ain't easy, that's for sure!
| ch := daemon.usage.DoChan("ContainerDiskUsage", func() (interface{}, error) { | ||
| // Retrieve container list | ||
| containers, err := daemon.Containers(&types.ContainerListOptions{ | ||
| containers, err := daemon.Containers(context.TODO(), &types.ContainerListOptions{ | ||
| Size: true, | ||
| All: true, | ||
| }) |
There was a problem hiding this comment.
package resenje.org/singleflight might be of help here.
There was a problem hiding this comment.
Thanks, look good. Do you mind addressing this in a follow up? The behaviour here doesn't change, and I would like to avoid blowing up this PR with a new package being vendored.
|
Thanks for a great analysis @corhere, I addressed your comments. |
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
Looking great! There's just one place where error-matching needs to be fixed up to handle wrapped errors.
|
Thanks! I just squashed the fix-up commits. |
| // TODO(mlaventure): we need to specify checkpoint options here | ||
| tsk, err := ctr.Start(ctx, checkpointDir, | ||
| container.StreamConfig.Stdin() != nil || container.Config.Tty, | ||
| tsk, err := ctr.Start(context.TODO(), // Passing ctx to ctr.Start caused integration tests to be stuck in the cleanup phase |
There was a problem hiding this comment.
I see this one was added in 4bafaa0#diff-034d55db8b341bd127a5394a84226b04c91d54669dd856051ceb0bcbbb50b380 (#43564)
Perhaps we should create a tracking ticket for this one to see if we can find out what's causing the actual issue.
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
changes look good; left some comments about commits that should ideally be squashed (let me know if you need help on that)
| // TODO(mlaventure): we need to specify checkpoint options here | ||
| tsk, err := ctr.Start(ctx, checkpointDir, | ||
| container.StreamConfig.Stdin() != nil || container.Config.Tty, | ||
| tsk, err := ctr.Start(context.TODO(), // Passing ctx to ctr.Start caused integration tests to be stuck in the cleanup phase |
There was a problem hiding this comment.
I see this one was added in 4bafaa0#diff-034d55db8b341bd127a5394a84226b04c91d54669dd856051ceb0bcbbb50b380 (#43564)
Perhaps we should create a tracking ticket for this one to see if we can find out what's causing the actual issue.
Co-authored-by: Paweł Gronowski <[email protected]> Signed-off-by: Paweł Gronowski <[email protected]>
This caused integration tests to timeout in the CI Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
|
Thanks @thaJeztah. Squashed most of the commits 😄 |
Signed-off-by: Paweł Gronowski <[email protected]>
6 participants
Replaces:
- What I did
I tracked down the integration test hang and reverted the changes which caused it.
I will be doing some further investigations to find out exactly why passing the given context causes this - but for now I propose to keep the existing behaviour of passing
context.TODOtocontainer.Runas this will unblock us in upstreaming the containerd integration changes.- How I did it
Bisecting changes from the original commit
- How to verify it
Check if integration tests don't timeout
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)