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 (backport #129) #132

Closed
wants to merge 3 commits into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Sep 13, 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.


This is an automatic backport of pull request #129 done by Mergify.

Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 9e0fb40)
Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 58b42e7)
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>
(cherry picked from commit 83b58dd)

# Conflicts:
#	pkg/controller/blockdevice/controller.go
@mergify mergify bot added the conflicts label Sep 13, 2024
Copy link
Author

mergify bot commented Sep 13, 2024

Cherry-pick of 83b58dd has failed:

On branch mergify/bp/v0.7.x/pr-129
Your branch is ahead of 'origin/v0.7.x' by 2 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 83b58dd.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   deploy/charts/harvester-node-disk-manager/templates/crds/harvesterhci.io_blockdevices.yaml
	modified:   manifests/crds/harvesterhci.io_blockdevices.yaml
	modified:   pkg/apis/harvesterhci.io/v1beta1/types.go
	new file:   pkg/provisioner/longhornv2.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   pkg/controller/blockdevice/controller.go

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Copy link
Author

mergify bot commented Sep 13, 2024

This pull request is now in conflict. Could you fix it @mergify[bot]? 🙏

@tserong
Copy link
Contributor

tserong commented Sep 13, 2024

I've fixed the conflicts, but github is still processing the update...

@Vicente-Cheng
Copy link
Collaborator

We should use blockdevice.go ... and minor conflict with the LVM provisioner.

Just curious why there is conflict. Does that mean we will have something wrong with the master branch?

@tserong
Copy link
Contributor

tserong commented Sep 13, 2024

Just curious why there is conflict. Does that mean we will have something wrong with the master branch?

No, I think what happened is a timing problem between merging #131 and this backport opening :-/

@tserong
Copy link
Contributor

tserong commented Sep 13, 2024

There's something screwy going on with this PR now. I've had this "processing updates" thing for probably >30 minutes now:

image

...and if you look at the commit hashes in this PR, they don't match the hashes in the underyling branch (https://github.com/harvester/node-disk-manager/commits/mergify/bp/v0.7.x/pr-129/)

If this doesn't resolve soon, I might just close this PR and open another one...

@tserong
Copy link
Contributor

tserong commented Sep 13, 2024

OK, this is getting ridiculous...

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

Successfully merging this pull request may close these issues.

2 participants