get container metadata from docker metadata #210

rpitonak · 2018-05-16T12:51:44Z

get_container_metadata implemented in Docker backend
minor change in ContainerMetadata, port_mappings is now dict {"host_port": [list_of_container_ports]}
test is provided

We need to discuss the naming of methods here, get_metadata and get_container_metadata sounds similar what can be confusing for users.

TomasTomecek

Wow, pretty good progress!

TomasTomecek · 2018-05-18T09:07:19Z

conu/apidefs/metadata.py

@@ -47,8 +47,8 @@ def __init__(self, name=None, identifier=None, labels=None, command=None,
        :param env_variables: dict, {name: value}
        :param image: Image, reference to Image instance
        :param exposed_ports: list, list of exposed ports
-        :param port_mappings: dict, dictionary of port mappings {"host_port": "container_port"}, example:
-            - {"8080":"80"} map port 80 in the container to port 8080 on the host
+        :param port_mappings: dict, dictionary of port mappings {"host_port": ["container_port"]}, example:


Hm, this doesn't make much sense to me. How could one external port be mapped to two internal?

TomasTomecek · 2018-05-18T09:09:07Z

conu/backend/docker/container.py

@@ -475,3 +490,69 @@ def write_to_stdin(self, message):
            self.popen_instance.stdin.flush()
        except subprocess.CalledProcessError as e:
            raise ConuException(e)
+
+    def get_container_metadata(self):


I believe we discussed this with @dhodovsk and my suggestion was to break backwards compatibility: .get_metadata method would return instance of the Metadata class, and .inspect method would yield the dict from docker. WDYT?

Agreed, get_metadata() can be used for conu-metadata. I'd suggest adding new commit that would rename all get_metadata() to inspect().

TomasTomecek · 2018-05-18T09:09:30Z

conu/backend/docker/container.py

+
+        docker_metadata = self.get_metadata(refresh=True)
+
+        print(docker_metadata)


probably a leftover from debugging

TomasTomecek · 2018-05-18T09:14:08Z

conu/backend/docker/container.py

+        # ['DISTTAG=f26container', 'FGC=f26']
+        env_variables = dict()
+        for env_variable in docker_metadata['Config']['Env']:
+            env_variables.update({env_variable.split('=')[0]: env_variable.split('=')[1]})


I advise to do .split("=", 1) since = can be put also in the value; the other potential error could occur if the value is unset

Please add these two cases to tests.

Real example:

"Env": [ "ASD=", "A=B=C=D", "XYZ", "DISTTAG=f28container", "FGC=f28" ],

$ env A=B=C=D HOSTNAME=36256ed1fe0d PWD=/ HOME=/root DISTTAG=f28container ASD= FGC=f28 TERM=xterm SHLVL=1 PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin _=/usr/bin/env

TomasTomecek · 2018-05-18T09:15:15Z

conu/backend/docker/container.py

+        status = ContainerStatus.CREATED
+
+        if docker_metadata['State']['Status'] == 'created':
+            status = ContainerStatus.CREATED


I'm not sure whether we should copy all the possible status from docker just like that. I would love to discuss this next week during our call.

On meeting we decided on 4 states:

Running

Not-Running (Stopped, Created, Exited - with 0 code)

Unknown (Restarting, Terminating,...)

Failed

TomasTomecek · 2018-05-18T09:16:35Z

conu/backend/docker/container.py

+            status = ContainerStatus.DEAD
+
+        # format of Port mappings from docker inspect:
+        # {'12345/tcp': [{'HostIp': '0.0.0.0', 'HostPort': '123'}, {'HostIp': '0.0.0.0', 'HostPort': '1234'}]}


see, it's actually the other way around: multiple host ports can point to a single container port

TomasTomecek · 2018-05-18T09:17:14Z

tests/integration/test_docker.py

+        assert container_metadata.status == ContainerStatus.RUNNING
+
+        c.delete(force=True)


this should be in finally: block

rpitonak · 2018-05-18T18:00:07Z

Thank you for the review. I pushed some new changes.

We can discuss name of the methods and ContainerStatus on next meeting. I totally agree to not copy all fields from docker but make it more general.

dhodovsk · 2018-05-21T11:49:50Z

conu/backend/docker/container.py

@@ -475,3 +490,69 @@ def write_to_stdin(self, message):
            self.popen_instance.stdin.flush()
        except subprocess.CalledProcessError as e:
            raise ConuException(e)
+
+    def get_container_metadata(self):


Agreed, get_metadata() can be used for conu-metadata. I'd suggest adding new commit that would rename all get_metadata() to inspect().

dhodovsk · 2018-05-21T15:00:53Z

conu/backend/docker/container.py

+
+        status = ContainerStatus.CREATED
+
+        if docker_metadata['State']['Status'] == 'created':


This could be a method on ContainerStatus -> status = ContainerStatus.get(string)

dhodovsk · 2018-05-21T15:01:15Z

conu/backend/docker/container.py

+        for env_variable in docker_metadata['Config']['Env']:
+            try:
+                env_variables.update({env_variable.split('=', 1)[0]: env_variable.split('=', 1)[1]})
+            except IndexError:


Is this expected? If not, we should raise something.

dhodovsk · 2018-05-21T15:26:26Z

conu/backend/docker/container.py

+                if key in port_mappings.keys():
+                    port_mappings[key].append(int(item['HostPort']))
+                else:
+                    port_mappings.update({key: [int(item['HostPort'])]})


This fails when inspecting container that is created with docker run -p 80 nginx

dhodovsk · 2018-05-21T15:30:56Z

conu/backend/docker/container.py

+            creation_timestamp=docker_metadata['Created'],
+            env_variables=env_variables,
+            image=image,
+            exposed_ports=list(docker_metadata['Config']['ExposedPorts'].keys()),


When there are no exposed ports on container, this field is missing in inspect and get_container_metadata() fails on KeyError

dhodovsk · 2018-05-21T15:40:36Z

tests/integration/test_docker.py

+            assert container_metadata.hostname == "my_hostname"
+            assert "XYZ" not in list(container_metadata.env_variables.keys())
+            assert container_metadata.port_mappings["1234"] == "12345/tcp"


container_metadata.port_mappings actually returns {'12345/tcp': [1234, 123]} in this case.

psklenar · 2018-05-22T13:38:09Z

jenkins/superCI comment

Status: failure, return code = 1
Result url: https://da.gd/3sTbH
Comment: See details for logs

dhodovsk · 2018-05-23T07:56:41Z

The requested changes were addressed, it looks good! Thank you!

TomasTomecek · 2018-05-23T13:48:43Z

conu/backend/docker/container.py

@@ -433,7 +439,7 @@ def get_status(self):

        :return: one of: 'created', 'restarting', 'running', 'paused', 'exited', 'dead'
        """
-        return self.get_metadata(refresh=True)["State"]["Status"]
+        return self.inspect(refresh=True)["State"]["Status"]


Would be awesome to use .get_metadata for these. Not blocking this PR,

TomasTomecek requested changes May 18, 2018

View reviewed changes

user-cont deleted a comment May 18, 2018

dhodovsk requested changes May 21, 2018

View reviewed changes

rpitonak added 5 commits May 22, 2018 14:18

get container metadata from docker metadata

657ffa5

fix of enviroment variables and ports parsing

66f6d6f

remove leftover

31cf5e9

rename get_metadata() to inspect()

1dcdf21

fix in ports parsing, handling exceptions, ContainerStatus enum changed

d19accb

rpitonak force-pushed the metadataClass branch from 8d7eed8 to d19accb Compare May 22, 2018 13:19

user-cont deleted a comment May 22, 2018

fix of redefinitions

e56580b

rpitonak force-pushed the metadataClass branch from 88fac85 to e56580b Compare May 22, 2018 14:39

user-cont deleted a comment May 22, 2018

dhodovsk approved these changes May 23, 2018

View reviewed changes

TomasTomecek reviewed May 23, 2018

View reviewed changes

TomasTomecek approved these changes May 23, 2018

View reviewed changes

TomasTomecek merged commit 3dd9575 into user-cont:master May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get container metadata from docker metadata #210

get container metadata from docker metadata #210

rpitonak commented May 16, 2018

TomasTomecek left a comment

TomasTomecek May 18, 2018

TomasTomecek May 18, 2018

dhodovsk May 21, 2018

TomasTomecek May 18, 2018

TomasTomecek May 18, 2018

TomasTomecek May 18, 2018

dhodovsk May 22, 2018

TomasTomecek May 18, 2018

TomasTomecek May 18, 2018

rpitonak commented May 18, 2018 •

edited

dhodovsk May 21, 2018

dhodovsk May 21, 2018

dhodovsk May 21, 2018

dhodovsk May 21, 2018

dhodovsk May 21, 2018

dhodovsk May 21, 2018

psklenar commented May 22, 2018

dhodovsk commented May 23, 2018

TomasTomecek May 23, 2018


		docker_metadata = self.get_metadata(refresh=True)

		print(docker_metadata)

		assert container_metadata.status == ContainerStatus.RUNNING

		c.delete(force=True)


		status = ContainerStatus.CREATED

		if docker_metadata['State']['Status'] == 'created':

get container metadata from docker metadata #210

get container metadata from docker metadata #210

Conversation

rpitonak commented May 16, 2018

TomasTomecek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpitonak commented May 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psklenar commented May 22, 2018

jenkins/superCI comment

dhodovsk commented May 23, 2018

Choose a reason for hiding this comment

rpitonak commented May 18, 2018 •

edited