-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
this is actually started as part of the development mode overrides (or improvement of development workflow), so it is an item on the 0.3.0 roadmap. |
[lvm1] | ||
iscsi_helper = tgtadm | ||
iscsi_protocol = iscsi | ||
lvm_conf_file = /etc/cinder/lvm.conf |
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.
does this lvm.conf exist in the container, i dont see it in the template folder?
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 doesn't exist, I think we can safely remove the reference to it: https://github.com/openstack/cinder/blob/master/cinder/volume/drivers/lvm.py#L59-L64
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. This will be removed before the WIP tag comes off.
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.
@wilkers-steve This is an awesome bit of work, and a great start toward getting LVM up and running. I'll do a proper review tomorrow, once I've had time to mull over my thoughts on ways to implement the manifest, but a couple of things stick out at 1st parse.
@@ -43,6 +43,9 @@ spec: | |||
{{ .Values.labels.node_selector_key }}: {{ .Values.labels.node_selector_value }} | |||
containers: | |||
- name: cinder-volume | |||
securityContext: | |||
privileged: true | |||
runAsUser: 0 |
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.
Now this is functional, we should be able to drop the privileges back down.
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.
still running as root? i guess we can come back to each of these and run as non-root users when we get into the security context items (later near 0.6.0), but we need to keep track of them.
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 will be removed with the next commit adding iSCSI to this 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.
well, to be fair...it's not the only one.i think privs and security is more of a project-wide security effort, and part of a later cleanup.
02d557e
to
8d9836e
Compare
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.
Steve, great work. A few comments, but otherwise looks good to me. This will be fun incorporating into the config management framework and actually seems like a great next step since you're already deep into this chart.
@@ -78,3 +85,18 @@ rbd_secret_uuid = {{- include "secrets/ceph-client-key" . -}} | |||
{{- end }} | |||
rbd_secret_uuid = {{ .Values.backends.rbd1.secret }} | |||
report_discard_supported = True | |||
{{ end }} | |||
|
|||
{{ if .Values.lvm.enabled }} |
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.
@intlabs @wilkers-steve This is a comment for the future:
Although some of this may change when cinder gets a config override pass, this section (and ceph above) to me says a helm-toolkit function to take a yaml declaration and turn it into ini format would be useful in places like this. An equivalent of `{{ toYaml ...`` but geared towards oslo/ini.
This way you are not trying to provide overrides for every option possible, you give the end user the flexibility to target any known param, and add more.
Again, cinder config override stuff may account for some of this, but likely not all and we may still have a need to define backends much like Steve is doing here. We won't know until we see what an expanded cinder.conf contains from oslo config gen.
In any event, what I am describing is:
values.yaml:
lvm:
config:
iscsi_helper: tgtadm
lvm_conf_file: /etc/cinder/lvm.conf
...
And then here:
{{ if .Values.lvm.enabled }}
{{ tuple .Values.lvm.config | include "helm-toolkit.yaml_to_oslo" }}
{{ end }}
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.
@alanmeadows I like what you're describing. I agree that'd be a useful feature to have on hand when required.
@@ -0,0 +1,27 @@ | |||
# Configuration for cinder-rootwrap |
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.
Good addition Steve. @v1k0d3n We likely need a feature to account for _rootwrap.conf.tpl consistency across the board. This is the first (welcomed) introduction. This is inline with me bringing in policy.json and paste everywhere.
@@ -153,6 +156,169 @@ dependencies: | |||
- keystone-api | |||
- cinder-api | |||
|
|||
mounts: |
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 did awesome laborious work here Steve. There was a change of approach for mounts to make the .Values storage for them simply append as opposed to housing all mounts per #319. This means users cannot "undo" our mounts but they can add it does make values.yaml a bit smaller and keeps the mounts we need closer to the actual manifest. This is of course my fault for sending you down this path and then changing course after this PR.
We can either move these back into the manifests and copy #319 in a separate PR or update them here. Totally up to you.
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.
@alanmeadows Thanks for the feedback. I can move these back to the manifests in this pull request.
5226b6e
to
60a99bf
Compare
@alanmeadows @v1k0d3n @intlabs I've changed the mounts to function similarly to #319. The only snowflake was cinder-volume, but we can address how to conditionally handle backend-specific mounts as we move forward |
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.
Steve, this looks awesome. I think we should also provide iSCSI as part of this PR, so that the volumes can actually be consumed. As soon as that's in I think we should be good to merge. :)
60a99bf
to
89603bc
Compare
This adds the ability for cinder to use LVM as a backend
89603bc
to
fc88837
Compare
@wilkers-steve is PR this in or out for openstack? |
@v1k0d3n I would say it could go either way. I wouldn't let this block the -1 workflow on the patchset for moving into OpenStack. If I get the iscsi bits added before the +2 happens for moving into infra, we can go ahead and merge it in. If it doesn't, I'll add the work in once we move into OpenStack |
Lets get this in once we're in OpenStack. |
closing this PR in favor of moving the work to Openstack proper: https://github.com/openstack/openstack-helm |
Initial commit for adding LVM support for Cinder. Adds necessary
configuration files for supporting configuration of the backend
What is the purpose of this pull request?: Add support for LVM backend for Cinder
What issue does this pull request address?: Fixes #293
Notes for reviewers to consider:
Currently the configmaps for the LVM backend aren't handled with conditionals. This will change before the WIP tag gets removed.
Specific reviewers for pull request: