feat: bubble up image size as pod events #106
Conversation
vadasambar
changed the title
vadasambar
changed the title
vadasambar
changed the title
feat: implement logic for warner - pass down max image size to the warner refactor: pass max size directly to warner - change pkg name `imagesizewarner` -> `imagesize` test: add test cases for image size refactor: add helpful logs - fix segfault if warm-metal fails to fetch size from the container registry
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), ctxTimeout) | ||
| defer cancel() | ||
| // TODO(vadasambar): set a timeout for the context here |
There was a problem hiding this comment.
Is this already fixed?
| repo := p.image.Name() | ||
| imageSpec := &cri.ImageSpec{Image: p.image.String()} | ||
| creds, withCredentials := p.keyring.Lookup(repo) | ||
| if !withCredentials { | ||
|
|
||
| p.checkForImageSize(nil, pod, ns) |
There was a problem hiding this comment.
Would it be possible and helpful to add tests for getting image size with and without creds?
| Pod: req.VolumeContext[ctxKeyPodName], | ||
| Namespace: req.VolumeContext[ctxKeyPodNamespace], |
There was a problem hiding this comment.
Adding these might also help fix the bug in current logs where pod and namespace are empty in the log messages for printing image size.
| if current.Cmp(*w.maxImageSize) <= 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Is this where we check if maxImageSize flag is set, and if not then returning without errors?
There was a problem hiding this comment.
If yes, then maybe we should add this check early before calling GetImageSize() function. That would reduce the additional work on the driver if the flag is not enabled.
| // fetchImageSize gets the image size from the container registry | ||
| func (w *warner) fetchImageSize(c client.RegistryClient, image reference.Named) (*resource.Quantity, error) { | ||
| var mlist []manifesttypes.ImageManifest | ||
| imageManifest, err := c.GetManifest(context.Background(), image) |
There was a problem hiding this comment.
I see that we are setting timeout on the context only in the warn() function and not here. Is this intentional?
| var parsedSize *resource.Quantity | ||
|
|
||
| for _, m := range mlist { | ||
| // TODO(vadasambar): support other OS platforms |
There was a problem hiding this comment.
Do we want to fix this in the same PR or create a separate issue for tracking this?
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| func TestWarn(t *testing.T) { |
There was a problem hiding this comment.
It would be nice if we could add tests for parsing maxImageSize with invalid or empty inputs.
Co-authored-by: Mugdha Adhav <[email protected]>
Co-authored-by: Mugdha Adhav <[email protected]>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. |
|
Closing this PR after a prolonged period of inactivity. Please create a new PR if the changes of the PR are still relevant. |
Problem
If the size of the image that warm-metal is pulling is too big, image pulling (and mounting) can fail. The user does not understand why the image pulling and mounting failed.
Solution
This PR introduces a flag
--max-image-sizewhich can be set to any value like1Mi,1Gi,10Gietc., Warm-metal driver will get the image size from the registry and if the image size exceeds the--max-image-size, it will create anEventon the pod where the volume is being mounted.This is what it looks like in action:
The flag has no impact on mounting and unmounting. If there is a failure to fetch image size info from the registry, warm-metal just logs the error (it doesn't terminate the driver).