Devicemanager: New error codes + Device Manager unit tests and few other fixes. #420

avalluri · 2019-09-25T13:00:33Z

This PR address a couple of issues:

Defined new error codes that are returned by DeviceManager interface methods. Made changes to implementations to use these error codes and revised the error messages using error formatting features provided by go-1.13 fmt package.
Discussion on issue cache data in LVM device manager? #413, raised a requirement that PMEM-CSI should handle the DeleteVolume() call for volumes that the underlined device already deleted manually by the admin. So for such a volume we just ignore and return no error in DeviceManager.DeleteVolume(). This also makes DeleteVolume() idempotent.
Issue DeleteVolume response is incorrect if volume is in use #402 : CSI.DeleteVolume should return FailedPreCondition error in case of the volume is in use. Test case added for the same.
Unit tests for device manager
This PR also contains a commit that removes the unused code in DeviceManager.

pohly

Can you add tests for this, both for direct mode and LVM mode?

avalluri · 2019-10-09T12:46:26Z

Can you add tests for this, both for direct mode and LVM mode?

Tests for device manager is bit challenging, as they deal with devices. I stared with LVM tests: avalluri@c7437e5 .
The ugly part is how we run the test, as it needs sudo permissions. I tried running test inside the container and ended up like this:avalluri@c7437e5#diff-436cc358f7f160c8242374b3f820867d

avalluri · 2019-10-23T12:54:33Z

Can you add tests for this, both for direct mode and LVM mode?

@pohly, I added unit tests for LVM, but I am not sure yet about covering ndctl manager, which needs a bit thinking how to mimic the pmem device.

okartau · 2019-10-23T16:11:57Z

pkg/pmem-device-manager/pmme-lvm_test.go

@@ -0,0 +1,229 @@
+/*


name of that file seems misspelled, should be pmem-lvm_test.go ?

Yes, it should be pmd-lvm_test.go. I fixed it now.

pkg/pmem-device-manager/pmme-lvm_test.go

vendor-bom.csv

avalluri · 2019-10-24T08:43:25Z

Device manager tests failed in CI while setting up loop device:

[2019-10-24T07:52:16.650Z] DeviceManage LVM
[2019-10-24T07:52:16.650Z] /go/src/github.com/intel/pmem-csi/pkg/pmem-device-manager/pmd-lvm_test.go:35
[2019-10-24T07:52:16.650Z]   LVM [BeforeEach]
[2019-10-24T07:52:16.650Z]   /go/src/github.com/intel/pmem-csi/pkg/pmem-device-manager/pmd-lvm_test.go:54
[2019-10-24T07:52:16.650Z]     Should create a new device
[2019-10-24T07:52:16.650Z]     /go/src/github.com/intel/pmem-csi/pkg/pmem-device-manager/pmd-lvm_test.go:55
[2019-10-24T07:52:16.650Z] 
[2019-10-24T07:52:16.650Z]     Failed to create volume group
[2019-10-24T07:52:16.650Z]     Expected
[2019-10-24T07:52:16.650Z]         <*errors.errorString | 0xc000021700>: {
[2019-10-24T07:52:16.650Z]             s: "could not open /dev/loop-control: open /dev/loop-control: no such file or directory",
[2019-10-24T07:52:16.650Z]         }
[2019-10-24T07:52:16.650Z]     to be nil
[2019-10-24T07:52:16.650Z] 
[2019-10-24T07:52:16.650Z]     /go/src/github.com/intel/pmem-csi/pkg/pmem-device-manager/pmd-lvm_test.go:42

pohly

As discussed in #383 (review), please also revise error reporting and checking such that the error code returned by ns.cs.dm.GetDevice in nodeserver.go is checked.

pohly · 2019-10-30T13:22:45Z

Makefile

@@ -13,6 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

+GO=GOOS=linux GO111MODULE=off go


Why GOOS=linux?

Why GOOS=linux?

This is not added part of this change. It was existing before, I just moved from that place.

We weren't consistent then. I was looking at the first changed line and that only had:

build: $(CMDS) $(TEST_CMDS) go test -run none ./pkg/... ./test/e2e

Dockerfile

pohly · 2019-10-31T11:14:44Z

test/test.make

-	$(TEST_CMD) $(shell go list $(TEST_ARGS) | sed -e 's;$(IMPORT_PATH);.;')
+	$(TEST_CMD) $(shell $(GO) list $(TEST_ARGS) | grep -v pmem-device-manager | sed -e 's;$(IMPORT_PATH);.;')
+RUN_DM_TESTS = \
+    $(TEST_CMD) ./pkg/pmem-device-manager -exec $(SUDO)


This makes "make test" depend on sudo, which is not currently a requirement and also shouldn't be. Developers should be able to build and test on a machine with a user account that isn't allowed to become root.

The other issue is that the test depends on /dev/loop-control, right?

Let me propose a different approach to running tests that need to be run in a certain environment:

in the test code, check those preconditions (are root, have /dev/loop-control): if not met, skip the test

integrate the running of unit tests into our E2E testing:

build test binaries with go test -c

copy test binaries into the first worker

run them there as root

This is more complicated, but it also gives us better test coverage, because then operations that may depend on the OS kernel of the cluster node actually get tested with that kernel. We can also do unit tests that depend on actual PMEM.

This makes "make test" depend on sudo, which is not currently a requirement and also shouldn't be. Developers should be able to build and test on a machine with a user account that isn't allowed to become root.

To avoid this root user. I initially tried ruining these tests in a container (#420 (comment)).

The other issue is that the test depends on /dev/loop-control, right?

Let me propose a different approach to running tests that need to be run in a certain environment:

in the test code, check those preconditions (are root, have /dev/loop-control): if not met, skip the test

integrate the running of unit tests into our E2E testing:

build test binaries with go test -c

copy test binaries into the first worker

run them there as root

This is more complicated, but it also gives us better test coverage, because then operations that may depend on the OS kernel of the cluster node actually get tested with that kernel. We can also do unit tests that depend on actual PMEM.

Ok, Instead of running these(device manager) unit tests in full(3-node) test cluster, I would propose to run them in a single virtual machine node. I pushed changes to support this: 45f21a6

This single node vm installs only dependencies needed for PMEM, such as LVM2, ndctl. No other packages like Docker, Kubernetes.

Ok, Instead of running these(device manager) unit tests in full(3-node) test cluster, I would propose to run them in a single virtual machine node.

That's a massive change to some already pretty complex scripts. What's the advantage compared to just using the normal VM cluster?

We need to consider two different use cases:

a developer using this while making changes to the source code

the CI jobs

That's a massive change to some already pretty complex scripts.

No, it's not that massive change as it looks in the PR. Most of the changes are indentations. It's just replacing few bits in the right place in test/start-kubernetes.sh script.

Defined package level error codes. The implementations are supposed to return these error codes. And also revised the error message formatting and wrapping using go-1.13 fmt package.

Discussion on issue intel#413, raised a requirement that PMEM-CSI should handle the DeleteVolume() call for volumes that the underlined device already deleted manually by the admin. So for such a volume we just ignore and return no error in DeviceManager.DeleteVolume(). This also makes DeleteVolume() idempotent.

pohly · 2019-11-21T09:10:22Z

@avalluri: so what did you change to fix the failure? It's a bit hard to tell because of the rebase. Is it 8b22191 and thus #472?

avalluri · 2019-11-21T09:14:17Z

@avalluri: so what did you change to fix the failure? It's a bit hard to tell because of the rebase. Is it 8b22191 and thus #472?

I am about to mention that :) Yes, This should fix.

avalluri · 2019-11-21T09:17:09Z

I believe it is better to run this test inside the existing steps

If this what you feel right, we can still run using make run_dm_tests in CI, then it picks the first node in the default cluster to run device manager tets.

avalluri · 2019-11-21T09:23:17Z

@pohly I took your change pohly@a9e7136 and squashed to ff07726. Thanks.

avalluri · 2019-11-21T11:59:59Z

DestroyNamespace will first deactivate a namespace, then destroy it. Suppose the driver crashes or gets killed after the first and before the second step. We are left with an inactive namespace that shouldn't be ignored.

Interesting question. Ya, may be true. We cannot ignore that namespace in this case.

avalluri · 2019-11-21T12:02:44Z

DestroyNamespace will first deactivate a namespace, then destroy it. Suppose the driver crashes or gets killed after the first and before the second step. We are left with an inactive namespace that shouldn't be ignored.

Interesting question. Ya, may be true. We cannot ignore that namespace in this case.

By the way, we already dealing with only active namespaces for ListDevices, this case affects this also.

avalluri · 2019-11-21T12:41:37Z

There is something weird going on in CI, 1-16 Direct tests running for 2h 28m but no results yet. Somehow got stuck. I am not sure if the issue with the code or something else.

pohly · 2019-11-21T13:13:42Z

Interesting question. Ya, may be true. We cannot ignore that namespace in this case.

So what do you think of the solution I have here: 45f7c42#diff-b99dbbc6a60be19c9eacba35ad8a5526

By the way, we already dealing with only active namespaces for ListDevices, this case affects this also.

What do we need to fix for ListDevices? Should it report inactive, non-empty namespaces and currently ignores them?

pohly · 2019-11-21T13:15:15Z

There is something weird going on in CI, 1-16 Direct tests running for 2h 28m but no results yet. Somehow got stuck. I am not sure if the issue with the code or something else.

You could try running the E2E tests in that mode locally.

avalluri · 2019-11-21T13:25:04Z

So what do you think of the solution I have here: 45f7c42#diff-b99dbbc6a60be19c9eacba35ad8a5526

Should work, I couldn't see any issues with that.

What do we need to fix for ListDevices? Should it report inactive, non-empty namespaces and currently ignores them?

Yes, it also should have that fix. Maybe we better fix this ndctl layer.

pohly · 2019-11-21T15:12:15Z

pkg/ndctl/region.go

@@ -93,7 +93,7 @@ func (r *Region) ActiveNamespaces() []*Namespace {
 	return r.namespaces(true)
 }

-//AllNamespaces returns all namespaces in the region
+//AllNamespaces returns all non-zero sized namespaces in the region


Can you add a comment here explaining why it ignores namespaces with zero size? I know its in the commit message, but that's not necessarily where someone will look when reading this description.

pohly · 2019-11-21T15:17:53Z

pkg/ndctl/region.go

-		if !onlyActive || C.ndctl_namespace_is_active(ndns) == true {
-			namespaces = append(namespaces, (*Namespace)(ndns))
+		ns := (*Namespace)(ndns)
+		if (onlyActive && ns.Active()) || ns.Size() != 0 {


Doesn't this break the semantic of onlyActive?

If it is true, then inactive namespaces are returned if their size is > 0, which is not what the caller asked for, is it?

Yes, you are right, Fixed it now. Thanks.

pohly · 2019-11-21T15:57:29Z

@avalluri: Did you also make some more changes to the Jenkinsfile? It's no longer valid: https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-420/44/pipeline

…paces While deleting a namespace ndctl might still keeep the namespace inactive and with size zero. And also there might be possible that the unsuccessful deletes via Region.DestroyNamespace() might left the namespace disabled but not deleted, so we should not ignore them too. This change, considers the size of the found namespace and considers only if it's non-zero.

Tests for both device managers: direct and LVM. LVM tests used file backed loop device to mimic an logical volume group, This requires /dev/loop-control file on host where the tests are run. Intentionally disable running these device manager unit tests as it requires sudo/root user permission.

Device manager tests need root/sudo user permission, and loop control device on host where they run. So to avoid this we run these tests under a new virtual machine. Added new make target 'run_dm_tests' which sets up pmem-csi-${CLUSTER}-worker1 node if not exists and runs the device manager unit tests in it. To make use of existing test cluster scripts, made needed changes to them to support creating a single pmem node.

pohly · 2019-11-22T08:50:30Z

Jenkinsfile

+                               ${env.BUILD_IMAGE} bash -c 'set -x; \
+                                   swupd bundle-add openssh-client && \
+                                   make run_dm_tests; \
+                                   make stop'"


This step is now treated as success even if make run_dm_tests fails. As I intend to remove this step in #473, we can merge this PR without fixing this.

okartau mentioned this pull request Oct 7, 2019

(to be closed after #420 implements same) DeleteVolume returns FailedPrecondition if volume is in use #417

Closed

pohly suggested changes Oct 7, 2019

View reviewed changes

avalluri force-pushed the devicemanager branch 5 times, most recently from 1bc573f to 3cbca7b Compare October 23, 2019 12:49

avalluri requested review from pohly and okartau October 23, 2019 12:54

okartau reviewed Oct 23, 2019

View reviewed changes

pkg/pmem-device-manager/pmme-lvm_test.go Outdated Show resolved Hide resolved

okartau reviewed Oct 23, 2019

View reviewed changes

vendor-bom.csv Outdated Show resolved Hide resolved

avalluri force-pushed the devicemanager branch from 3cbca7b to ee26179 Compare October 24, 2019 07:39

avalluri mentioned this pull request Oct 28, 2019

Ephemeral/generic #383

Merged

pohly suggested changes Oct 28, 2019

View reviewed changes

avalluri force-pushed the devicemanager branch 4 times, most recently from 5aa343e to 127e7ac Compare October 30, 2019 11:48

pohly reviewed Oct 30, 2019

View reviewed changes

avalluri requested a review from pohly October 31, 2019 07:31

avalluri force-pushed the devicemanager branch from 127e7ac to fc7976a Compare October 31, 2019 07:55

avalluri changed the title ~~Devicemanager: Return no error if the device not found while deleting~~ Devicemanager: New error codes + LVM unit test and few other fixes. Oct 31, 2019

avalluri requested a review from okartau October 31, 2019 08:02

pohly reviewed Oct 31, 2019

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

pohly reviewed Oct 31, 2019

View reviewed changes

avalluri force-pushed the devicemanager branch from fc7976a to 127d02d Compare November 1, 2019 14:20

avalluri added 2 commits November 21, 2019 11:04

DeviceManager: define error codes

08dd1ea

Defined package level error codes. The implementations are supposed to return these error codes. And also revised the error message formatting and wrapping using go-1.13 fmt package.

avalluri force-pushed the devicemanager branch from e8b480a to 6287946 Compare November 21, 2019 09:06

avalluri force-pushed the devicemanager branch from 6287946 to 8f0a948 Compare November 21, 2019 14:42

pohly reviewed Nov 21, 2019

View reviewed changes

avalluri force-pushed the devicemanager branch 2 times, most recently from 63ca968 to c3ef004 Compare November 22, 2019 04:07

avalluri added 4 commits November 22, 2019 06:41

Jenkinsfile: Add new build stage for running device manager tests

b7cb533

avalluri force-pushed the devicemanager branch from c3ef004 to b7cb533 Compare November 22, 2019 04:42

pohly approved these changes Nov 22, 2019

View reviewed changes

pohly reviewed Nov 22, 2019

View reviewed changes

pohly merged commit 310b2ec into intel:devel Nov 22, 2019

pohly mentioned this pull request Nov 22, 2019

more flexible tracking of namespaces used by PMEM-CSI #475

Open

okartau mentioned this pull request Nov 29, 2019

DeleteVolume response is incorrect if volume is in use #402

Closed

avalluri deleted the devicemanager branch October 20, 2020 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devicemanager: New error codes + Device Manager unit tests and few other fixes. #420

Devicemanager: New error codes + Device Manager unit tests and few other fixes. #420

avalluri commented Sep 25, 2019 •

edited

Loading

pohly left a comment

avalluri commented Oct 9, 2019

avalluri commented Oct 23, 2019

okartau Oct 23, 2019

avalluri Oct 24, 2019

avalluri commented Oct 24, 2019 •

edited

Loading

pohly left a comment

pohly Oct 30, 2019

avalluri Oct 31, 2019 •

edited

Loading

pohly Oct 31, 2019

pohly Oct 31, 2019

avalluri Nov 7, 2019

avalluri Nov 7, 2019

pohly Nov 7, 2019

avalluri Nov 11, 2019

pohly commented Nov 21, 2019

avalluri commented Nov 21, 2019 •

edited

Loading

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

pohly commented Nov 21, 2019

pohly commented Nov 21, 2019

avalluri commented Nov 21, 2019

pohly Nov 21, 2019

avalluri Nov 22, 2019

pohly Nov 21, 2019

avalluri Nov 22, 2019

pohly commented Nov 21, 2019

pohly Nov 22, 2019

Devicemanager: New error codes + Device Manager unit tests and few other fixes. #420

Devicemanager: New error codes + Device Manager unit tests and few other fixes. #420

Conversation

avalluri commented Sep 25, 2019 • edited Loading

pohly left a comment

Choose a reason for hiding this comment

avalluri commented Oct 9, 2019

avalluri commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avalluri commented Oct 24, 2019 • edited Loading

pohly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avalluri Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Nov 21, 2019

avalluri commented Nov 21, 2019 • edited Loading

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

avalluri commented Nov 21, 2019

pohly commented Nov 21, 2019

pohly commented Nov 21, 2019

avalluri commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Nov 21, 2019

Choose a reason for hiding this comment

avalluri commented Sep 25, 2019 •

edited

Loading

avalluri commented Oct 24, 2019 •

edited

Loading

avalluri Oct 31, 2019 •

edited

Loading

avalluri commented Nov 21, 2019 •

edited

Loading