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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions supervisor/docker/__init__.py
View file
Open in desktop
Expand Up @@ -56,10 +56,10 @@ class DockerInfo:
def new(data: dict[str, Any]):
"""Create a object from docker info."""
return DockerInfo(
AwesomeVersion(data["ServerVersion"]),
data["Driver"],
data["LoggingDriver"],
data["CgroupVersion"],
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

)

@property
Expand Down