-
Notifications
You must be signed in to change notification settings - Fork 71
Conversation
93140ad
to
a0073c7
Compare
cmd = [self.kubectl, "create", "-f", tmp_file, "--namespace=%s" % self.namespace] | ||
if self.config_file: | ||
cmd.append("--kubeconfig=%s" % self.config_file) | ||
self._call(cmd) |
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 we add deleting / removing persistent storage or just running?
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 guess for now we can stick with this and revisit when we do uninstall.
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.
ping @dustymabe how should we check before running this? another if statement at the beginning of the function to check what actions are available?
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.
Right.. I would move "checks" to the beginning of the function. No reason to do work if we fail the checks
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.
Given this is "persistent" we probably don't want to ever be deleting. I believe OpenShift does not delete storage when all objects are destroyed. It's treated special so as to do no unexpected harm.
a0073c7
to
e6355e3
Compare
Oh, another note, logging is starting to get SO MESSY. Way too much logging coming out now when running each application, this will be fixed in #134 |
Ok.. ran through it once.. not sure I quite understand it all just yet.. still digesting. I think this would be better/easier to understand if you broke up the big commit into smaller commits with good commit messages. Let's chat later today or next week about the architecture |
# Pass configuration, path of the app, graph, provider as well as dry-run for provider init() | ||
if REQUIREMENTS_KEY in self.graph[0]: | ||
logger.debug("Requirements key detected. Running action.") | ||
Requirements(self.config, self.basepath, self.graph[0][REQUIREMENTS_KEY], provider_key, dryrun).run() |
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 be
requirements = Requirements(etc...)
requirements.run()
e6355e3
to
0ed429f
Compare
Merged in changes from @dustymabe 's suggestions :) Let's split this into different commit's and have a look at this after the weekend |
806a5ba
to
1aaa682
Compare
Okay, so I've split the commits as well as added checking Ping @dustymabe The only thing that's left is documentation to be added. We will need a discussion with @bexelbie about this. With a new feature like this added, it's hard for users to see the significant changes to atomicapp if we only update the Nulecule spec. A detailed documentation that lists all Nulecule functions in atomicapp should be here. I can volunteer to help add this. Side note: |
# Pass configuration, path of the app, graph, provider as well as dry-run for provider init() | ||
if REQUIREMENTS_KEY in self.graph[0]: | ||
logger.debug("Requirements key detected. Running action.") | ||
r = Requirements(self.config, self.basepath, self.graph[0][REQUIREMENTS_KEY], provider_key, dryrun) |
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.
Split up this line.. too long
117e7cd
to
b0970fc
Compare
"%s action is not available for provider %s. Doing nothing." % | ||
(action, self.key)) | ||
return | ||
|
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.
@dustymabe like this? don't see any other way since each cmd call will be different for delete / create / etc. when more functions are added.
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.
Would rather do:
if action not in ['run']:
logger.warning()
In the future when we add more it would be:
if action not in ['run', 'stop', etc..]:
logger.warning()
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.
updated ^^
Tested with changes and can confirm functionality:
|
377c149
to
ea722d5
Compare
p = plugin.getProvider(provider) | ||
self.provider = p(config, basepath, dryrun) | ||
self.provider.init() | ||
|
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.
^^
ea722d5
to
c703374
Compare
from atomicapp.plugin import Plugin | ||
|
||
plugin = Plugin() | ||
plugin.load_plugins() |
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.
move to inside of __init__
Sorry I haven't been following this from the start. Question: where is the volume supposed to be mounted so the application can actually use it? In my experience if persistent storage is needed it's defined in a template and referenced in the pod spec so it's actually mounted. But to specify all of that in Nulecule feels like we're duplicating kubernetes. I think the spec is lacking here but this looks like a good start to implement what we have. LGTM |
@aweiteka I think you got the approach right: the Nulecule Specification is making a statement about what type of storage an application needs, Atomic App is going to auto generate a Kubernetes/OpenShift snippet for that and the application can use it. We can also keep the storage requirements within the artifacts of the provider, but than we will obfuscate the application, eg the deployment guy will not be able to parameterize the size or type of the storage. Furthermore, what is the spec missing wrt a storage requirement? |
considering @aweiteka's new comment in projectatomic/nulecule#193 the application artefact will define how it wants to use the volume, where it should be mounted. We don't need to provide a path in the spec. I think we should move forward with this review. @aweiteka @goern can you please review so that we can get this merged? |
cmd.append("--kubeconfig=%s" % self.config_file) | ||
lines = self._call(cmd) | ||
|
||
# If there are no persistent volumes to claim, warn the user |
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.
There are more error cases. I can think of two:
- claim with that name has already been created.
- user is claiming more space than allowed.
These (and other errors like this I can't think of) can be generally caught with one check, I would think.
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.
Ah yes, this should error out automatically though when calling the API. But it would be nice to parse and create a good logging output for this :)
Nice work! LGTM. |
c703374
to
4986521
Compare
^^ changed the small comment in code from @vpavlin 's comment :) I'll update notes / comments in a different PR for changes unrelated to persistent storage. |
@cdrage, since there has been a lot of work in master can we get a rebase on it. Might be good to test on top of master anyway. |
4986521
to
58aaa15
Compare
Rebased and checks pass! |
With @aweiteka LGTM and a LGTM from me.. merging. |
Add persistent storage for kubernetes
DONE:
Initial work on persistent storage. Work's correctly. Just need to clean-up most of the code.It's a Work-In-Progress so excuse the multiple log messages, but you get the gist of how I implemented it.Code comments / docsMore tests!tests cover 92% and under
kubernetes.py persistent_storage
function tooClean up log messagesTODO:
kubectl pvc
output or something to see if there is actually any persistent volumes yet for Kubernetes. Warn the user if so.Notes: