Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

Auto tests for PR: kubevirt/kubevirt#1839 #537

Merged
merged 27 commits into from
May 17, 2019

Conversation

sHaggYcaT
Copy link
Contributor

@sHaggYcaT sHaggYcaT commented Jan 7, 2019

What this PR does / why we need it:

Auto tests for PR: kubevirt/kubevirt#1839

Release note:

NONE

@sHaggYcaT sHaggYcaT added the WIP label Jan 7, 2019
@kubevirt-bot kubevirt-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L labels Jan 7, 2019
@MarSik
Copy link

MarSik commented Jan 7, 2019

@sHaggYcaT I do not think you have to check all the VM creation, just use those as functions. I am pretty sure there was a way to call a method and create VM from yaml without having to implement everything on you own.

See for example https://github.com/kubevirt/kubevirt-ansible/blob/master/tests/high_performance_vm_test.go#L46

Currently it is very hard to see the test code in all the boilerplate around it.

@sHaggYcaT sHaggYcaT changed the title initial Auto tests for PR: kubevirt/kubevirt#1839 Jan 7, 2019
@kubevirt-bot kubevirt-bot added size/XL needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L labels Jan 28, 2019
@sHaggYcaT
Copy link
Contributor Author

@MarSik ^^updated version ^^
Do not ready for merge, but almost done

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2019
Copy link

@MarSik MarSik left a comment

Choose a reason for hiding this comment

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

In general it looks reasonable, make sure you properly cleanup the started VMs and check the patterns for your Context and It descriptions, I do not think the text there should repeat

return arguments
}

var _ = Describe("numaSupport", func() {
Copy link

Choose a reason for hiding this comment

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

NUMA? This has nothing to do with numa.. guest topology might be better name here. Also shouldn't you annotate this with the test plan ID somehow so the results are automatically connected with it?

Copy link
Contributor Author

@sHaggYcaT sHaggYcaT Feb 15, 2019

Choose a reason for hiding this comment

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

NUMA

Just old variable, I'll rename it. I've already rename everything about NUMA, but in this place forget to do it

Also shouldn't you annotate this with the test plan ID somehow so the results are automatically connected with it?

I'm going to do this work it in another PR for all my test plans together. Now I just want to merge this code ASAP (before it outdated, and before it requires rebase/difficult merges, etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already fixed

"time"
)

type Config struct {
Copy link

Choose a reason for hiding this comment

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

Why not reuse the kubevirt/kubernetes API objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please show an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^ @MarSik ^^

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@sHaggYcaT sHaggYcaT Feb 27, 2019

Choose a reason for hiding this comment

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

I've just made it

@sHaggYcaT
Copy link
Contributor Author

sHaggYcaT commented Feb 15, 2019

make sure you properly cleanup the started VMs

I've tested it in the openshift local installation and in the official CI. If test passed, then everything delete. If not passed, then failed VMI not delete (and it's helpful for debug). It's not problem for github CI, because it creates dedicated environment for each patch. And it's helpful if you run test manually using our internal CI

@sHaggYcaT
Copy link
Contributor Author

@MarSik how to remove "WIP" flag?

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2019
@lukas-bednar lukas-bednar removed the WIP label Mar 4, 2019
@lukas-bednar
Copy link
Contributor

@sHaggYcaT could you please try to add following value to LagoInitFile.yml.j2

diff --git a/playbooks/provider/lago/LagoInitFile.yml.j2 b/playbooks/provider/lago/LagoInitFile.yml.j2
index d6fb07ac..8cbcd7a4 100644
--- a/playbooks/provider/lago/LagoInitFile.yml.j2
+++ b/playbooks/provider/lago/LagoInitFile.yml.j2
@@ -6,6 +6,7 @@ nat-settings: &nat-settings
     management: False
 
 vm-common-settings: &vm-common-settings
+    vcpu: 4
     root-password: 123456
     service_provider: systemd
     artifacts:

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2019
@sHaggYcaT
Copy link
Contributor Author

@sHaggYcaT could you please try to add following value to LagoInitFile.yml.j2

Thank you @lukas-bednar

@@ -6,6 +6,7 @@ nat-settings: &nat-settings
management: False

vm-common-settings: &vm-common-settings
vCpu: 4
Copy link
Contributor

@lukas-bednar lukas-bednar Mar 4, 2019

Choose a reason for hiding this comment

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

it is lowercased vcpu, I am afraid that it might cause that this option is going to be ignored :-O

For further reference, you can following this document: https://lago.readthedocs.io/en/latest/LagoInitFile.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukas-bednar looks like it doesn't work :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forget to say, your latest suggestion works

return arguments
}

var _ = Describe("numaSupport", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix

@@ -134,3 +134,55 @@ func RemoveDataVolume(dvName string, namespace string) {
err = virtCli.CdiClient().CdiV1alpha1().DataVolumes(namespace).Delete(dvName, nil)
Expect(err).ToNot(HaveOccurred())
}

func CreateVMorVMIfromManifest(manifest, nameSpace string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

See tests/framework/types.go
Use: func (vm VirtualMachine) Create() (string, string, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// Find out how many VMs with specific resource needs can be run in the cluster

func min(a, b int) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

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, thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

virtClient, err := kubecli.GetKubevirtClient()
ktests.PanicOnError(err)
address_common := "tests/manifests/sockets_cores_threads/"
const TestSystemNamespace = "kubevirt-test-default"
Copy link
Contributor

Choose a reason for hiding this comment

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

import
tests "kubevirt.io/kubevirt-ansible/tests/framework"
use like:
tests.NamespaceTestDefault
remove this const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*
*/

package tests_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Add polarion test ids to all test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ILpinto done

@sHaggYcaT
Copy link
Contributor Author

Failed in webUI deploy:

17:03:26 TASK [kubevirt_web_ui : Wait until Web UI is ready: 'v1.4.0-13'] ***************

Copy link
Member

@vladikr vladikr left a comment

Choose a reason for hiding this comment

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

Looks fine to me now. Some parts of this code could be implemented differently, but I don't see a point on stalling on this now. It probably can be improved in the future or re-implemented on another platform.

Let's wait for the tests to finish.

@sHaggYcaT
Copy link
Contributor Author

@vladikr thank you.
@MarSik permitted few work-arounds to fix CI. So, I not sure, maybe my fixes broken something. In this case let me know please.
my tests finished without errors: check [rfe_id:1443]. So, I'll merge this PR

@sHaggYcaT sHaggYcaT merged commit 4d6422a into kubevirt:master May 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants