Add CDI e2e tests #246

shiywang · 2018-05-28T09:27:50Z

Signed-off-by: Shiyang Wang shiywang@redhat.com

nellyc · 2018-05-30T16:54:42Z

tests/cdi/cdi_test.go

+	})
+})
+
+func WriteJson(name string, json string) (string, error) {


are suppose the following 3 funcs are here until PR kubevirt/kubevirt#980 will get merged.
after that there is no point in this code duplication. we will just need to import utils.go

yes, once that pr got merged, I will rebase my pr here.

nellyc · 2018-05-30T16:56:11Z

tests/cdi/cdi_test.go

+
+// template parameters
+const (
+	pvcEPHTTPNOAUTHURL = "https://raw.githubusercontent.com/shiywang/kubevirt-ansible/add_vm/tests/images/testvm.qcow2"


i guess we need to setup a place with all images in it.

yeah, I have a pr to push a small test image (~13mb) to github repo here: #245, but if you think that's not a good idea, maybe we should put images in some registry or cdn or some http service ? I just don't know where could we have the place to put those images right now : -/

nellyc · 2018-05-30T17:02:09Z

roles/cdi/defaults/main.yml

-repo_tag: "{{ cdi_repo_tag | default('jcoperh') }}"
-release_tag: "{{ cdi_release_tag | default('latest') }}"
+repo_tag: "{{ cdi_repo_tag | default('kubevirt') }}"
+release_tag: "{{ cdi_release_tag | default('v0.5.0-alpha.0') }}"


IF CDI repo is really going to follow the same versions as kubevirt, i would keep only 1 param
also not sure about this default... i would prefer to not have a default and fail, than to run on a version somebody put here god knows how long ago, or would use latest and if it fails at least you know the latest version is not working

yeah, I guess we can eliminate cdi_repo_tag by hard coding it into template, but I guess this is also fine ? I would like to keep it cuz sometime it maybe more easy for debugging, but for this case since there's only one yaml file here, so I guess I don't have strong opinion on keep one parameter or two.

also not sure about this default... i would prefer to not have a default and fail, than to run on a version somebody put here god knows how long ago, or would use latest and if it fails at least you know the latest version is not working

Yeah, this is what I suggested using latest and I filed an issue here: kubevirt/containerized-data-importer#177, but seems they didn't use the latest tag on purpose.

gbenhaim

I don't see the
cdi controller and data importer pod logs in
https://jenkins.ovirt.org/job/kubevirt_kubevirt-ansible_standard-check-pr/880/artifact/exported-artifacts/check-patch.openshift_3-9.el7.x86_64/vms_logs/

Do those pods write any logs?

gbenhaim · 2018-06-24T14:22:18Z

tests/cdi/cdi_test.go

+var _ = Describe("CDI create importer-pod write images into pv", func() {
+	flag.Parse()
+
+	overrideTemplateFromParameters := func(srcFilePath, DstFilePath string, params ...string) string {


s/DstFilePath/dstFilePath

gbenhaim · 2018-06-24T14:44:22Z

tests/cdi/cdi_test.go

+		By(fmt.Sprintf("Creating %s:%s from JSON file via oc-create command", resourceType, resourceName))
+		out, err := RunOcCommand("create", "-f", filePath)
+		Expect(err).ToNot(HaveOccurred())
+		message = fmt.Sprintf(resourceType+" \"%s\" created\n", resourceName)


Why do you need to check the message? return code isn't enough?

gbenhaim · 2018-06-24T15:12:18Z

tests/cdi/cdi_test.go

+			}
+		})
+
+		It("create pvc and vm should success", func() {


I think that this assertion should be split into three parts (so in case of a failure we will know where to look):

Verify that CDI populated the PVC

Verify that the PVC is attached to the VM.

Verify that the VM is functional (maybe using SSH?) - this way we ensure that the image didn't get corrupted during the import process.

Okay, I already split it into four steps.
For the last one, we have some lib in kubevirt here I guess we could reuse, I will add it in a follow-up pr @gbenhaim wdyt ?

Verify that the VM is functional (maybe using SSH?)

shiywang · 2018-06-25T08:01:02Z

I don't see the
cdi controller and data importer pod logs in
https://jenkins.ovirt.org/job/kubevirt_kubevirt-ansible_standard-check-pr/880/artifact/exported-artifacts/check-patch.openshift_3-9.el7.x86_64/vms_logs/

@gbenhaim not sure, I saw the logs here: http://jenkins.ovirt.org/job/kubevirt_kubevirt-ansible_standard-check-pr/882/consoleFull, the tests case runs well and on my local env I can check pod logs using oc logs but don't know how lago collect those logs ? is there any chances lago didn't collect logs in the right place ?

@gbenhaim
most of the comments are addressed, PTAL

gbenhaim · 2018-06-25T08:32:21Z

Lago collects /var/log from all the master and nodes.
For example, here are the logs of the containers that run on the master:
https://jenkins.ovirt.org/job/kubevirt_kubevirt-ansible_standard-check-pr/880/artifact/exported-artifacts/check-patch.openshift_3-9.el7.x86_64/vms_logs/lago-master/_var_log/containers/

shiywang · 2018-06-25T10:54:37Z

@gbenhaim this is a tricky problem, here is my logs on my nodes, but I don't know why lago doesn't collect those, when I use oc logs to debug cdi controller and data importer pod lastweek, the logs are much fewer than I debugged on my own node, I created an issue to track here also @copejon @jeffvance do you have any ideas on the cdi-controller logs problem ?

[root@cnv-executor-shiywang-node2 ~]# cd /var/log/containers/
[root@cnv-executor-shiywang-node2 containers]# ls
apiserver-7vkfj_kube-service-catalog_apiserver-10e129e2a435d70ce28ddfaf5e6cfa142ef557ad66c44a6a8ad9d432ca15091b.log
asb-1-jwb24_openshift-ansible-service-broker_asb-7177a94ca46938063be69dde617bc0694e663723814aee631e35db45e126a777.log
cdi-deployment-56976686f-g8ccq_golden-images_cdi-controller-87b07d95e9018a05d694a6b6ed3c005d6e781969d4621e7334d3cf55f410a329.log
controller-manager-r8kdr_kube-service-catalog_controller-manager-234cca339034c5021add4197033af573c1e0e6f9aa39c158be3bc795ed84e067.log
glusterfs-storage-7k4kv_app-storage_glusterfs-eefc8c11f34bae7840692a744ffe91632a40e06520375d24d8cfe37caf3ed34a.log
heapster-s5f95_openshift-infra_heapster-33345ce13b6f7f18a82a88324c7fa65dffd52427399db74fce1741f56c01045e.log
heapster-s5f95_openshift-infra_heapster-84c3b71dfbf5aa58cd9b14eff0840165177ad9854b5ec04773126f116d1a996b.log
heketi-storage-1-7sm64_app-storage_heketi-7046edc47ec7e560ff4657b32d270d5d0c2644c95929026e837cf4fcbfcc2d57.log
ovs-8krqr_openshift-sdn_openvswitch-c849cc836940a814716ce8ae4f4650248f1b8be2da00a830fdaf1c217fbb500a.log
router-1-5thxz_default_router-bb6635de24b50102f0b98ea84bd6e7496e9c5c1b46857b43305df87d4b38056b.log
sdn-mdhlh_openshift-sdn_sdn-18b5bc58c13b052a0129aabf5671f7522c5bd8902710517c158a8790c4291170.log
sync-c8gjv_openshift-node_sync-93d9eca6690c99072af68861399ca43c6be3fd14936b5d152a910f85e19cd043.log
virt-api-7797f95869-8jr7x_kube-system_virt-api-c24ab742a5a31f10679f77fdb84f701e968362c5865fba03086e6800687a995e.log
virt-controller-69cc6b4897-mfsfk_kube-system_virt-controller-13e2a824efa9834cd55313bb25923f460c48a59eb44c275154a835bfdeda9074.log
virt-handler-94fhj_kube-system_virt-handler-5dfb91f216bccf77ca38739140e08731f214f9579792ae064b81b5a2830b6f3e.log
webconsole-785689b664-2gkdg_openshift-web-console_webconsole-55a87e6d7ea43413ecdc5448a768ae31e6936d3bbf262149cfe2fc86e27ba732.log
webconsole-785689b664-2gkdg_openshift-web-console_webconsole-b149afca9f89555fceedaef26e26dd54077a25242842135ba5a77cc87a6eac61.log
[root@cnv-executor-shiywang-node2 containers]# tailf cdi-deployment-56976686f-g8ccq_golden-images_cdi-controller-87b07d95e9018a05d694a6b6ed3c005d6e781969d4621e7334d3cf55f410a329.log
{"log":"E0625 10:45:36.001006       1 reflector.go:205] github.com/kubevirt/containerized-data-importer/pkg/client/informers/externalversions/factory.go:74: Failed to list *v1alpha1.DataVolume: datavolumes.cdi.io is forbidden: User \"system:serviceaccount:golden-images:cdi-sa\" cannot list datavolumes.cdi.io at the cluster scope: User \"system:serviceaccount:golden-images:cdi-sa\" cannot list all datavolumes.cdi.io in the cluster\n","stream":"stderr","time":"2018-06-25T10:45:36.001166173Z"}
{"log":"E0625 10:45:37.002552       1 reflector.go:205] github.com/kubevirt/containerized-data-importer/pkg/client/informers/externalversions/factory.go:74: Failed to list *v1alpha1.DataVolume: datavolumes.cdi.io is forbidden: User \"system:serviceaccount:golden-images:cdi-sa\" cannot list datavolumes.cdi.io at the cluster scope: User \"system:serviceaccount:golden-images:cdi-sa\" cannot list all datavolumes.cdi.io in the cluster\n","stream":"stderr","time":"2018-06-25T10:45:37.002697948Z"}
...
...

shiywang · 2018-06-25T13:54:01Z

@aglitke other than those two issues here: #300 #297, could you please take a look at this pr ?

nellyc · 2018-06-25T12:50:36Z

tests/cdi/test-vm.yml

+  creationTimestamp: null
+  name: cdi-test
+objects:
+- apiVersion: kubevirt.io/v1alpha1


i think this should get updated to v1alpha2,
i wonder how can we keep it up-to-date with moving versions
maybe it should be a param as well, but need to figure out where to bring it from

alexxa

@shiywang Please see the comments.

alexxa · 2018-07-19T08:51:17Z

README.md

 To deploy KubeVirt on an existing OpenShift cluster run the command below. For more information on clusters and other deployment scenarious see [playbooks instructions](./playbooks/README.md).

 ```
 ansible-playbook -i localhost playbooks/kubevirt.yml -e@vars/all.yml
 ```
 >**Note:** Check default variables in [vars/all.yml](./vars/all.yml) and update them if needed.

+### E2E Testing


@shiywang IMHO, It's more user-friendly to give instructions/steps in a form of a numerated list. First, the description of what should be done is given, and then how to do it. The usage instructions/steps go well with the imperative mood.

For example:

Login into the cluster (or Ensure it is possible to login into the cluster)
oc login

Compile tests inside the docker container and copy it to ....
make generate-tests

Run tests ...
make test

...

alexxa · 2018-07-19T08:56:25Z

README.md

 To deploy KubeVirt on an existing OpenShift cluster run the command below. For more information on clusters and other deployment scenarious see [playbooks instructions](./playbooks/README.md).

 ```
 ansible-playbook -i localhost playbooks/kubevirt.yml -e@vars/all.yml
 ```
 >**Note:** Check default variables in [vars/all.yml](./vars/all.yml) and update them if needed.

+### E2E Testing
+
+After a successful deployment or using an exist cluster, make sure you can `oc login` into cluster.


Don't use command as a verb in documentation.

s/using an exist cluster/using the existing cluster

s/into cluster/into the cluster

alexxa · 2018-07-19T09:03:11Z

README.md

+```
+make generate-tests
+```
+Will compile tests from tests inside docker container and copy it to _out directory inside kubevirt-ansible repository.


Tests from Tests? Tests are already inside and some will be compiled in the same container? It's a very confusing phrase. Maybe: Compile tests which are inside THE docker container?

s/to _out directory/to the _out directory

s/kubevirt-ansible repository/the kubevirt-ansible repository

alexxa · 2018-07-19T09:04:29Z

README.md

+```
+make test
+```
+Will run all the e2e tests with kubeconfig from `~/.kube/config`, or you can pass it to tests via:


Run all the e2e tests with/using the ~/.kube/config file, or ...

alexxa · 2018-07-19T09:08:38Z

README.md

+./_out/tests/<name>.test -kubeconfig=your_kubeconfig -tag=kubevirt_images_tag -prefix=kubevirt -test.timeout 60m
+```
+
+If you want to test PVC's `storage.import.endpoint` with different/internal images, we provided an ENV called `STREAM_IMAGE_URL` which you can set up in your own environment like:


avoid WE in docs:

To test PVC's storage.import.endpoint with other images, use the STREAM_IMAGE_URL environment variable:

alexxa · 2018-07-19T09:11:35Z

README.md

+
+If you want to test PVC's `storage.import.endpoint` with different/internal images, we provided an ENV called `STREAM_IMAGE_URL` which you can set up in your own environment like:
+```
+export STREAM_IMAGE_URL=https://test.net/0.1.0/internal-test-vm.img


Is it some sample URL? If so, i would change it for =<the_image_url>

gbenhaim · 2018-07-19T10:34:48Z

{"log":"I0719 09:53:51.773757       1 event.go:218] Event(v1.ObjectReference{Kind:\"PersistentVolumeClaim\", Namespace:\"kubevirt-test-default\", Name:\"golden-pvc\", UID:\"a41f1f64-8b39-11e8-810f-5452c0a8c802\", APIVersion:\"v1\", ResourceVersion:\"7916\", FieldPath:\"\"}): type: 'Normal' reason: 'FailedBinding' no persistent volumes available for this claim and no storage class is set\n","stream":"stderr","time":"2018-07-19T09:53:51.773899245Z"}

Full master log: https://jenkins.ovirt.org/job/kubevirt_kubevirt-ansible_standard-check-pr/1015//artifact/exported-artifacts/check-patch.openshift_3-10.el7.x86_64/vm_logs/on_exit/lago-master/_var_log/containers/master-controllers-lago-master_kube-system_controllers-6e517eeeaa361316ebb6a015d704cf2684cc5ace29067e690c48dec57bfb1153.log/*view*/

Is it related to #305 ?

alexxa · 2018-07-19T11:20:04Z

tests/cdi/cdi_suite_test.go

+	// Wait until the namespaces are terminated
+	fmt.Println("")
+	for _, namespace := range testNamespaces {
+		fmt.Printf("Waiting for namespace %s to be removed, this can take a while ...\n", namespace)


Simply: "Removing the %s namespace. It can take some time."

alexxa · 2018-07-19T11:28:45Z

tests/cdi/cdi_test.go

+		url = pvcEPHTTPNOAUTHURL
+	}
+
+	Context("with valid image url will succeed", func() {


what will succeed?

alexxa · 2018-07-19T11:41:19Z

tests/util.go

+)
+
+func ProcessTemplateWithParameters(srcFilePath, dstFilePath string, params ...string) string {
+	By(fmt.Sprintf("Overriding template from %s to %s", srcFilePath, dstFilePath))


the template

alexxa · 2018-07-19T11:44:07Z

tests/util.go

+func writeJson(jsonFile string, json string) (string, error) {
+	err := ioutil.WriteFile(jsonFile, []byte(json), 0644)
+	if err != nil {
+		return "", fmt.Errorf("failed to write json file %s", jsonFile)


the json file

alexxa · 2018-07-19T11:50:37Z

tests/util.go

+}
+
+func createResourceWithFilePath(resourceType, resourceName, filePath, nameSpace string) {
+	By(fmt.Sprintf("Creating %s:%s from JSON file via oc-create command", resourceType, resourceName))


the json file

via/with

the oc-create command

shiywang · 2018-07-19T12:53:24Z

tests/cdi/cdi_suite_test.go

+}
+
+var _ = BeforeSuite(func() {
+	virtCli, err := kubecli.GetKubevirtClient()


will change to use CreateNamespace once kubevirt/kubevirt#1343 merged

shiywang · 2018-07-19T15:38:07Z

@gbenhaim @aglitke yeah, it is really weird, my debug pr got passed which is exactly same as this one, oh, the only difference is here: 7be3d68#diff-1c9e6b58cf30323b99cfe0468b617262R6, so I think it may releated to #305 I guess ?

shiywang · 2018-07-19T15:39:41Z

This one passed #325
@aglitke @jeffvance PTAL

jeffvance · 2018-07-19T17:55:34Z

tests/cdi/cdi_test.go

+		})
+
+		Specify("the PVC should become bound", func() {
+			tests.WaitUntilResourceReadyByNameTestNamespace("pvc", pvcName, "-o=jsonpath='{.metadata.annotations}'", "pv.kubernetes.io/bind-completed:yes pv.kubernetes.io/bound-by-controller:yes")


Since annotations are maps I don't think you can rely on the order of 2 annotations. It is more reliable and accurate to test each annotation value separately, imo.

I guess I can only keep one, pv.kubernetes.io/bind-completed:yes is already enough.

jeffvance · 2018-07-19T18:18:34Z

tests/cdi/cdi_test.go

+		})
+
+		Specify("the importer-pod should become completed", func() {
+			tests.WaitUntilResourceReadyByLabelTestNamespace("pod", "app=containerized-data-importer", "-o=jsonpath='{.items[0].status.phase}'", "Succeeded")


how do you know the 1st pod listed (matching the label) is the pod you're interested in? I think this test is too imprecise.

because I only created one and I didn't find any other labels to match more imprecisely. do you have better suggestions? if not maybe we should create an issue for CDI repo, to add more precise labels to importer-pod?

I was thinking that maybe namespace (which is random-ized) would be a good additional filter. But if you have full control of the test env such that you know no other cdi tests will run in parallel with this test then what you have here is fine. My comment is not about the value of the label but that there can potentially be >1 importer pod if more than one cdi test is run at the same time.

thank you for the suggestion, I will create an issue for that about the randomized namespace and more than one pod conflicts, will do that in a follow-up pr

Signed-off-by: Shiyang Wang <shiywang@redhat.com>

shiywang · 2018-07-20T02:08:47Z

@aglitke @jeffvance updated, PTAL

shiywang · 2018-07-20T15:18:25Z

@lukas-bednar @nellyc could you help me merge this pr ? thank you so much !!

shiywang force-pushed the cdi-test branch 2 times, most recently from 35b4f22 to d6db8f2 Compare May 29, 2018 12:40

shiywang changed the title ~~WIP: add CDI e2e tests~~ Add CDI e2e tests May 29, 2018

shiywang mentioned this pull request May 29, 2018

dockerhub image tag latest need to be latest image kubevirt/containerized-data-importer#177

Closed

nellyc reviewed May 30, 2018

View reviewed changes

shiywang force-pushed the cdi-test branch from 1e16469 to f0edd6b Compare May 31, 2018 08:06

shiywang changed the title ~~Add CDI e2e tests~~ WIP Add CDI e2e tests Jun 1, 2018

shiywang force-pushed the cdi-test branch 2 times, most recently from 133f956 to 07df419 Compare June 8, 2018 11:01

shiywang force-pushed the cdi-test branch 4 times, most recently from 97daeec to 1c4e4b6 Compare June 12, 2018 11:24

This was referenced Jun 14, 2018

CDI: Failed to list *v1alpha1.DataVolume: datavolumes.cdi.io is forbidden: #284

Closed

CDI version issues kubevirt/containerized-data-importer#203

Closed

shiywang force-pushed the cdi-test branch from c0e97ed to f937784 Compare June 15, 2018 08:22

shiywang changed the title ~~WIP Add CDI e2e tests~~ Add CDI e2e tests Jun 15, 2018

shiywang force-pushed the cdi-test branch 4 times, most recently from 4d343cc to cb2b537 Compare June 22, 2018 10:38

gbenhaim suggested changes Jun 24, 2018

View reviewed changes

shiywang force-pushed the cdi-test branch from 4e4ce80 to 3e1bd7c Compare June 25, 2018 07:10

shiywang mentioned this pull request Jun 25, 2018

Lago collects logs doesn't contain cdi-controller and importer-pod #297

Closed

gbenhaim approved these changes Jun 25, 2018

View reviewed changes

nellyc reviewed Jun 25, 2018

View reviewed changes

shiywang force-pushed the cdi-test branch from 79d9401 to 54e20b6 Compare July 19, 2018 08:26

alexxa reviewed Jul 19, 2018

View reviewed changes

shiywang force-pushed the cdi-test branch 2 times, most recently from 7ebe59d to e7a94bc Compare July 19, 2018 12:47

shiywang commented Jul 19, 2018

View reviewed changes

shiywang force-pushed the cdi-test branch 3 times, most recently from baf2a36 to 19ccecf Compare July 19, 2018 13:50

jeffvance reviewed Jul 19, 2018

View reviewed changes

shiywang force-pushed the cdi-test branch from 19ccecf to 397d3eb Compare July 20, 2018 02:03

Shiyang Wang added 5 commits July 20, 2018 10:07

add cdi e2e test code, manifest util lib

326a9c6

Signed-off-by: Shiyang Wang <shiywang@redhat.com>

change hack bash scripts

e1ccfd0

Signed-off-by: Shiyang Wang <shiywang@redhat.com>

change playbooks files

1a0894f

Signed-off-by: Shiyang Wang <shiywang@redhat.com>

add e2e tests usage documentation

0bfdca2

Signed-off-by: Shiyang Wang <shiywang@redhat.com>

add back openshift_storage_glusterfs_storageclass_default=true

eac26dd

shiywang force-pushed the cdi-test branch from 397d3eb to eac26dd Compare July 20, 2018 02:07

aglitke approved these changes Jul 20, 2018

View reviewed changes

gbenhaim merged commit fe2a3ef into kubevirt:master Jul 22, 2018

shiywang deleted the cdi-test branch July 22, 2018 14:41

Add CDI e2e tests #246

Add CDI e2e tests #246

Conversation

shiywang commented May 28, 2018 • 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

gbenhaim left a 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

shiywang Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

shiywang commented Jun 25, 2018

gbenhaim commented Jun 25, 2018

shiywang commented Jun 25, 2018 • edited Loading

shiywang commented Jun 25, 2018

Choose a reason for hiding this comment

alexxa left a 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

alexxa Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

gbenhaim commented Jul 19, 2018 • 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

shiywang commented Jul 19, 2018

shiywang commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang commented Jul 20, 2018

shiywang commented Jul 20, 2018 • edited Loading

shiywang commented May 28, 2018 •

edited

Loading

shiywang Jun 25, 2018 •

edited

Loading

shiywang commented Jun 25, 2018 •

edited

Loading

alexxa Jul 19, 2018 •

edited

Loading

gbenhaim commented Jul 19, 2018 •

edited

Loading

shiywang Jul 20, 2018 •

edited

Loading

shiywang commented Jul 20, 2018 •

edited

Loading