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

Host local storage support #1076

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

torchiaf
Copy link
Collaborator

@torchiaf torchiaf commented Jul 19, 2024

Summary

PR Checklist

  • Is this a multi-tenancy feature/bug?
    • Yes, the relevant RBAC changes are at:
  • Do we need to backport changes to the old Rancher UI, such as RKE1?
    • Yes, the relevant PR is at:
  • Are backend engineers aware of UI changes?
    • Yes, the backend owner is:

Related Issue harvester/harvester#6057
Related Issue harvester/harvester#5953

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

  • Add a LVM disk

image

  • Add LVM storage class

image

Copy link

mergify bot commented Sep 20, 2024

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

@torchiaf torchiaf force-pushed the 6057-local-path-storage branch 2 times, most recently from 9273c79 to 28d3fab Compare September 22, 2024 16:47
@tserong
Copy link

tserong commented Sep 23, 2024

I've done some initial testing of this, and provisioning and unprovisioning LHv1 and LHv2 disks seems to work, but I've found two things so far that need tweaking:

  1. This PR sets a storageclass parameter named engineVersion with value LonghornV1 or LonghornV2, but it should instead be dataEngine with value v1 or v2 (see https://raw.githubusercontent.com/longhorn/longhorn/v1.7.1/examples/v2/storageclass.yaml for an example of a V2 storage class).

  2. spec.fileSystem.provisioned is deprecated in favour of spec.provision, but it seems we're still setting spec.fileSystem.provisioned here. If I use this PR to provision a Longhorn V2 disk for example, then check the BD CR, I get this:

> kubectl -n longhorn-system get bds/989754e4e66edadfd3390974a1aba3f8 -o yaml 
[...]
spec:
  devPath: /dev/sdb
  fileSystem:
    mountPoint: ""
    provisioned: true
  nodeName: harvester-node-0
  provision: false
  provisioner:
    longhorn:
      engineVersion: LonghornV2
[...]

...but it should be:

spec:
  devPath: /dev/sdb
  fileSystem:
    mountPoint: ""
  nodeName: harvester-node-0
  provision: true
  provisioner:
    longhorn:
      engineVersion: LonghornV2

@torchiaf
Copy link
Collaborator Author

Thanks @tserong , the code is now updated to handle the v1/v2 engineVersion in Storage Class page and use the new spec.provision field in blockDevice crd in Disks assignement.

Copy link

mergify bot commented Sep 24, 2024

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

@tserong
Copy link

tserong commented Sep 24, 2024

Thanks @torchiaf, the spec.provision field is working correctly now.

The StorageClass is not quite right - the v1/v2 value is correct, but the parameter name engineVersion needs to be changed to dataEngine (sorry if this wasn't clear earlier, and I know it's probably a pain that this is a different name than what's in the block device CR).

I also have a question: previously there was a "force formatted" checkbox for Longhorn V1:

image

...but this isn't present anymore, although there's still a tip about it:

image

Is this correct?


I also have a problem provisioning Longhorn V1 disks. They seem to add successfully, but shortly thereafter, they aren't mounted and are unschedulable. I suspect this is actually a problem in node-disk-manager. I'll report back once I've figured out what's going on with this.

image

@tserong
Copy link

tserong commented Sep 24, 2024

...oh also, one more thing: After a disk is provisioned, do we need to show what provisioner is used when viewing the disks on that node? Because there's no indication of that currently - you just see the disk like this:

image

@Vicente-Cheng, WDTY? Also, I haven't tested what's shown here in the LVM case.

@torchiaf
Copy link
Collaborator Author

@tserong I've fixed the dataEngine parameter, thanks.

For the other points:

  • We don't show the 'Force Formatted' checkbox if the disk has already been formatted. This mechanism is unchanged from master:
    • We show up the Force Formatted warning message if any of the new disks is formatted -> blockDevice.spec.fileSystem.forceFormatted. Could you verify this value? I can't replicate it in my environment.

image

  • Not sure what was the root cause of provisioning errors, let me know if any API parameter from the UI is wrong.

@Vicente-Cheng
Copy link

Vicente-Cheng commented Sep 24, 2024

...oh also, one more thing: After a disk is provisioned, do we need to show what provisioner is used when viewing the disks on that node? Because there's no indication of that currently - you just see the disk like this:

image

@Vicente-Cheng, WDTY? Also, I haven't tested what's shown here in the LVM case.

I checked with the latest changes. It looks like @torchiaf already added the provisioner fields.
截圖 2024-09-25 上午1 22 41

I thought the current display looked good to me. WDYT? @tserong

We show up the Force Formatted warning message if any of the new disks is formatted -> blockDevice.spec.fileSystem.forceFormatted. Could you verify this value? I can't replicate it in my environment.

I thought we could keep that, but only for the v1 engine.
For v2 engine, we should not let users select any formatted options.

I also have a problem provisioning Longhorn V1 disks. They seem to add successfully, but shortly thereafter, they aren't mounted and are unschedulable. I suspect this is actually a problem in node-disk-manager. I'll report back once I've figured out what's going on with this.

I will check this... It is not related to UI issue. The CRD fields looks good.
Fixed, PR here: harvester/node-disk-manager#139

BTW, I checked the whole LVM flow, and everything looks fine. Thanks!

@tserong
Copy link

tserong commented Sep 25, 2024

Thanks @Vicente-Cheng, @torchiaf! Will re-test this morning.

'vgName'
];

const DEFAUL_TOPOLOGIES = [{
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo DEFAUL_TOPOLOGIES

…Code rafactoring.

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
…rovisioner parameters

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
- Add lvm driver addon
- Add longhorn v2 engine in provisioner dropdowns
- Save storage class parameters for longhorn
- Decouple Storage class longhron params
- Add create new option for LVM groups

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
…sion in blockDevice CR

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Copy link
Collaborator Author

@tserong @Vicente-Cheng I've changed the AccessMode for LonghornV2 disks and limited the VolumeBindingMode to WaitForFirstConsumer in Storage Classes create page.

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Copy link

mergify bot commented Sep 27, 2024

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

@tserong
Copy link

tserong commented Sep 27, 2024

Thanks @torchiaf, I've just tested and the AccessMode change for LHv2 volumes works fine when the new volumes are created on the Volumes page. For new volumes created while creating virtual machines though (Virtual Machines: Create: Volumes: Add Volume), it's still using ReadWriteMany.

@Vicente-Cheng
Copy link

Thanks @torchiaf, I have tested the latest changes.
Looks the VolumeBindingMode works good with WaitForFirstConsumer with StorageClass page.

But I tested the volume creation. The accessMode is still RWX instead RWO. See below
截圖 2024-09-26 下午4 45 51

Create Vol002 from UI, then check the accessMode
截圖 2024-09-26 下午4 46 04

Could you help to check it again?
Thanks!

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

Successfully merging this pull request may close these issues.

4 participants