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.

Sign up for GitHub

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

Jump to bottom

Decompose the provisioner #109

Merged
merged 4 commits into from
Aug 21, 2024
Merged

Decompose the provisioner #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d1b5f52
CRD: add Provisioner field
Vicente-Cheng May 30, 2024
c8d0bec
controller: decompose the LH provisioner
Vicente-Cheng Jun 5, 2024
02a2d8d
vendor: bump wrangler v3 and k8s v0.30.3
Vicente-Cheng Aug 13, 2024
b1e3382
common: move to wrangler v3 and update the generated codes
Vicente-Cheng Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
84 changes: 56 additions & 28 deletions deploy/charts/harvester-node-disk-manager/templates/crds/harvesterhci.io_blockdevices.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@

---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
{}
creationTimestamp: null
name: blockdevices.harvesterhci.io
spec:
group: harvesterhci.io
Expand All @@ -23,7 +21,7 @@ spec:
- jsonPath: .status.deviceStatus.details.deviceType
name: Type
type: string
- jsonPath: .spec.devPath
- jsonPath: .status.deviceStatus.devPath
name: DevPath
Copy link
Contributor

Choose a reason for hiding this comment

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

A general question about NDM which I've only thought to ask just now while reviewing this PR: Why do we have both spec.DevPath and status.deviceStatus.devPath? Can these two values ever be different? If not, should we remove one of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spec.DevPath means the dev path when we create the blockdevice CRD (means the first time and fixed)
The status.deviceStatus.devPath means the current dev path.

So I thought showing the status.deviceStatus.devPath seems more useful for us.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah showing the actual current path makes sense.

I've just realised this (kinda) explains harvester/harvester#6224 where there were discrepancies between spec.DevPath and status.deviceStatus.devPath. I mean, there never should have been three BDs that all had the same status.deviceStatus.devPath, and I assume that was (somehow) caused by multiple dodgy devices with the same WWN, but the fact that one is the path when the device was added, and one is the current path, at least sheds some light on the discrepancies I noticed in that issue.

Thinking aloud: Maybe we should just drop spec.DevPath? Does it help at all to know what the path was originally? Note: I'm not suggesting we make any further changes to this PR in this area. Just an item for thought/consideration later.

Copy link
Collaborator Author

@Vicente-Cheng Vicente-Cheng Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we should just drop spec.DevPath

+1, it's a redundant field for me with the current design.
We can do more cleanup for these fields. I plan to introduce a new naming mechanism to replace the current one in the next release (means v1.5). That we would be good at some devices w/o wwn. Maybe we could redesign these fields at that time.

We still need to check the WWN duplicated. But we could have better handling with those devices w/o wwn.

UPDATE

I assume that was (somehow) caused by multiple dodgy devices with the same WWN, but the fact that one is the path when the device was added, and one is the current path, at least sheds some light on the discrepancies I noticed in that issue.

I am also curious about that part. We should make sure the duplicated wwn is not added, but somehow, it was added. Maybe there still is a corner case on the wwn checking mechanism.

Copy link
Member

@votdev votdev Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can these two values ever be different?

Yes. If you have a device file like /dev/disk/by-path/pci-0000:00:10.0-scsi-0:0:0:0 this can change when the PCI slots are reordered, e.g. by adding new hardware. Therefore /dev/disk/by-id/ should be used because they are predictable due the fact that these device files contains the WWN or serial number of the disk for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @votdev,
Yeah, if we would like to keep the spec.devPath field. We should use a meaningful path like the above instead of the /dev/sdx. As I mentioned above, we could redesign the field and naming mechanism on v1.5.

