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

Filesystem PVC as VM disk #734

Merged
merged 15 commits into from Feb 20, 2018

Conversation

Projects
None yet
4 participants
@davidvossel
Member

davidvossel commented Feb 13, 2018

This is built off the work in #693

  • Adds filesystem PVC disk support
  • Replaces direct ISCSI volume support with new generic PVC volumes
  • Revises storage functional tests
@cynepco3hahue

Looks great to me 😄
I think it fails on KubeVirt CI because the environment does not have iscsi-initiator-utils because it installed during vagrant provision stage

@cynepco3hahue

This comment has been minimized.

Member

cynepco3hahue commented Feb 14, 2018

@davidvossel Can you also adapt tests under tests/vm_configuration_test.go from the usage of direct LUN to PVC?

@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 14, 2018

Can you also adapt tests under tests/vm_configuration_test.go from the usage of direct LUN to PVC?

@cynepco3hahue Is there any reason to convert those LUN functional tests that exercise the bus? There's a registry disk test that verifies every driver works.

@cynepco3hahue

This comment has been minimized.

Member

cynepco3hahue commented Feb 14, 2018

@davidvossel I think it no reason 😄 I checked converter code and we assign bus only under Convert_v1_Disk_To_api_Disk so it does not really matter what volume source do we use.
I think we can just remove them(I already prepared the PR so you do not need to do it under your PR) if @fromanirh does not have any objections.

@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 14, 2018

@cynepco3hahue Those tests were already removed from this PR. I had to do that to get the tests to compile since i stripped out LUN support.

@@ -360,16 +327,27 @@ func newPVC(os string, withAuth bool) *k8sv1.PersistentVolumeClaim {
}
}
func newPV(os string, lun int32, withAuth bool) *k8sv1.PersistentVolume {
func createPvISCSI(os string, lun int32, withAuth bool) {

This comment has been minimized.

@cynepco3hahue

cynepco3hahue Feb 14, 2018

Member

If we drop ISCSI volume source support we can remove all functions related to it

This comment has been minimized.

@davidvossel

davidvossel Feb 14, 2018

Member

This function is creating the PV for the iscsi demo target. All the direct LUN related functions have been removed now.

@cynepco3hahue

This comment has been minimized.

Member

cynepco3hahue commented Feb 14, 2018

@davidvossel I see you already removed them

local DISK_NAME="/tmp/$(basename $FN)"
dd if=/dev/zero of=$DISK_NAME bs=1M count=1024
mkfs.ext4 $DISK_NAME
sleep 10

This comment has been minimized.

@davidvossel

davidvossel Feb 15, 2018

Member

@cynepco3hahue do you know why this sleep is here?

This comment has been minimized.

@cynepco3hahue

cynepco3hahue Feb 15, 2018

Member

@davidvossel I saw that sometimes mount failed without sleep

@fabiand

This comment has been minimized.

Member

fabiand commented Feb 15, 2018

Hm, standard ci passed:

Ran 36 of 36 Specs in 1461.028 seconds

SUCCESS! -- 36 Passed | 0 Failed | 0 Pending | 0 Skipped PASS

It just failed to tear down a node.

@fabiand

This comment has been minimized.

Member

fabiand commented Feb 15, 2018

ci test please

@rmohr

Can't we just look up in the tests where the iscsi pod is deployed and fetch the right IP in the tests? I would really prefer that, instead of switching to the daemonset.

@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 19, 2018

Can't we just look up in the tests where the iscsi pod is deployed and fetch the right IP in the tests? I would really prefer that, instead of switching to the daemonset.

@rmohr the problem is manual testing and development. we can't make a VM manifest that just works (like cluster/vm-pvc.yaml) if someone has to go in and manually enter a POD ip into the manifest after every deploy. During development that's tedious and is error prone. I'm favoring ease of use over efficiency here for the sake of the demo.

@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 19, 2018

retest this please

2 similar comments
@rmohr

This comment has been minimized.

Member

rmohr commented Feb 19, 2018

retest this please

@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 19, 2018

retest this please

cynepco3hahue and others added some commits Feb 1, 2018

Add support to use file PV's as VM disks
Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
Introduce NFS server
Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
Fix unit tests
Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
Adapt functional tests
- get pod IP of ISCSI targer and use it under PV definition
- add NFS PV's and PVC's

Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
Prepare code for functional tests
Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
Re-enable storage tests
Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
add functional test to verify multiple restart of vm with pvc disk
Signed-off-by: David Vossel <davidvossel@gmail.com>
Remove direct ISCSI volume support in favor of PVCs
Signed-off-by: David Vossel <davidvossel@gmail.com>
ensure iscsi demo binds to local port 3260 successfully before markin…
…g ready

Signed-off-by: David Vossel <davidvossel@gmail.com>
make iscsi demo a daemon set
Signed-off-by: David Vossel <davidvossel@gmail.com>

davidvossel added some commits Feb 13, 2018

ensure previous iscsi target gets cleaned up if deployment was in use
Signed-off-by: David Vossel <davidvossel@gmail.com>
verify readiness of iscsi demo server before starting storage tests
Signed-off-by: David Vossel <davidvossel@gmail.com>
remove direct lun disk config functional tests
Signed-off-by: David Vossel <davidvossel@gmail.com>
increase storage timeouts
Signed-off-by: David Vossel <davidvossel@gmail.com>
@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 19, 2018

retest this please

@davidvossel

This comment has been minimized.

Member

davidvossel commented Feb 19, 2018

@rmohr I rebased and the registry-disk failures went away. jenkins passes now, hooray!

@rmohr

This comment has been minimized.

Member

rmohr commented Feb 20, 2018

Can't we just look up in the tests where the iscsi pod is deployed and fetch the right IP in the tests? I would really prefer that, instead of switching to the daemonset.

Since it is already done, my biggest concern (more work) is gone. Fine with proceeding with the localhost solution.

@rmohr

rmohr approved these changes Feb 20, 2018

@@ -34,7 +34,7 @@ yum -y remove NetworkManager firewalld
yum -y install https://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm
yum -y install jq sshpass
yum -y install bind-utils net-tools
yum -y install bind-utils net-tools iscsi-initiator-utils

This comment has been minimized.

@rmohr

rmohr Feb 20, 2018

Member

We are now trying to install them two times. Not a blocker since it save to do so.

@rmohr rmohr merged commit 372441e into kubevirt:master Feb 20, 2018

4 of 6 checks passed

check-patch.el7.x86_64 Test script failed
Details
standard-ci Some tests failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.09%) to 66.756%
Details
kubevirt-functional-tests/jenkins/pr/vagrant-dev All is well
Details
kubevirt-functional-tests/jenkins/pr/vagrant-release All is well
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment