-
Notifications
You must be signed in to change notification settings - Fork 262
Conversation
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #411 +/- ##
==========================================
- Coverage 62.91% 61.85% -1.07%
==========================================
Files 62 64 +2
Lines 3613 3827 +214
==========================================
+ Hits 2273 2367 +94
- Misses 1089 1172 +83
- Partials 251 288 +37
Continue to review full report at Codecov.
|
@YujiOshima - nice work. Just a general question... does MAAS support the notion of detachable disk / volumes? At least with the AWS plugin at https://github.com/docker/infrakit.aws it understands the Does this model make sense for MAAS or for bare-metal where disks/volumes can be attached /detached from compute instances? |
@chungers Thank you! |
In the meantime you could always use Docker volume plugin. For example just have 1 NFS share and create sub directories in your container when they don't exist yet. That way you don't need to request a share. |
@Lennie Thank you for the comment. Only for docker swarm flavor and an app in the docker container, It is true. But for other flavor or outside of docker container (I don't know the specific use case, may be docker daemon config or something), you need Infrakit volume management. |
Just saying workarounds are possible if you need to deploy soon and can't wait. Or need something for a dev. env. on a laptop. Also just realized you can create NFS server as a docker container as well. But would be great to have a proper option for MAAS. But also don't forget the dev. laptop. |
@Lennie Thank you for a good point. Yes, we need to do a lot of things before volume management. |
Set up nodes. | ||
|
||
``` | ||
./setup-nodes.sh |
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.
Is this file included in this PR? I don't see it. Would you mind including it?
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 update the Readme. Now I have only a simple document, but I will enhance it with the addition of functions.
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.
Sorry I was confused. This is a file from your repo... I see now, so there's no need to include this.
examples/instance/maas/instance.go
Outdated
if err != nil { | ||
return err | ||
} | ||
return ioutil.WriteFile(tagFile, encoded.Bytes(), 0666) |
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.
Files are written locally to store tags... is there a way in MAAS to tag resources and query by tags? https://docs.ubuntu.com/maas/2.1/en/intro-concepts#tags
If we can reduce amount of state we need store locally, the better. If there are ways to do tagging via the MAAS api, then we should consider making that change in a future PR?
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 think that is a better implementation. This time we implemented it in a simple way that does not depend on the api version, but we will change the information to keep it in a robust way without duplication.
} | ||
|
||
// DescribeInstances returns descriptions of all instances matching all of the provided tags. | ||
func (m maasPlugin) DescribeInstances(tags map[string]string) ([]instance.Description, 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.
See comment above -- is there a way in MAAS to query by tags of the nodes? Using files here is ok but we'd have to make sure these files are highly available.
@@ -0,0 +1,24 @@ | |||
{ |
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 file is named .yml
but the content is a JSON...
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.
Oh..This is a mistake. Fixed.
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
@YujiOshima - Before I can merge this PR, can you please add a few tests? You can look at the Also, once this PR is accepted, would you mind open an issue (and assign to yourself) on improving this plugin by using MAAS tagging API to implement the instance state persistence? I want to make sure that if we accept this PR, this plugin will continue to be improved and is useful for those learning to work with InfraKit. Thank you! |
Please sign your commits following these rules: $ git clone -b "maas" git@github.com:YujiOshima/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354523656
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
@chungers Yeah! I added a few tests!
Of course! I will do so soon. And I want to proceed while discussing. |
I used a gomaasapi.testservice for test this plugin. |
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.
LGTM -- Let's get this merged in and we can start iterating on this. We can incubate this plugin in this repo and spin it off to a separate project when it's ready.
Added support for dynamic DDC generation
Support BareMetal deployment.
An Instance plugin for Canonical MaaS node.
#398