Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Docker info more robust #3720

Merged
merged 1 commit into from Jul 7, 2022
Merged

Docker info more robust #3720

merged 1 commit into from Jul 7, 2022

Conversation

pvizeli
Copy link
Member

@pvizeli pvizeli commented Jul 7, 2022 •
edited

Proposed change

We should update min requirements to 20.10.0 of docker. Anyway this make it more stable between versions.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:
  • Link to cli pull request:

Checklist

  • The code change is tested and works locally.
  • 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.

If API endpoints of add-on configuration are added/changed:

@pvizeli pvizeli added the refactor A code change that neither fixes a bug nor adds a feature label Jul 7, 2022
@frenck
Copy link
Member

frenck commented Jul 7, 2022

Simple and effective 👍

@pvizeli pvizeli added bugfix A bug fix and removed refactor A code change that neither fixes a bug nor adds a feature labels Jul 7, 2022
ludeeus
ludeeus reviewed Jul 7, 2022
View reviewed changes
supervisor/docker/__init__.py
AwesomeVersion(data.get("ServerVersion", "0.0.0")),
data.get("Driver", "unknown"),
data.get("LoggingDriver", "unknown"),
data.get("CgroupVersion", "1"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 is what we want for cgroup version right?
Should we make this "0" to indicate a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is: It doesn't have to be a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its not a problem, this PR does not make sense

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case of cgroup, no cgroup means v1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the others can change on docker updates, at least we would start and not break

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, in future we support cgroup2 as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 things:

  1. Docker doesn't provide the info
  2. The cgroup version wrong

This mitigates 1 (preventing it from crashing), but it also means we don't know anything about 2. We can't tell if it is wrong or right

@pvizeli pvizeli merged commit a5cc3cb into main Jul 7, 2022
@pvizeli pvizeli deleted the rebust-docker branch July 7, 2022 08:01
@Joeviocoe

This comment was marked as off-topic.

Sign in to view
@frenck
Copy link
Member

frenck commented Jul 7, 2022 •
edited

This highlights the continued need to disable auto-updates for Supervisor

This highlight people need to run supported installation methods; as set out by the requirements and keep their systems up to date. This didn't show up in tests either, probably because everyone who did tests (and the beta channel) did have their systems in check.

If you want that level of control over your system, we suggest using a different installation method. For example, use the Home Assistant Container method instead.

Locking down this PR for further comments, as the above comment is not related to this PR review.

@home-assistant home-assistant locked as off-topic and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@frenck frenck frenck approved these changes

@ludeeus ludeeus ludeeus left review comments

Assignees
No one assigned
Labels
bugfix A bug fix cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pvizeli @frenck @Joeviocoe @ludeeus @homeassistant