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.

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

Support multiple docker socket #2471

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docker-compose/glances.conf
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,8 @@ max_name_size=20
all=False
# Define Podman sock
#podman_sock=unix:///run/user/1000/podman/podman.sock
# Define Docker sock
docker_sock=unix:///var/run/docker.sock
Copy link
Owner

Choose a reason for hiding this comment

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

It will be better to let default value of docker_sock to empty (not defined) in order to use (by default) the standard docker.from_env() builder.
Glances is multi-platform and i am pretty sure that the default value of docker_sock on Windows is not unix:///var/run/docker.sock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for this


[amps]
# AMPs configuration are defined in the bottom of this file
Expand Down
19 changes: 11 additions & 8 deletions glances/plugins/containers/engines/docker.py
Original file line number Diff line number Diff line change
Expand Up @@ -215,25 +215,26 @@ class DockerContainersExtension:

CONTAINER_ACTIVE_STATUS = ['running', 'paused']

def __init__(self):
def __init__(self, server_urls:str|list = 'unix:///var/run/docker.sock'):
Copy link
Owner

Choose a reason for hiding this comment

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

Same remark, let the server_urls default value to '' (empty).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just something to note with Python support.

Glances v4 would be target Py3.8+

The str | list syntax was only support from Py3.10 onwards, iirc.
So better use the older Union[] style to prevent older interpreters from crashing.

Also I think returning a list[str] even when there is only a single elemenet would reduce unnecessary type checks later

if import_docker_error_tag:
raise Exception("Missing libs required to run Docker Extension (Containers) ")

self.client = None
self.clients = None
self.ext_name = "containers (Docker)"
self.stats_fetchers = {}

self.connect()
self.connect(server_urls)

def connect(self):
def connect(self, base_urls:str|list = 'unix:///var/run/docker.sock'):
Copy link
Owner

Choose a reason for hiding this comment

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

Idem

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as typing issue above

"""Connect to the Docker server."""
# Init the Docker API Client
base_urls = [base_urls] if isinstance(base_urls, str) else base_urls
try:
# Do not use the timeout option (see issue #1878)
self.client = docker.from_env()
Copy link
Owner

Choose a reason for hiding this comment

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

test if base_urls is empty then use from_env() else use the base_urls list instead

self.clients = [docker.DockerClient(url) for url in base_urls]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will end up failing/dropping all socket connections if any one of them fail.

I think a more tolerant approach where we just ignore the connections that fail would be better as they user will still get to see some of the containers if they are active.

A log message indicating the failed socket would be helpful for debugging

except Exception as e:
logger.error("{} plugin - Can't connect to Docker ({})".format(self.ext_name, e))
self.client = None
self.clients = None

def update_version(self):
# Long and not useful anymore because the information is no more displayed in UIs
Expand All @@ -248,7 +249,7 @@ def stop(self):
def update(self, all_tag):
"""Update Docker stats using the input method."""

if not self.client:
if not self.clients:
return {}, []

version_stats = self.update_version()
Expand All @@ -257,7 +258,8 @@ def update(self, all_tag):
try:
# Issue #1152: Docker module doesn't export details about stopped containers
# The Containers/all key of the configuration file should be set to True
containers = self.client.containers.list(all=all_tag)
containers = []
[[containers.append(container_per_client) for container_per_client in client.containers.list(all=all_tag)] for client in self.clients]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same problem as mentioned earlier, if one of the socket connections break or the server ends up serving a malformed response, then we will end up with no results.

except Exception as e:
logger.error("{} plugin - Can't get containers list ({})".format(self.ext_name, e))
return version_stats, []
Expand Down Expand Up @@ -299,6 +301,7 @@ def generate_stats(self, container):
# Container Status (from attrs)
'Status': container.attrs['State']['Status'],
'Created': container.attrs['Created'],
'Socket_URL': container.client.api.base_url,
'Command': [],
}

Expand Down
22 changes: 20 additions & 2 deletions glances/plugins/containers/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ def __init__(self, args=None, config=None):
self.display_curse = True

# Init the Docker API
self.docker_extension = DockerContainersExtension() if not import_docker_error_tag else None
self.docker_extension = DockerContainersExtension(server_urls=self._docker_sock()) if not import_docker_error_tag else None

# Init the Podman API
if import_podman_error_tag:
self.podman_client = None
else:
self.podman_client = PodmanContainersExtension(podman_sock=self._podman_sock())
self.podman_client = PodmanContainersExtension(podman_sock=self._podman_sock()) # TODO: podman also
Copy link
Owner

Choose a reason for hiding this comment

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

IS it also possible to have multiple socket for Podman ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nicolargo
Yep.
In fact its more common to have custom socket's in the podman ecosystem.


# Sort key
self.sort_key = None
Expand All @@ -91,6 +91,16 @@ def __init__(self, args=None, config=None):
self.update()
self.refresh_timer.set(0)

def _docker_sock(self):
"""Return the docker socks.
Default value: unix:///var/run/docker.sock
"""
conf_docker_sock = self.get_conf_value('docker_sock')
if len(conf_docker_sock) == 0:
return "unix:///var/run/docker.sock"
Copy link
Owner

Choose a reason for hiding this comment

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

To be refactor according to previous comments

else:
return conf_docker_sock

def _podman_sock(self):
"""Return the podman sock.
Could be desfined in the [docker] section thanks to the podman_sock option.
Expand Down Expand Up @@ -297,6 +307,8 @@ def msg_curse(self, args=None, max_width=None):
ret.append(self.curse_add_line(msg))
msg = ' {:<7}'.format('Tx/s')
ret.append(self.curse_add_line(msg))
msg = '{:<30}'.format('Socket URL')
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also should display this only optionally. As in the user can opt in if they want this to be displayed.

The containers plugin has very little screen room, so this will reduce it even further

ret.append(self.curse_add_line(msg))
msg = ' {:8}'.format('Command')
ret.append(self.curse_add_line(msg))

Expand Down Expand Up @@ -381,6 +393,12 @@ def msg_curse(self, args=None, max_width=None):
except KeyError:
msg = ' {:<7}'.format('_')
ret.append(self.curse_add_line(msg))
# Socket URL
if container['Socket_URL'] is not None:
msg = '{:<30}'.format(container['Socket_URL'])
else:
msg = '{:<30}'.format('_')
ret.append(self.curse_add_line(msg, splittable=True))
# Command
if container['Command'] is not None:
msg = ' {}'.format(' '.join(container['Command']))
Expand Down