Kubernetes container handler #68

ashcrow · 2016-12-06T22:52:23Z

Retooling the old Kubernetes container manager code for the new service.

ashcrow · 2016-12-06T22:52:34Z

(5) [multi-host-mgr-2] Create ContainerManager Service

ashcrow · 2016-12-07T20:18:27Z

@mbarnes this PR ready to have it's spelling mistakes found. 😆

mbarnes · 2016-12-07T20:32:40Z

@mbarnes this PR ready to have it's spelling mistakes found. 😆

I see one already. 😉

mbarnes

Looks good, just a few nits, some of which are deferrable.

mbarnes · 2016-12-07T20:38:40Z

src/commissaire/containermgr/kubernetes/__init__.py

+        if not part.startswith('/'):
+            self.logger.debug(
+                'Part given without starting slash. Adding...')
+            part = '/{}'.format(part)


nit: This could just be part = '/' + part. Optional; not gonna block over it.

mbarnes · 2016-12-07T20:49:38Z

src/commissaire/containermgr/kubernetes/__init__.py

+        :returns: requests.Response
+        """
+        part = self._fix_part(part)
+        payload = json.dumps(payload)


nit: Type-flipping is kinda sneaky; I'd prefer a different name, like payload_str.

mbarnes · 2016-12-07T20:51:38Z

src/commissaire/containermgr/kubernetes/__init__.py

@@ -93,6 +93,22 @@ def check_config(cls, config):
                    'Server URL scheme must be "https" when using client '
                    'side certificates (got "{}")'.format(url.scheme))

+    def _fix_part(self, part):
+        """
+        Fixes part if it doesn't start with a slash.


Clearer wording: Ensures the URI part starts with a slash.

mbarnes · 2016-12-07T20:52:29Z

src/commissaire/containermgr/kubernetes/__init__.py

+        :returns: requests.Response
+        """
+        part = self._fix_part(part)
+        payload = json.dumps(payload)


nit: Type-flipping is kinda sneaky; I'd prefer a different name, like payload_str.

mbarnes · 2016-12-07T21:09:51Z

src/commissaire/containermgr/kubernetes/__init__.py

+        self.logger.error(
+            'Non-created response when trying to register the node {}.'
+            'Status: {}, Data: {}'.format(name, resp.status_code, resp.text))
+        return False


Not a blocker, but eating the status code here might bite you later. Maybe return it instead of a bool, or better raise an exception with the code embedded if it's not 2xx? I seem to recall doing something like that in the past.

I'm okay with calling it tech debt for now.

I'll look at doing the raise pattern.

mbarnes · 2016-12-07T21:10:17Z

src/commissaire/containermgr/kubernetes/__init__.py

+        self.logger.error(
+            'Unexpected response when trying to remove the node {}.'
+            'Status: {}, Data: {}'.format(name, resp.status_code, resp.text))
+        return False


See my other comment about eating the status code.

mbarnes · 2016-12-07T21:11:48Z

src/commissaire/containermgr/kubernetes/__init__.py

+            if raw:
+                data = data['status']
+            return data
+        return False


See my other comment about eating the status code.
Also, None seems like a better fallback value than False.

Also included _post which is used by register_node.

Also added _delete which is used by remove_node.

ashcrow · 2016-12-07T23:05:13Z

⬆️

mbarnes · 2016-12-08T14:24:48Z

src/commissaire/containermgr/kubernetes/__init__.py

@@ -225,10 +226,12 @@ def register_node(self, name):
        resp = self._post(part, payload)
        if resp.status_code == 201:
            return True


Not to be overly nit-picky, but you could eliminate the return value since it will never return False. It either works or throws an exception.

mbarnes · 2016-12-08T14:25:23Z

src/commissaire/containermgr/kubernetes/__init__.py

        :rtype: bool
+        :raises: commissaire.containermgr.ContainerManagerError
        """
        part = '/nodes/{}'.format(name)

        resp = self._delete(part)
        if resp.status_code == 200:
            return True


See other return value comment.

mbarnes · 2016-12-08T14:26:20Z

src/commissaire/containermgr/kubernetes/__init__.py

        :rtype: bool