type: string
- jsonPath: .status.deviceStatus.fileSystem.mountPoint
Expand All @@ -43,14 +41,19 @@ spec:
openAPIV3Schema:
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
Expand All @@ -66,9 +69,9 @@ spec:
overwrite the existing one
type: boolean
mountPoint:
description: 'DEPRECATED: no longer use and has no effect. a string
with the partition''s mount point, or "" if no mount point was
discovered'
description: |-
DEPRECATED: no longer use and has no effect.
a string with the partition's mount point, or "" if no mount point was discovered
type: string
provisioned:
description: a bool indicating whether the filesystem can be provisioned
Expand All @@ -84,8 +87,39 @@ spec:
nodeName:
description: name of the node to which the block device is attached
type: string
provisioner:
properties:
longhorn:
description: a provisioner for provision Longhorn volume backend
disk
properties:
engineVersion:
description: a string with the engine version for the provisioner
enum:
- LonghornV1
- LonghornV2
type: string
required:
- engineVersion
type: object
lvm:
description: a provisioner for provision LVM volume backend disk
properties:
parameters:
description: a string slice for the parameters
items:
type: string
type: array
vgName:
description: a string with the volume group name for the provisioner
type: string
required:
- vgName
type: object
type: object
tags:
description: a string with for device tag for provisioner, e.g. "default,small,ssd"
description: a string slice with device tag for provisioner, e.g.
["default", "small", "ssd"]
items:
type: string
type: array
Expand Down Expand Up @@ -156,10 +190,9 @@ spec:
- part
type: string
driveType:
description: a string represents the type of drive bus, options
are "HDD", "FDD", "ODD", or "SSD", which correspond to a
hard disk drive (rotational), floppy drive, optical (CD/DVD)
drive and solid-state drive
description: |-
a string represents the type of drive bus, options are "HDD", "FDD", "ODD", or "SSD",
which correspond to a hard disk drive (rotational), floppy drive, optical (CD/DVD) drive and solid-state drive
enum:
- HDD
- FDD
Expand Down Expand Up @@ -207,9 +240,9 @@ spec:
- Unknown
type: string
uuid:
description: UUID is a filesystem-level UUID, which is retrieved
from the filesystem metadata inside the partition This would
be volume UUID on macOS, PartUUID on linux, empty on Windows
description: |-
UUID is a filesystem-level UUID, which is retrieved from the filesystem metadata inside the partition
This would be volume UUID on macOS, PartUUID on linux, empty on Windows
type: string
vendor:
description: a string with the name of the hardware vendor
Expand Down Expand Up @@ -253,8 +286,9 @@ spec:
- type
type: object
parentDevice:
description: a string with the parent device path of the disk,
e.g. "/dev/sda" e.g `/dev/sda` is the parent for `/dev/sda1`
description: |-
a string with the parent device path of the disk, e.g. "/dev/sda"
e.g `/dev/sda` is the parent for `/dev/sda1`
type: string
partitioned:
description: a bool indicating if the disk is partitioned
Expand Down Expand Up @@ -299,9 +333,3 @@ spec:
served: true
storage: true
subresources: {}
status:
acceptedNames:
kind: ""
plural: ""
conditions: []
storedVersions: []
74 changes: 36 additions & 38 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,34 @@
module github.com/harvester/node-disk-manager

go 1.22.0

toolchain go1.22.3
go 1.22.5

