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.

Sign up for GitHub

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

Jump to bottom

Add tests for adding/removing disks/nics to/from a vm template #3070

Merged
merged 1 commit into from Dec 5, 2019
Merged

Add tests for adding/removing disks/nics to/from a vm template #3070

merged 1 commit into from Dec 5, 2019

Conversation

rhrazdil
Copy link

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/kubevirt Related to kubevirt-plugin labels Oct 23, 2019
@rhrazdil rhrazdil force-pushed the kubevirt-template-resources branch 2 times, most recently from 0e2a681 to cf7f051 Compare October 25, 2019 11:57
@rhrazdil rhrazdil changed the title Add tests for adding/removing disks/nics to/from a vm template [WIP] Add tests for adding/removing disks/nics to/from a vm template Nov 5, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2019
@rhrazdil rhrazdil changed the title [WIP] Add tests for adding/removing disks/nics to/from a vm template Add tests for adding/removing disks/nics to/from a vm template Dec 3, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2019
@rhrazdil
Copy link
Author

rhrazdil commented Dec 3, 2019

@suomyi

@rhrazdil rhrazdil force-pushed the kubevirt-template-resources branch 3 times, most recently from 2aa1795 to ba893ac Compare December 3, 2019 15:31
atiratree
atiratree reviewed Dec 4, 2019
View reviewed changes
frontend/packages/kubevirt-plugin/integration-tests/tests/vm.detail.flavor.scenario.ts
@@ -27,10 +27,10 @@ describe('KubeVirt VM detail - edit flavor', () => {
it(
'changes tiny to large',
async () => {
const vm1Config = vmConfig(configName.toLowerCase(), provisionConfig, testName);
const vm1Config = vmConfig(configName.toLowerCase(), testName, provisionConfig);
Copy link
Member

@atiratree atiratree Dec 4, 2019 •
edited

Choose a reason for hiding this comment

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

This whole vmConfig creation is quite painful. the config is modified in 3 places

  1. call getProvisionConfigs
  2. call vmConfig
  3. customize result (you need to be familiar with the config format)
  4. pass it to vm multiple times

IMO, we should introduce builder pattern here with sensible defaults.
e.g

const vm: VirtualMachine = new VMBuilder(ProvisionConfigName.CONTAINER, testname)
    .addStorage(additionalStorage)
    .setNetworks([])
    .startOnCreation(true)
    .build()

vm.create()
console.log(vm.getConfig()) // to inspect

thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Yes yes yes I agree 100%, I also hate that we need to pass the config twice, first to create the VirtualMachine object and then to the create method.
I'm not sure if this is the right time to do it though because we need to close one epic automation gap epic first and this would be a huge change.

Copy link

Choose a reason for hiding this comment

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

indeed, I agree it would be nicer but it is a big change in a late moment in the cycle. Lets address as a followup once the automation gap is closed.

frontend/packages/kubevirt-plugin/integration-tests/tests/vmtemplate.resources.scenario.ts

describe('Test adding/removing discs/nics to/from a VM template', () => {
const provisionConfigContainer = getProvisionConfigs().get(ProvisionConfigName.CONTAINER);
const commonSettings = {
Copy link
Member

Choose a reason for hiding this comment

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

it could incorporate all of these functions and configs, which are quite hard to read IMO

frontend/packages/kubevirt-plugin/integration-tests/tests/vmtemplate.resources.scenario.ts
});

it('Adds a NIC to a VM template', async () => {
expect(vm.getAttachedNICs()).toContain(networkInterface);
Copy link
Member

Choose a reason for hiding this comment

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

the disadvantage of this approach is that Add NIC test will fail if adding disk fails in beforeAll

frontend/packages/kubevirt-plugin/integration-tests/tests/vmtemplate.resources.scenario.ts

it('Removes a disk from VM template', async () => {
expect(vm.getAttachedDisks()).not.toContain(hddDisk);
});
Copy link
Member

Choose a reason for hiding this comment

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

same for the remove

frontend/packages/kubevirt-plugin/integration-tests/tests/vmtemplate.resources.scenario.ts
const dataVolumeName = `${vm.name}-${vmTemplate.name}-${rootDisk.name}-clone`;
const dataVolume = getResourceObject(dataVolumeName, vm.namespace, 'datavolume');
const srcPvc = get(dataVolume, 'spec.source.pvc.name', '');
expect(srcPvc).toEqual(`${vmTemplate.name}-${rootDisk.name}`);
Copy link
Member

@atiratree atiratree Dec 4, 2019 •
edited

Choose a reason for hiding this comment

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

after #3665 merges, we should check that it is not possible to remove url disk from the template list after the creation.

edit:
I meant edit should not be possible (applies to some vm disks as well)

I guess deletion should be possible - not entirely sure about that.

Copy link
Member

Choose a reason for hiding this comment

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

and that the vm source type equals to Attached Clone Disk

Copy link
Member

Choose a reason for hiding this comment

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

also, the naming will change a bit.

Copy link
Member

Choose a reason for hiding this comment

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

we should also test that datavolume is not leaking after the template deletion

Copy link

Choose a reason for hiding this comment

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

now disabled, will be addressed in a different PR.

@jelkosz
Copy link

jelkosz commented Dec 5, 2019

The comments can be addressed as followups, acking this one.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2019
@jelkosz
Copy link

jelkosz commented Dec 5, 2019

/retest

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jelkosz, rhrazdil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 519ecf6 into openshift:master Dec 5, 2019
@spadgett spadgett added this to the v4.4 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jelkosz jelkosz jelkosz left review comments

@atiratree atiratree atiratree left review comments

@jhadvig jhadvig Awaiting requested review from jhadvig

@vojtechszocs vojtechszocs Awaiting requested review from vojtechszocs

Assignees

@jelkosz jelkosz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@rhrazdil @jelkosz @openshift-ci-robot @openshift-bot @atiratree @spadgett @openshift-merge-robot