+        :raises: commissaire.containermgr.ContainerManagerError
        """
        part = '/nodes/{0}'.format(name)
        resp = self._get(part)
        # TODO: Stronger checking would be better
        if resp.status_code == 200:
            return True


See other return value comment.

Are you saying something like:

if resp.status_code != 200: raise ....

Yeah, or maybe raise on anything outside the 2xx range.

ashcrow · 2016-12-08T14:44:21Z

⬆️

mbarnes

Fixup looks good but there's a couple other places where this can be done (see below).

mbarnes · 2016-12-08T16:36:51Z

Grrr... can't seem to link to specific lines.

They're in register_node() and remove_node().

ashcrow · 2016-12-08T16:44:54Z

@mbarnes ok I'll update them with the same patterns.

ashcrow · 2016-12-08T17:00:15Z

⬆️

mbarnes

Nice! Looks good.

ashcrow added 2 commits December 6, 2016 17:31

containermgr: Added _fix_path to kubernetes handler.

b4cbcd8

containermgr: Added _put to kubernetes handler.

7c06d35

ashcrow added enhancement WIP labels Dec 6, 2016

ashcrow mentioned this pull request Dec 7, 2016

ContainerManagerService projectatomic/commissaire-service#35

Merged

ashcrow force-pushed the kubernetes-container-handler branch from 1886cb0 to 5a89921 Compare December 7, 2016 19:28

ashcrow changed the title ~~WIP: Kubernetes container handler~~ Kubernetes container handler Dec 7, 2016

ashcrow removed the WIP label Dec 7, 2016

mbarnes self-assigned this Dec 7, 2016

mbarnes suggested changes Dec 7, 2016

View reviewed changes

ashcrow added 14 commits December 7, 2016 17:45

containermgr: Added ContainerManagerError base exception class.

299fecb

containermgr: Added register_node to kubernetes handler.

f553aad

Also included _post which is used by register_node.

containermgr: Added remove_node to kubernetes handler.

5d04eb3

Also added _delete which is used by remove_node.

containermgr: Added get_node_status to kubernetes handler.

e39b29a

doc: api docs for containermgr/service.

2e83bba

fixup! containermgr: Added _fix_path to kubernetes handler.

60b1011

fixup! containermgr: Added _put to kubernetes handler.

7a3a5e8

fixup! containermgr: Added register_node to kubernetes handler.

dae9aff

fixup! containermgr: Added register_node to kubernetes handler.

f59001c

fixup! containermgr: Added ContainerManagerError base exception class.

6d20245

fixup! containermgr: Added remove_node to kubernetes handler.

24b5ac1

fixup! containermgr: Added get_node_status to kubernetes handler.

96b564c

containermgr: Updated node_registered to work like other methods.

131864a

test: Fix tests for the kubernetes containermgr.

2b7ca59

ashcrow force-pushed the kubernetes-container-handler branch from 9b6f572 to 2b7ca59 Compare December 7, 2016 23:04

mbarnes suggested changes Dec 8, 2016

View reviewed changes

fixup! containermgr: Updated node_registered to work like other methods.

83e3a74

mbarnes suggested changes Dec 8, 2016

View reviewed changes

vagrant: Auto starting commissaire-containermgr

866776f

ashcrow added 4 commits December 8, 2016 11:55

fixup! containermgr: Added register_node to kubernetes handler.

0b4aa3d

fixup! containermgr: Added remove_node to kubernetes handler.

0baf7fc

fixup! containermgr: Added get_node_status to kubernetes handler.

e8abc82

fixup! containermgr: Updated node_registered to work like other methods.

7554b75

mbarnes approved these changes Dec 8, 2016

View reviewed changes

mbarnes merged commit 84ca57b into projectatomic:master Dec 8, 2016

ashcrow mentioned this pull request Dec 9, 2016

ContainerManager Service projectatomic/commissaire-service#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubernetes container handler #68

Kubernetes container handler #68

ashcrow commented Dec 6, 2016 •

edited

ashcrow commented Dec 6, 2016

ashcrow commented Dec 7, 2016

mbarnes commented Dec 7, 2016

mbarnes left a comment

mbarnes Dec 7, 2016

mbarnes Dec 7, 2016

mbarnes Dec 7, 2016

mbarnes Dec 7, 2016

mbarnes Dec 7, 2016

ashcrow Dec 7, 2016

mbarnes Dec 7, 2016

mbarnes Dec 7, 2016

ashcrow Dec 7, 2016

ashcrow commented Dec 7, 2016

mbarnes Dec 8, 2016

mbarnes Dec 8, 2016

mbarnes Dec 8, 2016

ashcrow Dec 8, 2016

mbarnes Dec 8, 2016

ashcrow Dec 8, 2016

ashcrow commented Dec 8, 2016

mbarnes left a comment

mbarnes commented Dec 8, 2016

ashcrow commented Dec 8, 2016

ashcrow commented Dec 8, 2016

mbarnes left a comment

Kubernetes container handler #68

Kubernetes container handler #68

Conversation

ashcrow commented Dec 6, 2016 • edited

ashcrow commented Dec 6, 2016

ashcrow commented Dec 7, 2016

mbarnes commented Dec 7, 2016

mbarnes 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

ashcrow commented Dec 7, 2016

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

ashcrow commented Dec 8, 2016

mbarnes left a comment

Choose a reason for hiding this comment

mbarnes commented Dec 8, 2016

ashcrow commented Dec 8, 2016

ashcrow commented Dec 8, 2016

mbarnes left a comment

Choose a reason for hiding this comment

ashcrow commented Dec 6, 2016 •

edited