replace (
gopkg.in/yaml.v3 => gopkg.in/yaml.v3 v3.0.0-20220521103104-8f96da9f5d5e
k8s.io/api => k8s.io/api v0.27.15
k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.27.15
k8s.io/apimachinery => k8s.io/apimachinery v0.27.15
k8s.io/apiserver => k8s.io/apiserver v0.27.15
k8s.io/cli-runtime => k8s.io/cli-runtime v0.27.15
k8s.io/client-go => k8s.io/client-go v0.27.15
k8s.io/cloud-provider => k8s.io/cloud-provider v0.27.15
k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.27.15
k8s.io/code-generator => k8s.io/code-generator v0.27.15
k8s.io/component-base => k8s.io/component-base v0.27.15
k8s.io/component-helpers => k8s.io/component-helpers v0.27.15
k8s.io/controller-manager => k8s.io/controller-manager v0.27.15
k8s.io/cri-api => k8s.io/cri-api v0.27.15
k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.27.15
k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.27.15
k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.27.15
k8s.io/kube-proxy => k8s.io/kube-proxy v0.27.15
k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.27.15
k8s.io/kubectl => k8s.io/kubectl v0.27.15
k8s.io/kubelet => k8s.io/kubelet v0.27.15
k8s.io/kubernetes => k8s.io/kubernetes v1.27.15
k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.27.15
k8s.io/metrics => k8s.io/metrics v0.27.15
k8s.io/mount-utils => k8s.io/mount-utils v0.27.15
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.27.15
k8s.io/api => k8s.io/api v0.30.3
k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.30.3
k8s.io/apimachinery => k8s.io/apimachinery v0.30.3
k8s.io/apiserver => k8s.io/apiserver v0.30.3
k8s.io/cli-runtime => k8s.io/cli-runtime v0.30.3
k8s.io/client-go => k8s.io/client-go v0.30.3
k8s.io/cloud-provider => k8s.io/cloud-provider v0.30.3
k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.30.3
k8s.io/code-generator => k8s.io/code-generator v0.30.3
k8s.io/component-base => k8s.io/component-base v0.30.3
k8s.io/component-helpers => k8s.io/component-helpers v0.30.3
k8s.io/controller-manager => k8s.io/controller-manager v0.30.3
k8s.io/cri-api => k8s.io/cri-api v0.30.3
k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.30.3
k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.30.3
k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.30.3
k8s.io/kube-proxy => k8s.io/kube-proxy v0.30.3
k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.30.3
k8s.io/kubectl => k8s.io/kubectl v0.30.3
k8s.io/kubelet => k8s.io/kubelet v0.30.3
k8s.io/kubernetes => k8s.io/kubernetes v1.30.3
k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.30.3
k8s.io/metrics => k8s.io/metrics v0.30.3
k8s.io/mount-utils => k8s.io/mount-utils v0.30.3
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.30.3
)

require (
Expand All @@ -43,15 +41,15 @@ require (
github.com/pilebones/go-udev v0.0.0-20210126000448-a3c2a7a4afb7
github.com/pkg/errors v0.9.1
github.com/prometheus/procfs v0.10.1
github.com/rancher/lasso v0.0.0-20240415182150-5993b260dd08
github.com/rancher/wrangler/v2 v2.1.4
github.com/rancher/lasso v0.0.0-20240705194423-b2a060d103c1
github.com/rancher/wrangler/v3 v3.0.0
github.com/sirupsen/logrus v1.9.3
github.com/stretchr/testify v1.9.0
github.com/urfave/cli/v2 v2.3.0
golang.org/x/crypto v0.22.0
k8s.io/api v0.30.0
k8s.io/apimachinery v0.30.0
k8s.io/client-go v0.28.6
k8s.io/api v0.30.3
k8s.io/apimachinery v0.30.3
k8s.io/client-go v0.30.3
k8s.io/utils v0.0.0-20240310230437-4693a0247e57
)

Expand All @@ -63,7 +61,7 @@ require (
github.com/coreos/go-systemd/v22 v22.5.0 // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/emicklei/go-restful/v3 v3.9.0 // indirect
github.com/emicklei/go-restful/v3 v3.11.0 // indirect
github.com/evanphx/json-patch v5.6.0+incompatible // indirect
github.com/felixge/httpsnoop v1.0.3 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
Expand All @@ -76,7 +74,6 @@ require (
github.com/godbus/dbus/v5 v5.1.0 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/gnostic v0.7.0 // indirect
github.com/google/gnostic-models v0.6.9-0.20230804172637-c7be7c783f49 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/gofuzz v1.2.0 // indirect
Expand Down Expand Up @@ -123,10 +120,11 @@ require (
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
howett.net/plist v0.0.0-20181124034731-591f970eefbb // indirect
k8s.io/code-generator v0.28.6 // indirect
k8s.io/gengo v0.0.0-20220902162205-c0856e24416d // indirect
k8s.io/code-generator v0.30.3 // indirect
k8s.io/gengo v0.0.0-20240228010128-51d4e06bde70 // indirect
k8s.io/gengo/v2 v2.0.0-20240228010128-51d4e06bde70 // indirect
k8s.io/klog/v2 v2.120.1 // indirect
k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9 // indirect
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
k8s.io/mount-utils v0.29.4 // indirect
sigs.k8s.io/controller-runtime v0.10.1 // indirect
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
Expand Down
Loading
Loading