New Rancher Container Driver #876

InAnimaTe · 2016-09-29T03:31:35Z

Rancher Container Driver

Description

This is a new Container Driver for Rancher, the fantastic container orchestration platform that supports the likes of Kubernetes, Docker Swarm, and Mesos frameworks. It provides a fantastic abstraction layer for working with these frameworks on any infrastructure you'd like! Plus it's open-source!

Utilizing the v1 API and extensively working with the docs, I've managed to get an initial version of a fairly reliable ContainerDriver for Libcloud. It follows the normal Container conventions as closely as possible and provides extensive docstrings for excellent interaction, even without a massive understanding of Rancher itself.

The three main top-level Rancher items are implemented:

Containers
Services (ex_*)
Environments (ex_*)

Although not every possible action is available, the necessities like creation and deletion plus things like activation, get, search, and list are included.

As you review this, please ask questions if you aren't sure about how something is implemented!

Background

With Arroyo Networks' upcoming NaaS platform, Inflow, I've been hard at work establishing our Infrastructure assets. Rancher has seemed a clear choice to help us maintain a nimble platform not tied to any specific back-end Infrastructure provider. Hence, much of our Python code needs a way to interact with our clusters, so writing a Libcloud plugin was pretty much a no-brainer.

Example

from libcloud.container.providers import get_driver
from libcloud.container.types import Provider

driver = get_driver(Provider.RANCHER)
connection = driver(key="ACCESS_KEY_HERE", secret="SECRET_KEY_HERE", host="172.30.0.100", port=8080)

image = ContainerImage("hastebin", "hastebin", "rlister/hastebin", "latest", driver=None) 
newcontainer = connection.deploy_container("myawesomepastebin", image, environment={"STORAGE_TYPE": "file"})

Status

done, ready for review

I have tested this works appropriately with Rancher Server v1.1.3 (API v1).

Of course, I want persons looking at this to pay attention to the whole PR. However, there are a few key areas I want some focus on:

Unnecessary sections (do I really need parse_body?)
Inefficient handling (is there a better way to do Image parsing or TLS/SSL support?)
Error Handling looks/works sanely.
Convention adherence (I'm a newer programmer; I may not know well established *ism's)

I encourage reviewers to pull my branch and play with this ;)

Notes

Does not support user based API credentials, only Environment API credentials (one env, no cluster support)
Does not support images other than docker formatted images. docker: prefix is forced!
- Images follow a standardized format. See deploy_container docstring!
Does not support filters for searching/listing. (no image or all support in list_containers)
The launchConfig for ex_deploy_service is actually all defined at the top level. This is because I easily pass it through _build_payload (**kwargs) since the options are nearly identical to those of a normal container. No reason to duplicate work. This also makes it easier since most people only generally want to deploy a service with only one container anyways. This may need to change later when we want >1 container for a service.
Passing your own cert/key for SSL/TLS is not presently supported.
For SSL/TLS (https) support with newer versions of OpenSSL, the following is necessary (in your own code):
```
import ssl
import libcloud.security
libcloud.security.SSL_VERSION = ssl.PROTOCOL_TLSv1_2
```

Checklist (tick everything that applies)

Code linting (required, can be done after the PR checks) - flake8 completed successfully ;)
Documentation - See Docstrings and docs/
Tests - Perfected ;)
ICLA (required for bigger changes) - Corporate Agreement Submitted!

Thanks

I have worked closely with Anthony Shaw, a core contributor to Libcloud (especially the Container stuffs) as well as Vincent Fiduccia, software architect at Rancher Labs. The have been invaluable in helping me establish a solid driver for Rancher.

tonybaloney · 2016-09-29T05:56:46Z

this is fantastic work, I'll work through the review

morissette · 2016-09-29T06:24:55Z

Love it! Is their a direct kubernetes driver yet? Im on the road else id
look at source

On Thursday, September 29, 2016, Anthony Shaw notifications@github.com
wrote:

this is fantastic work, I'll work through the review

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#876 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADdgZ4e87yOMzidAuP8ryItrd7wQ7tDmks5qu1MggaJpZM4KJi43
.

vincent99 · 2016-09-29T07:40:40Z

Nice @InAnimaTe

Random comments related to the Rancher integration...

A little late now, but we do have a python API binding you could use.
Listing every field of resources like container/service is going to quickly be out of date.. I don't know if passing a map is an option that fits into the rest of libcloud rather than 30 positional params, but if so you can save a lot of work keeping it in sync.
resource.transitioning == 'yes' would be better for PENDING state than endswith('ing')
API v2[-beta] renames environment to stack, so while you might not want to use v2 yet (for compatibility with Rancher v1.1.x) you should probably refer to it as a stack in the public interface.
outputs, previousEnvironment & previousExternalId are technically settable by users for obscure technical reasons, but are not really user inputs. So you could remove those as options for a env/stack.

mmellison · 2016-09-29T12:39:08Z

I agree, having all those arguments is outrageous and will fail quickly. No reason why the common ones can't be called out directly and the rest of them just accepted as kwargs.

mmellison

Looking good.

mmellison · 2016-09-29T12:52:54Z

libcloud/container/drivers/rancher.py

+
+class RancherResponse(JsonResponse):
+
+    def parse_body(self):


Why override this method? Two issues:

Your version just returns None if the content-type header is not set by the server (oops).

Below you set the Accept header to application/json only; so it would be reasonable that we should only get json back.

I think you should just let the base class handle this.

mmellison · 2016-09-29T12:56:05Z

libcloud/container/drivers/rancher.py

+    def parse_error(self):
+        if self.status == 401:
+            raise InvalidCredsError('Invalid credentials')
+        return self.body


Should super be called here?

This still needs to be addressed.

you don't need to call super. the super has no constructor and its only if you wanted to pass specified args to it which you aren't collecting. If rancher APIs have a special error model then this is the bit where you abstract that field from the body and raise the correct error.

I'm not talking about adding an init to this class, I mean that it should be return super().parse_error(). because looking at the base class, it looks like parse_error is suppose to return a parsed response, not just the raw response body.

mmellison · 2016-09-29T13:01:31Z

libcloud/container/drivers/rancher.py

+    # As in the /v1/
+    version = '1'
+
+    def __init__(self, key, secret, secure=False, host='localhost', port=80):


Why not secure by default like the base classes?

mmellison · 2016-09-29T13:04:30Z

libcloud/container/drivers/rancher.py

+        self.connection.host = host
+        self.connection.port = port
+        self.connection.secure = secure
+


Why are these three lines necessary if the base class is suppose to initialize the connection? Seems redundant.

mmellison · 2016-09-29T13:05:49Z

libcloud/container/drivers/rancher.py

+                                                     secure=secure, host=host,
+                                                     port=port)
+        if host.startswith('https://'):
+            secure = True


See my other comment above, but I would think it should default to true, and then be disabled (to follow the pattern of the base classes).

Also, shouldn't this be set before the call to super's init? Otherwise its not caught in the base classes.

mmellison · 2016-09-29T13:07:28Z

libcloud/container/drivers/rancher.py

+            "rancherCompose": ranchercompose,
+            "startOnCreate": start
+        }
+        data = json.dumps({k: v for k, v in payload.items() if v is not None})


Why not just let the method accept **payload keyword args?

mmellison · 2016-09-29T13:21:14Z

libcloud/container/drivers/rancher.py

+                          disks=None, kind=None, memorymb=None,
+                          networklaunchconfig=None, requsetedipaddress=None,
+                          userdata=None, vcpu=None, **kwargs):
+        """


Not to nitpick too much, but if we're passing around so many damn arguments, then at least use shorter and pythonic naming conventions to help ease with reading the code, e.g. networklaunchconfig -> net_launch_conf

I think it would be better to accept a dict of service arguments, e.g. (self, name, image, env_id, service_args=None, **payload)

yep. snake_casing please

InAnimaTe · 2016-09-30T15:43:05Z

Thanks a lot of the input everyone. I've modified and changed around quite a bit based on input which has made this driver quite a bit better ;)

I'd like some more input from @tonybaloney about how the response class should be utilized. Other than that, I'm working on a test (similar to the docker test) and should have a license agreement signed soon.

tonybaloney · 2016-09-30T05:45:38Z

libcloud/container/drivers/rancher.py

+
+        launchconfig = {
+            "imageUuid": self._degen_image(image),
+            **kwargs


You can't do that :-) if you want to merge the 2 you can just use the + operator. what's the intention here?

So, it does actually work (something new with python3 i guess). What I'm trying to do there is take in extra container options and adding them to the launchConfig. This makes it super easy to use that function and for other programs that utilize this (they don't have to worry about building a separate dict if they just want to modify a few options)

Remembering that this has to work in Py2, I'll use the dict constructor and change it here ;)

mmellison · 2016-10-06T23:07:14Z

libcloud/container/drivers/rancher.py

+                                                     secure=secure, host=host,
+                                                     port=port)
+        if host.startswith('http://'):
+            secure = False


This should be called before the call to super, so the base classes get the change.

mmellison · 2016-10-06T23:10:00Z

libcloud/container/drivers/rancher.py

+            "vip": vip
+        }
+
+        data = json.dumps({k: v for k, v in service_payload.items()


If libcloud is targeting < Py2.7 then dict comprehensions will not work. If so, just use dict(..)

it does target 2.6

mmellison · 2016-10-06T23:13:53Z

libcloud/container/drivers/rancher.py

+    def parse_error(self):
+        if self.status == 401:
+            raise InvalidCredsError('Invalid credentials')
+        return self.body


This still needs to be addressed.

tonybaloney · 2016-10-07T02:13:14Z

libcloud/container/drivers/rancher.py

@@ -41,7 +41,7 @@ class RancherResponse(JsonResponse):
    def parse_error(self):
        if self.status == 401:
            raise InvalidCredsError('Invalid credentials')
-        return self.body
+        return super().parse_error()


You can just take this method out altogether and let it fall through to the base class if you're not going to do anything Rancher-specific with the response.

mmellison · 2016-10-08T02:15:16Z

👍

tonybaloney · 2016-10-08T02:28:56Z

fantastic. congratulations! LGTM - merging.

Mario Loria added 3 commits September 27, 2016 23:12

initial push

3143283

docstrings, flake8, error handling

675cb6b

docstring cleanup

b713cab

mmellison suggested changes Sep 29, 2016

View reviewed changes

Mario Loria added 8 commits September 29, 2016 11:12

extra contains entire json resp

aabd03a

secure by default

1f4e906

environment -> stack

0d3f819

move _build_payload to bottom near other helper functions

401f6c2

kwargs, no more build_payload

8836d95

remove unneeded stuffs

a2485c1

minor stuffs

3404e0c

casing and option changes

055045f

Mario Loria added 2 commits October 4, 2016 21:52

added search for stacks and services

ae1abbd

and container search ;)

2099083

tonybaloney reviewed Oct 5, 2016

View reviewed changes

Mario Loria added 2 commits October 5, 2016 18:51

fixup kwargs config handling

facae42

docs written

08423b7

mmellison suggested changes Oct 6, 2016

View reviewed changes

Mario Loria added 3 commits October 6, 2016 21:17

move protocol check before super

fae2049

dict instead of comprehension

fe877ee

mod return in parse_error

d17d5f8

tonybaloney reviewed Oct 7, 2016

View reviewed changes

Mario Loria added 3 commits October 6, 2016 22:19

tonybaloney rm parse_error sugg

4c046ac

properly parse_error now

7f4ac83

everything is perfect; add tests

81fba12

Mario Loria added 4 commits October 7, 2016 20:47

cleanup comments on test

4794638

and secrets file

d16d6f2

flake and doc fixes

f191ead

lint fixes in doc examples

d964bfa

asfgit closed this in 5f1d6bc Oct 8, 2016

asfgit pushed a commit that referenced this pull request Oct 8, 2016

changes for #876

1a83614

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Rancher Container Driver #876

New Rancher Container Driver #876

InAnimaTe commented Sep 29, 2016 •

edited

tonybaloney commented Sep 29, 2016

morissette commented Sep 29, 2016

vincent99 commented Sep 29, 2016 •

edited

mmellison commented Sep 29, 2016

mmellison left a comment

mmellison Sep 29, 2016

mmellison Sep 29, 2016

mmellison Oct 6, 2016

tonybaloney Oct 7, 2016

mmellison Oct 7, 2016

mmellison Sep 29, 2016

mmellison Sep 29, 2016

mmellison Sep 29, 2016

mmellison Sep 29, 2016

mmellison Sep 29, 2016

tonybaloney Sep 30, 2016

InAnimaTe commented Sep 30, 2016

tonybaloney Sep 30, 2016

InAnimaTe Oct 5, 2016

InAnimaTe Oct 5, 2016

mmellison Oct 6, 2016

mmellison Oct 6, 2016 •

edited

tonybaloney Oct 6, 2016

mmellison Oct 6, 2016

tonybaloney Oct 7, 2016

mmellison commented Oct 8, 2016

tonybaloney commented Oct 8, 2016

New Rancher Container Driver #876

New Rancher Container Driver #876

Conversation

InAnimaTe commented Sep 29, 2016 • edited

Rancher Container Driver

Description

Background

Example

Status

Notes

Checklist (tick everything that applies)

Thanks

tonybaloney commented Sep 29, 2016

morissette commented Sep 29, 2016

vincent99 commented Sep 29, 2016 • edited

mmellison commented Sep 29, 2016

mmellison 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InAnimaTe commented Sep 30, 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

mmellison Oct 6, 2016 • 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

mmellison commented Oct 8, 2016

tonybaloney commented Oct 8, 2016

InAnimaTe commented Sep 29, 2016 •

edited

vincent99 commented Sep 29, 2016 •

edited

mmellison Oct 6, 2016 •

edited