-
Notifications
You must be signed in to change notification settings - Fork 624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand docker version evaluation. #4834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @donaldguy
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
To expound a little/summarize #4827:
As such the hardcoding of a check on docker version in this specific case is a rather course instrument, but I suspect it may come up again in the future. A more appropriate specific solution might be to (simply) bubble this error: supervisor/supervisor/docker/network.py Lines 116 to 122 in 480b383
more aggressively as ~fatal in the case of any essential component (supervisor, observer, dns?, etc.) or more aggressively attempt to do discovery and routing to ~misplaced components |
Warns about home-assistant#4827 as well as potential future regressions pushing a same-tree commit to see if that continues past the now satisfied CLA check
a995a27
to
1bc5b2b
Compare
async def test_evaluation(coresys: CoreSys): | ||
"""Test evaluation.""" | ||
async def test_evaluation_supported(coresys: CoreSys): | ||
"""Test real evaluation with a current docker daemon.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Test real evaluation with a current docker daemon.""" | |
"""Test evaluation with a known good docker version.""" |
I kinda thought at one point it would be good to just let it hit the real thing, but I could not finagle getting pytest not to mock it out.
From an architecture side, would it not make more sense to just decide on a few main version and support those only? Same as what happens with Python versions today. |
I wouldn't disagree, and obvs the defacto over in hassos land is that there is a single pin at a time I would say broadly that:
and/but then speaking as a I am willing to have HASSIO holding-back this machine from taking on newer docker engine, but I am pretty peeved to have it break on That said, my suspicion is that HA et al are facing down a bigger container runtime reckoning sooner-rather-than later, with cgroups v1 officially officially deprecated (for sometime-this-year removal) in systemd 255 and with that a question of e.g. jumping ship(/"jumping whale") from docker/moby entirely to podman, or systemd-nspawn, or whatever. It did not escape my notice that the So ... there's that. |
Wrt close: A bit on the fence weather we should add this code, since with 25.0.1 this is kinda already in the past at this point. On the other hand, maybe it indeed helps for the future. Question: Did you test that with 25.0.0? Did the message appear early enough so it would have been useful for people? |
The issue with this approach is in order for anyone to see this message we have to update supervisor. Like if Docker has a bad version in the future, using this as a mechanism for messaging means we need to urgently get a new version of supervisor to stable with this evaluation updated and the new bad version listed. We can't anticipate future bad versions of docker so existing versions of supervisor won't be aware to warn people. Even then it would still only help people that had not updated docker yet. People that have updated docker already have unhealthy systems and so are pretty stuck and unlikely to be able to update supervisor to receive this message. I think #355 is the better approach. The supervised installer should probably pin to a known good version and not simply install the latest. In addition if a known bad version of docker remains out as latest for a longer time we do actually have a way to get messages out to users without needing to update Supervisor or Home Assistant - this is why https://alerts.home-assistant.io/ was created. We can create a new alert and ensure anyone with a supervised type installation sees it as alert in their Home Assistant so they are aware of the potential issue. This seems like a better approach then handling it in supervisor here to me. |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
EDIT: apparently contrary to my impression of their plans, they cut a docker 25.0.1 this morning, so this is kinda for no one now. The broader idea could still be helpful in future, but lmk if just want to close
Proposed change
Expand the
EvaluateDockerVersion
to include both a minimum version check and a list of known bad versions for poor supervised-installer users. (I would also accept, even perhaps prefer e.g. a constraint in https://github.com/home-assistant/supervised-installer/blob/main/homeassistant-supervised/DEBIAN/control but this is already here)expand test suite accordingly
do away with the apparently otherwise unused
supported_version
property of DockerInfo, concentrating that logic in the evaluation itselfType of change
Additional information
As I discussed at #4827 (comment)
there is already a fix landed upstream in moby for this ~bug but they are not interested in fast-tracking a patch release apparently.
Ditto is perhaps foolish to suspect a supervisor release would be forthcoming either, but I'd already spent ~5 hours struggling with debugging this issue myself (errantly over-focalizing on pyudev getting denied on
udev_monitor_new_from_netlink
... for some reason), so figured might as well spend 2 hours more closing the loopChecklist
The code change is tested and works locally.
I have verified that the additions do not hamper supervisor (and enough else for the setup wizard to showup at
http://localhost:9123
) startup on docker 24.0.7 in devcontainer (running under orbstack on an M1 MacBook air).I have not yet investigated exact behavior/ordering of failures during startup under docker-ce 25.0.0
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (
black --fast supervisor tests
)Tests have been added to verify that the new code works.