Send progress updates during image pull for install/update #6102
Conversation
mdegat01
added
missing-documentation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
supervisor/docker/interface.py
Outdated
| # Hopefully these come in order but if they sometimes get out of sync, avoid accidentally going backwards | ||
| # If it happens a lot though we may need to reconsider the value of this feature | ||
| if job.done: | ||
| _LOGGER.debug( | ||
| "Received pull image log with status %s for job %s but job was done, skipping", | ||
| reference.status, | ||
| job.uuid, | ||
| ) | ||
| return | ||
|
|
||
| if job.stage and stage < PullImageLayerStage.from_status(job.stage): | ||
| _LOGGER.debug( | ||
| "Received pull image log with status %s for job %s but job was already on stage %s, skipping", | ||
| reference.status, | ||
| job.uuid, | ||
| job.stage, | ||
| ) | ||
| return |
There was a problem hiding this comment.
Absent from a Docker bug, I can't see how this would happen. We connect through a unix socket which is not packet based but a stream, it guarantee things being in order (just like TCP). So unless Docker sends it in the wrong order, or our bus reorder things, this really should never happen!
I'd not make this a debug log, it is very unlikely we'd ever notice this. I'd raise severity to error here, and maybe even send to Sentry so we can start investigate why that happens.
There was a problem hiding this comment.
Absent from a Docker bug, I can't see how this would happen.
I agree. This wasn't actually what I was worried about though. We are reading in these logs in the executor. We are then scheduling a task on the normal event loop per log called fire_event. Then that is scheduling another task on the normal event loop per listener listening for that event to process it.
My concern is that all this task scheduling will cause us to process them out of order. Not that docker will actually give them to us out of order.
I'd raise severity to error here, and maybe even send to Sentry
I'd rather only send it to sentry tbh. Given what we're talking about I don't think the impact on the user is enough to raise it to their attention. If they miss a few of the 100 ms progress updates that really doesn't matter much. But we would like to know about it. Maybe time to make use of that capture_event API in Sentry rather then capture_exception
There was a problem hiding this comment.
Ok added a new type of exception for this called DockerLogOutOfOrder. Raising and capturing it in sentry.
I'm still logging at debug level for now because I don't think users really need to know about this. Let me know if you want me to bump the severity in addition to capturing it in sentry.
There was a problem hiding this comment.
That is fine by me, with the Sentry capture we'll definitely notice if that happens in practice 👍 .
mdegat01
force-pushed
the
image-install-progress-updates
branch
from
3ba493d to
70ddf1d
Compare
mdegat01
removed
missing-documentation
Proposed change
When pulling an image during an install or update of something, create a sub job per layer and use that to report progress on that layer. This way Home Assistant receives progress updates for an image pull and would be able to present that in the UI.
This isn't a perfect solution since we're still unable to report on the overall progress of the final image since we don't know how big each layer is until we get there. But it does provide a lot of progress information and could be used to provide users with some sense of progress during these tasks, unlike today.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed: