Skip to content
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

feat: add Longhorn V2 provisioner #129

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

tserong
Copy link
Contributor

@tserong tserong commented Sep 9, 2024

Problem:
We need to add a provisioner for LH V2 disks

Solution:
This PR

Related Issue:
harvester/harvester#6014
harvester/harvester#5274

Test plan:
TBD

Note:
The first two commits in this PR are cherry picks from #128, because I needed the changes to the blockdevices CRD. I'll drop/rebase appropriately once that PR goes in.

@tserong
Copy link
Contributor Author

tserong commented Sep 9, 2024

OK, that force push should have fixed the dirty validation, and we can ignore CodeFactor, because it's just whining about generated code :-)

Some further comments lifted from the commit message on the last commit:

I've implemented this as a separate LonghornV2Provisioner for the sake of neatness, rather than grafting it into LonghornV1Provisioner with a bunch of if statements, but some of the implementation (the UnProvision() and Update() functions) still call back to the LonghornV1Provisioner to avoid excessive code duplication.

The current implementation theoretically supports using virtio disks, except that NDM as a whole ignores those, as they don't have WWNs. This is something we need to address separately as part of harvester/harvester#6034

pkg/provisioner/longhornv2.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we need to wait for the LVM provisioner and rebase again.

Copy link

mergify bot commented Sep 12, 2024

This pull request is now in conflict. Could you fix it @tserong? 🙏

Signed-off-by: Tim Serong <tserong@suse.com>
Signed-off-by: Tim Serong <tserong@suse.com>
This allows adding extra disks for use by Longhorn's V2 data engine.
I've implemented this as a separate LonghornV2Provisioner for the
sake of neatness, rather than grafting it into LonghornV1Provisioner
with a bunch of `if` statements, but some of the implementation
(the UnProvision() and Update() functions) still call back to the
LonghornV1Provisioner to avoid excessive code duplication.

The current implementation theoretically supports using virtio disks,
except that NDM as a whole ignores those, as they don't have WWNs.
This is something we need to address separately as part of
harvester/harvester#6034

Related issue: harvester/harvester#5274

Signed-off-by: Tim Serong <tserong@suse.com>
@tserong tserong merged commit 83b58dd into harvester:master Sep 13, 2024
6 checks passed
@tserong tserong deleted the wip-lhv2-provisioner branch September 13, 2024 04:47
@harvester harvester deleted a comment from mergify bot Sep 13, 2024
@harvester harvester deleted a comment from mergify bot Sep 13, 2024
@tserong
Copy link
Contributor Author

tserong commented Sep 13, 2024

@Mergifyio backport v0.7.x

Copy link

mergify bot commented Sep 13, 2024

backport v0.7.x

✅ Backports have been created

@tserong
Copy link
Contributor Author

tserong commented Sep 13, 2024

OK, I give up, obviously I can't get mergify to redo a backport PR, I'm just going to do this by hand... (see #133)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants