-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add lvm provisioner #128
Add lvm provisioner #128
Conversation
abe41ff
to
f83ecd5
Compare
codeFactor complains about the generated code. Please ignore that. |
f83ecd5
to
097bf8c
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.
Thanks, overall works well, just some suggestions.
deploy/charts/harvester-node-disk-manager/templates/crds/harvesterhci.io_lvmvolumegroups.yaml
Outdated
Show resolved
Hide resolved
097bf8c
to
0fec916
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.
Looks good in general, I just noticed a couple of things
0fec916
to
379b9e8
Compare
- Also move types.go to blockdevice.go - Add new field `spec.provisioned` Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
4f4cb86
to
bbd213e
Compare
bbd213e
to
95ee809
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.
I did a build of this and gave it a quick test on a single node cluster with two extra disks, and it works nicely (I provisioned and unprovisioned both disks to the same VG and it looks like it's behaving correctly).
Regarding the NotFoundError, it might be preferable to import "k8s.io/apimachinery/pkg/api/errors" then call errors.NewNotFound(...)
to create the error, and errors.IsNotFound()
to check it, rather than having our own error variable. If that makes sense, you can ignore my comments below regarding error variable naming and import paths :-)
- Now we could use the following fields to provision the volume group ``` Spec: provisioner: lvm: vgName: vg001 ``` - Introduce the new field to trigger provision ``` Spec: provision: true ``` - The new CRD lvmvgs is introduced, this will contain the node info and the volume group information. Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
95ee809
to
884615d
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.
LGTM
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!
@Mergifyio backport v0.7.x |
✅ Backports have been created
|
Problem:
Add LVM provisioner
Solution:
Add LVM provisioner
Related Issue:
harvester/harvester#6014
harvester/harvester#5724
Test plan:
TBD