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

Conversation

loopback-kr
Copy link

Description

Docker can separate docker context by user. That is, we can have multiple docker.sock files per user. The /var/run/docker.sock file is default socket file for default context, but run/user/<USER-ID>/docker.sock file also can exists for each user account, and the user can switch context and use docker in that context. eg, docker rootless mode, but i think glances binds only one docker.sock file through the volume option when create a new container of glances.

I wish glances could take multiple docker.sock files and monitor containers per context when glances.conf is set as below.

[containers]
# Define Podman sock
#podman_sock=unix:///run/user/1000/podman/podman.sock
# Define Docker sock
#docker_sock=unix:///var/run/docker.sock
docker_sock=unix:///run/user/1000/docker.sock,unix:///run/user/1001/docker.sock

I just push a draft version for CLI mode.
The web interface and Podman parts also need to be implemented.
I hope it's worth it. Thanks.

Resume

  • Bug fix: no
  • New feature: yes
  • Fixed tickets: comma-separated list of tickets fixed by the PR, if any

Copy link
Owner

@nicolargo nicolargo left a comment

Choose a reason for hiding this comment

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

Hi @loopback-kr !

Thanks for the interesting PR.

For the moment, i can not merge it to the develop branch. I have added some comments inside your code.

Nicolas

@@ -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

@@ -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


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

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


# 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.

"""
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


def connect(self):
def connect(self, base_urls:str|list = '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.

Same as typing issue above

@@ -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
Collaborator

Choose a reason for hiding this comment

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

+1 for this

@@ -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
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

try:
# Do not use the timeout option (see issue #1878)
self.client = docker.from_env()
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

@@ -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.


# 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
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.

@@ -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

@RazCrimson
Copy link
Collaborator

RazCrimson commented Jul 22, 2023

@loopback-kr
I'll take care of the clean up and merge, if you don't mind.
Please let me know if you want to get this done by yourself.

Thanks for the contribution!

@loopback-kr
Copy link
Author

There is not enough time for this work. I would be happy if you could complete the code for me.

@RazCrimson RazCrimson added this to the Glances 4.1.0 milestone May 7, 2024
@nicolargo
Copy link
Owner

@RazCrimson Do you think that this PR can be rejected ?

@RazCrimson
Copy link
Collaborator

@RazCrimson Do you think that this PR can be rejected ?

I'll go ahead fix up the changes and update the PR by 4.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants