Implement vgmanager by sp98 · Pull Request #27 · openshift/lvm-operator

sp98 · 2021-12-19T14:54:13Z

This PR is based on the Rohan's PR

Creates VgManager Image
Adds utility for reading block devices
Implements controller for vgManager to create vgs.

Testing on SNO cluster:

VgManager Image used : quay.io/sp1098/vgmanager:latest 

$ oc version
Client Version: 4.9.0
Server Version: 4.9.0
Kubernetes Version: v1.22.0-rc.0+894a78b

# sapillai @ localhost in ~ [10:52:00]
$ oc get pv
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM          STORAGECLASS              REASON   AGE
pvc-481b5016-6126-4ed1-a42f-5be48f6d132f   10Gi       RWO            Delete           Bound    default/pvc1   topolvm-provisioner-vg1            74s

# sapillai @ localhost in ~ [10:52:03]
$ oc get pods -n lvm-operator-system
NAME                                  READY   STATUS    RESTARTS      AGE
controller-manager-54c78c8df6-mvzkl   2/2     Running   0             42m
topolvm-controller-58c8f86445-brq4x   4/4     Running   0             41m
topolvm-node-md28g                    4/4     Running   8 (40m ago)   41m
vg-manager-bcklj                      1/1     Running   0             41m

# sapillai @ localhost in ~ [10:52:30]
$ oc get pv
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM          STORAGECLASS              REASON   AGE
pvc-481b5016-6126-4ed1-a42f-5be48f6d132f   10Gi       RWO            Delete           Bound    default/pvc1   topolvm-provisioner-vg1            112s

# sapillai @ localhost in ~ [10:52:41]
$ oc get pvc
NAME   STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS              AGE
pvc1   Bound    pvc-481b5016-6126-4ed1-a42f-5be48f6d132f   10Gi       RWO            topolvm-provisioner-vg1   118s

# sapillai @ localhost in ~ [10:52:46]
$ oc get pods
NAME                                READY   STATUS    RESTARTS   AGE
rhocs-bm5labengblrredhatcom-debug   1/1     Running   0          50m
topo-test-6df5cf5457-m8gnz          1/1     Running   0          2m5s

TODO (in follow up PRs)

Revisit controller implementation of the vgManager daemoset.
Update ListBlockDevice method to read devices using --json flag

nbalacha

Initial comment: please reorder commits as follows:

blockutil
vgmanager implementation (includes main.go changes from current build commit)
build (only makefile and dockerfile changes)
go mod tidy

pkg/internal/blockutil.go

nbalacha · 2021-12-21T12:13:33Z

Dockerfile.vgmanager

+COPY controllers/ controllers/
+
+# Build
+RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o vgmanager cmd/vgmanager/main.go


We will need to build for multiple archs.

how about we do it in a new PR since we have to do it for the operator image as well ?

cmd/vgmanager/main.go

nbalacha · 2021-12-21T12:20:37Z

cmd/vgmanager/main.go

+	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
+	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
+	opts := zap.Options{
+		Development: true,


This will need to be set to false in the future. This is fine for now.

pkg/internal/blockutil.go

nbalacha · 2021-12-21T12:31:08Z

pkg/internal/blockutil.go

+}
+
+// GetSize as int64
+func (b BlockDevice) GetSize() (int64, error) {


Why not uint64?

pkg/internal/blockutil.go

pkg/vgmanager/vgmanager_controller.go

nbalacha · 2021-12-21T13:43:19Z

pkg/vgmanager/vgmanager_controller.go

+	if err != nil {
+		r.Log.Error(err, "reconcile error")
+	}
+	r.Log.V(1).Info("reconcile complete", "result", res)


What is r.Log.V(1)?

Not sure. Looks like different verbosity levels. I'll remove it.

pkg/vgmanager/vgmanager_controller.go

nbalacha · 2021-12-21T14:01:48Z

pkg/vgmanager/vgmanager_controller.go

+// filterMatchingDevices returns unmatched and matched blockdevices
+func filterMatchingDevices(blockDevices []internal.BlockDevice, lvmCluster lvmv1alpha1.DeviceClass) ([]internal.BlockDevice, []internal.BlockDevice, error) {
+	// currently just match all devices
+	return []internal.BlockDevice{}, blockDevices, nil


Does this skip the installation disk?

No. Right now there is no filter. we just return all the devices back.

I spoke too soon. Yes there is a check. There is a filter called noBiosBootInPartLabel. It checks for bios or boot in the disk part label. Based on our experience with LSO so far, it might not be enough cover all the cases.

pkg/vgmanager/lvm.go

jmolmo · 2021-12-21T17:30:24Z

controllers/vgmanager_daemonset.go

 	SysVolName    = "sys"

-	LVMdDir    = "/mnt/lvmd/"
+	LVMdDir    = "/etc/topolvm"


if this is the directory where the lvmd config file is stored, then this can cause problems, because we already have another constant with the complete file path. So in case of change, everybody must know that we need to change this other constant.
I suggest to use only the "LvmdConfigFile" constant , and if it is needed extract the base path from the constant.

Good point. I would like to take this up in a follow up PR.

jmolmo · 2021-12-21T17:33:20Z

pkg/internal/blockutil.go

+	FSType string `json:"fsType"`
+	Size   string `json:"size"`
+	// Children   []BlockDevice `json:"children,omitempty"`
+	Rotational string `json:"rota"`


Having a different names (struct/json) only can drive to confusion. I would suggest to use the same names in both parts... so instead of "rota" use "rotational" , ro/readonly, rm/removable

I would take this is up in a follow up PR while working on this comment

leelavg · 2021-12-22T06:17:23Z

pkg/vgmanager/vgmanager_controller.go

+
+	// load lvmd config
+	lvmdConfig := &lvmdCMD.Config{
+		SocketName: topolvm.DefaultLVMdSocket,


Is the value of topolvm.DefaultLVMdSocket and what's supplied at https://github.com/red-hat-storage/lvm-operator/blob/7b5eb349b5fd22374b24b6b571238999c6c68e7b/controllers/topolvm_node.go#L277 to nodeplugin same?

not same. https://github.com/topolvm/topolvm/blob/af1ff7bb2620d492e50047a041997fd86d55ac48/constants.go#L36

Read block devices properties using lsblk Co-authored-by: Rohan CJ <rohantmp@gmail.com> Signed-off-by: Santosh Pillai <sapillai@redhat.com>

Signed-off-by: Santosh Pillai <sapillai@redhat.com>

nbalacha · 2021-12-22T12:13:00Z

pkg/vgmanager/vgmanager_controller.go

+
+	// load lvmd config
+	lvmdConfig := &lvmdCMD.Config{
+		SocketName: topolvm.DefaultLVMdSocket,


Please replace this with controllers.LVMdSocketPath

Co-authored-by: Rohan CJ <rohantmp@gmail.com> Signed-off-by: Santosh Pillai <sapillai@redhat.com>

openshift-ci · 2021-12-22T12:47:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nbalacha, sp98

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sp98 changed the title ~~Implement vgmanager~~ [DNM]Implement vgmanager Dec 20, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2021

sp98 force-pushed the implement-vgmanager branch from bba2432 to 596a924 Compare December 21, 2021 11:59

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2021

sp98 changed the title ~~[DNM]Implement vgmanager~~ Implement vgmanager Dec 21, 2021

nbalacha suggested changes Dec 21, 2021

View reviewed changes

openshift-ci bot assigned nbalacha Dec 21, 2021

sp98 force-pushed the implement-vgmanager branch from 596a924 to e53a2a7 Compare December 21, 2021 13:04

sp98 commented Dec 21, 2021

View reviewed changes

pkg/internal/blockutil.go Show resolved Hide resolved

sp98 force-pushed the implement-vgmanager branch from e53a2a7 to 248ca01 Compare December 21, 2021 13:15

sp98 requested a review from nbalacha December 21, 2021 13:16

nbalacha suggested changes Dec 21, 2021

View reviewed changes

nbalacha reviewed Dec 21, 2021

View reviewed changes

pkg/vgmanager/lvm.go Outdated Show resolved Hide resolved

sp98 force-pushed the implement-vgmanager branch from 248ca01 to b4cecca Compare December 21, 2021 14:51

sp98 requested a review from nbalacha December 21, 2021 15:14

jmolmo reviewed Dec 21, 2021

View reviewed changes

sp98 force-pushed the implement-vgmanager branch from b4cecca to e4558eb Compare December 22, 2021 05:45

leelavg reviewed Dec 22, 2021

View reviewed changes

sp98 and others added 2 commits December 22, 2021 15:49

feat: implement block device reader

b31cc3f

Read block devices properties using lsblk Co-authored-by: Rohan CJ <rohantmp@gmail.com> Signed-off-by: Santosh Pillai <sapillai@redhat.com>

feat: export LvmdConfigFile var in defaults file

eaf5392

Signed-off-by: Santosh Pillai <sapillai@redhat.com>

sp98 force-pushed the implement-vgmanager branch 2 times, most recently from 9e10c23 to 6fccbdd Compare December 22, 2021 11:32

nbalacha reviewed Dec 22, 2021

View reviewed changes

sp98 and others added 3 commits December 22, 2021 17:47

feat: implement vgManager controller

81e1524

Co-authored-by: Rohan CJ <rohantmp@gmail.com> Signed-off-by: Santosh Pillai <sapillai@redhat.com>

build: add docker file for vgmanager and update makefile

474904f

Co-authored-by: Rohan CJ <rohantmp@gmail.com> Signed-off-by: Santosh Pillai <sapillai@redhat.com>

chore: go mod tidy

d787cc0

Co-authored-by: Rohan CJ <rohantmp@gmail.com> Signed-off-by: Santosh Pillai <sapillai@redhat.com>

sp98 force-pushed the implement-vgmanager branch from 6fccbdd to d787cc0 Compare December 22, 2021 12:17

nbalacha approved these changes Dec 22, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2021

nbalacha merged commit 929e2e0 into openshift:main Dec 22, 2021

leelavg mentioned this pull request Dec 24, 2021

Add VGmanager daemonset, VGManager implementation #22

Closed

8 tasks

Conversation

sp98 commented Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbalacha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sp98 Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sp98 commented Dec 19, 2021 •

edited

Loading

sp98 Dec 22, 2021 •

edited

Loading