Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

New DeviceManager interface #46

Merged
merged 5 commits into from
Oct 29, 2018
Merged

New DeviceManager interface #46

merged 5 commits into from
Oct 29, 2018

Conversation

avalluri
Copy link
Contributor

These set of changes defines/implements new PmemDeviceManger interface, this hides the actual device management part from driver

Signed-off-by: Amarnath Valluri <[email protected]>
@okartau
Copy link
Contributor

okartau commented Oct 29, 2018

excellent idea to separate device interface, allows cleaner implementation of #26

Current code is using two variants(lvm,ndctl) for managing(create,delete)
underlined pmem devices. The better solution would be to define a generic
interface , which should be implemented by different implementations.

Signed-off-by: Amarnath Valluri <[email protected]>
@avalluri avalluri force-pushed the avalluri/devicemanager branch 2 times, most recently from dfe6d86 to f760cab Compare October 29, 2018 08:18
@avalluri
Copy link
Contributor Author

@okartau Now i fixed the make test failure. Can you please review.

@okartau
Copy link
Contributor

okartau commented Oct 29, 2018

This change increases csi-sanity error count to 1 (was: zero):
Node Service
.../github.com/kubernetes-csi/csi-test/pkg/sanity/tests.go:44
NodeStageVolume
.../github.com/kubernetes-csi/csi-test/pkg/sanity/node.go:244
should fail when no volume capability is provided [It]
.../github.com/kubernetes-csi/csi-test/pkg/sanity/node.go:309

Expected
    <codes.Code>: 2
to equal
    <codes.Code>: 3

.../github.com/kubernetes-csi/csi-test/pkg/sanity/node.go:325

@okartau
Copy link
Contributor

okartau commented Oct 29, 2018

looks like we return Unknown (value 2) instead of InvalidArgument (value 3) somewhere in NodeStageVolume

@okartau
Copy link
Contributor

okartau commented Oct 29, 2018

we don't have explicit check for capabilities existence in NodeStageVolume handler, not before nor after this change. Seems it still returned correct code before via some side effect. I propose we add explicit check for capabilities existence

@okartau
Copy link
Contributor

okartau commented Oct 29, 2018

make test is OK now!

@avalluri
Copy link
Contributor Author

avalluri commented Oct 29, 2018

looks like we return Unknown (value 2) instead of InvalidArgument (value 3) somewhere in NodeStageVolume

I think i found the issue behind this failure:

+	device, err := ns.dm.GetDevice(req.VolumeId)
+	if err != nil {
+		pmemcommon.Infof(3, ctx, "NodeStageVolume: did not find volume %s", attrs["name"])
+		return nil, err
 	}

@avalluri avalluri force-pushed the avalluri/devicemanager branch from f760cab to 7185764 Compare October 29, 2018 09:19
@okartau
Copy link
Contributor

okartau commented Oct 29, 2018

now csi-sanity test passes with zero errors

}
volumeGroups := []string{}
for _, bus := range ctx.GetBuses() {
glog.Infof("CreateDevice: Bus: %v", bus.DeviceName())
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, and 2 lines below, perhaps could start line with function name instead of CreateDevice: and CreateVolume. At least my initial idea with those comment was, indicate function name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will fix and resubmit

Added new command line option 'deviceManager' to select which device manager to
use for managing pmem devices, defaults to 'lvm'.

Signed-off-by: Amarnath Valluri <[email protected]>
@avalluri avalluri force-pushed the avalluri/devicemanager branch from 7185764 to 00d315c Compare October 29, 2018 14:05
@avalluri avalluri merged commit 7d0fcf8 into devel Oct 29, 2018
@avalluri avalluri deleted the avalluri/devicemanager branch October 30, 2018 12:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants