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

Adding support for block volumes #438

Closed
wants to merge 6 commits into from
Closed

Adding support for block volumes #438

wants to merge 6 commits into from

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Oct 16, 2019

This PR #380 rebased on top of PR #435, with one additional change that ensures that "block volmode.*should store data" actually executes.

pohly and others added 6 commits October 16, 2019 11:14
The binaries on the host were already locked on Fedora via the
versions that yum was asked to install, but kubeadm (against the
expectation) then still ended picking up the latest control plane
images as long as they had the same <major>.<minor> versions.

For full reproducibility, we also want the control plane to be exactly
from the same release as the host binaries.
This is needed for skipping attach on Kubernetes 1.13 where that
feature was alpha. Found when enabling additional tests. The
provisioning test did not actually try to use the volume.
Code changes to support provisioning of raw block volumes. For block volumes
driver need not to create a file-system in NodePublishVolume stage.

With this change we only support fsdax raw block volumes. Should find a way to
support 'dax' raw volume, which is a character device.

Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
As now we support block volumes we no more return "unsupported" for block
volumes. Hence reverting commit 3fcd755.
Several (all?) of the volumeMode tests run with the default filesystem
(i.e. the empty string). If we don't list that as supported, raw block
tests are not executed:

S [SKIPPING] in Spec Setup (BeforeEach) [0.005 seconds]
PMEM Volumes
/nvme/gopath/src/github.com/intel/pmem-csi/test/e2e/storage/csi_volumes.go:57
  [Driver: pmem-csi]
  /nvme/gopath/src/github.com/intel/pmem-csi/test/e2e/storage/csi_volumes.go:102
    [Testpattern: Dynamic PV (block volmode)] volumes
    /nvme/gopath/src/github.com/intel/pmem-csi/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go:92
      should store data [BeforeEach]
      /nvme/gopath/src/github.com/intel/pmem-csi/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/volumes.go:146

      Driver pmem-csi doesn't support  -- skipping

      /nvme/gopath/src/github.com/intel/pmem-csi/vendor/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go:151
@pohly
Copy link
Contributor Author

pohly commented Oct 16, 2019

Because the source branch is in the main repo, we can track test results for all configurations here:
https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/job/pmem-csi/job/blockvolume/

@pohly
Copy link
Contributor Author

pohly commented Oct 16, 2019

@pohly pohly closed this Oct 16, 2019
@avalluri
Copy link
Contributor

No luck, the "should store data" test failed on 1.13: https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/job/pmem-csi/job/blockvolume/2/execution/node/129/log

CSI block volume support is broken in Kubernetes 1.13. CSI block volume does not consider the driver's attachRequired: flag and tries to get attachment object for the volume, this is where the failure was. Here is the related upstream issue.

If we want to support 1.13 block volumes, We better branch out the devel and revert b59e2f3. @pohly what is your opinion?

@pohly
Copy link
Contributor Author

pohly commented Oct 18, 2019

If we want to support 1.13 block volumes, We better branch out the devel and revert b59e2f3. @pohly what is your opinion?

We could also revert that just for 1.13. Skipping attach was an alpha feature in that release after all; depending on an alpha feature is always going to be risky.

But that makes supporting 1.13 even more complicated. I'm leaning towards declaring it as "unsupported/untested". With 1.16 out, it's also not supported upstream anymore (well, at least not officially - it still got the CVE-2019-11253 fix).

@pohly pohly deleted the blockvolume branch October 18, 2019 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants