-
Notifications
You must be signed in to change notification settings - Fork 925
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
New Rancher Container Driver #876
Conversation
this is fantastic work, I'll work through the review |
Love it! Is their a direct kubernetes driver yet? Im on the road else id On Thursday, September 29, 2016, Anthony Shaw notifications@github.com
|
Nice @InAnimaTe Random comments related to the Rancher integration...
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
|
||
class RancherResponse(JsonResponse): | ||
|
||
def parse_body(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toapplication/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.
def parse_error(self): | ||
if self.status == 401: | ||
raise InvalidCredsError('Invalid credentials') | ||
return self.body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should super
be called here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# As in the /v1/ | ||
version = '1' | ||
|
||
def __init__(self, key, secret, secure=False, host='localhost', port=80): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not secure by default like the base classes?
self.connection.host = host | ||
self.connection.port = port | ||
self.connection.secure = secure | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these three lines necessary if the base class is suppose to initialize the connection? Seems redundant.
secure=secure, host=host, | ||
port=port) | ||
if host.startswith('https://'): | ||
secure = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"rancherCompose": ranchercompose, | ||
"startOnCreate": start | ||
} | ||
data = json.dumps({k: v for k, v in payload.items() if v is not None}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just let the method accept **payload
keyword args?
disks=None, kind=None, memorymb=None, | ||
networklaunchconfig=None, requsetedipaddress=None, | ||
userdata=None, vcpu=None, **kwargs): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. snake_casing please
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. |
|
||
launchconfig = { | ||
"imageUuid": self._degen_image(image), | ||
**kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't do that :-) if you want to merge the 2 you can just use the +
operator. what's the intention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remembering that this has to work in Py2, I'll use the dict constructor and change it here ;)
secure=secure, host=host, | ||
port=port) | ||
if host.startswith('http://'): | ||
secure = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called before the call to super, so the base classes get the change.
"vip": vip | ||
} | ||
|
||
data = json.dumps({k: v for k, v in service_payload.items() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If libcloud is targeting < Py2.7 then dict comprehensions will not work. If so, just use dict(..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does target 2.6
def parse_error(self): | ||
if self.status == 401: | ||
raise InvalidCredsError('Invalid credentials') | ||
return self.body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be addressed.
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
👍 |
fantastic. congratulations! LGTM - merging. |
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:
ex_*
)ex_*
)Although not every possible action is available, the necessities like creation and deletion plus things like activation, get, search, and list are included.
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
Status
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:
parse_body
?)*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!deploy_container
docstring!Does not support filters for searching/listing. (no
image
orall
support inlist_containers
)The
launchConfig
forex_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):
Checklist (tick everything that applies)
flake8
completed successfully ;)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.