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

Support for additional Bus Types #150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ricardo-larosa
Copy link

@ricardo-larosa ricardo-larosa commented Mar 3, 2023

Description

Add support for busType: nvme, sata, scsi_paravirtual, scsi_lsi_logic_parallel, scsi_lsi_logic_sas, scsi_buslogic

Checklist

  • tested with TestDiskCreateAttach
  • tested with e2e TestCSIAutomation
  • updated README and examples provided

TestDiskCreateAttach

Now is a table-test to allow testing of the different busTypes.
Screenshot 2023-03-03 at 09 03 51

E2E tests

I created a new argument busType in CreateStorageClass and CreatePV from cloud-provider-for-cloud-director testingsdk.
The PR is vmware/cloud-provider-for-cloud-director#233 and needs to be merged In order to run these tests.
However, I included these changes in this PR under vendor to allow you run the test. Obviously, such changes will be overwritten if you run go mod vendor before merging that PR.

The e2etest provides an optional --busType flag (defaults to scsi_paravirtual). Therefore, you can test all busTypes.
It also provide new optional flags:

  • --storageProfile (defaults to "*")
  • --userOrg (defaults to "system")

Since these busTypes have different readiness times please allow extra time by increasing the timeouts, I suggest -timeout=20m

In my environment, the 3 storageProfile tests fail, because the storage policies in the org that I am using are not configured the way the tests expects them to be.
Screenshot 2023-03-03 at 09 53 00

getDiskPath

Now implemented in pure Go. It does not longer run a command in console. The scan is now faster.
It uses the well-known library ghw from Jay Pipes.
In order to have this new functionality working the DeamonSet csi-vcd-nodeplugin from csi-node-crs.yaml in CAPVCD will need to mount /run and use a propagation HostToContainer.

          volumeMounts:
            ...
            - mountPath: /run
              name: host-run-dir
              mountPropagation:  "HostToContainer"
      volumes:
       ...
        - name: host-run-dir
          hostPath:
            path: /run
            type: Directory

Related PR: vmware/cluster-api-provider-cloud-director#414

BusTypes not supported

IDE controllers do not support VMs I left it out of scope.

Backwards compatibility

Existing PVs and StorageClasses are not modified by this change.


This change is Reviewable

Copy link
Contributor

@ymo24 ymo24 left a comment

Choose a reason for hiding this comment

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

Reviewed 19 of 218 files at r1, all commit messages.
Reviewable status: 19 of 218 files reviewed, 2 unresolved discussions (waiting on @ricardo-larosa)

a discussion (no related file):
Would you please restore the edits on vendor/github.com/vmware/cloud-provider-for-cloud-director/pkg/testingsdk/client.go and
vendor/github.com/vmware/cloud-provider-for-cloud-director/pkg/testingsdk/k8sclient.go?
I already removed storageClass related functions inside common code (CPI). Please use go mod vendor to get the latest code under cloud-provider-for-cloud-director.



pkg/csi/controller.go line 39 at r1 (raw file):

	DiskUUIDAttribute   = "diskUUID"
	FileSystemAttribute = "filesystem"
	DefaultFS           = "ext4"

Would you please use DefaultFSType instead?

Copy link
Contributor

@ymo24 ymo24 left a comment

Choose a reason for hiding this comment

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

BusLogic Parallel SCSI controller

Reviewed 1 of 218 files at r1.
Reviewable status: 20 of 218 files reviewed, 3 unresolved discussions (waiting on @ricardo-larosa)

a discussion (no related file):

Previously, ymo24 wrote…

Would you please restore the edits on vendor/github.com/vmware/cloud-provider-for-cloud-director/pkg/testingsdk/client.go and
vendor/github.com/vmware/cloud-provider-for-cloud-director/pkg/testingsdk/k8sclient.go?
I already removed storageClass related functions inside common code (CPI). Please use go mod vendor to get the latest code under cloud-provider-for-cloud-director.

Also, please add BusLogic SCSIas one of the busType



pkg/vcdcsiclient/disks.go line 46 at r1 (raw file):

		"scsi_paravirtual":        {"6", "VirtualSCSI"},
		"scsi_lsi_logic_parallel": {"6", "lsilogic"},
		"scsi_lsi_logic_sas":      {"6", "lsilogicsas"},

please add BusLogic SCSIas one of the busType

pkg/csi/controller.go Outdated Show resolved Hide resolved
@ricardo-larosa ricardo-larosa force-pushed the support-add-bus-types branch 3 times, most recently from e1e60c9 to 2008b42 Compare May 2, 2023 17:05
@ricardo-larosa
Copy link
Author

Depends on #178

- Adds support for busType: nvme, sata, scsi_paravirtual, scsi_lsi_logic_parallel, scsi_lsi_logic_sas, scsi_buslogic
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.

None yet

4 participants