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
Add tests for adding/removing disks/nics to/from a vm template #3070
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
import { | ||
createResource, | ||
deleteResource, | ||
deleteResources, | ||
} from '@console/shared/src/test-utils/utils'; | ||
import { get } from 'lodash'; | ||
import { testName } from '@console/internal-integration-tests/protractor.conf'; | ||
import { TEMPLATE_ACTIONS_TIMEOUT_SECS } from './utils/consts'; | ||
import { basicVMConfig, multusNAD, hddDisk, networkInterface, rootDisk } from './utils/mocks'; | ||
import { getProvisionConfigs } from './vm.wizard.configs'; | ||
import { VirtualMachine } from './models/virtualMachine'; | ||
import { VirtualMachineTemplate } from './models/virtualMachineTemplate'; | ||
import { ProvisionConfig } from './utils/types'; | ||
import { getResourceObject } from './utils/utils'; | ||
import { ProvisionConfigName } from './utils/constants/wizard'; | ||
|
||
describe('Test adding/removing discs/nics to/from a VM template', () => { | ||
const provisionConfigContainer = getProvisionConfigs().get(ProvisionConfigName.CONTAINER); | ||
const commonSettings = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
cloudInit: { | ||
useCloudInit: false, | ||
}, | ||
namespace: testName, | ||
description: `Default description ${testName}`, | ||
flavor: basicVMConfig.flavor, | ||
operatingSystem: basicVMConfig.operatingSystem, | ||
workloadProfile: basicVMConfig.workloadProfile, | ||
}; | ||
const vmTemplateConfig = (name: string, provisionConfig: ProvisionConfig) => { | ||
return { | ||
...commonSettings, | ||
name, | ||
provisionSource: provisionConfig.provision, | ||
storageResources: [], | ||
networkResources: [], | ||
}; | ||
}; | ||
const vmConfig = (name: string, templateConfig) => { | ||
return { | ||
...commonSettings, | ||
startOnCreation: true, | ||
name, | ||
template: templateConfig.name, | ||
provisionSource: templateConfig.provisionSource, | ||
storageResources: [], | ||
networkResources: [], | ||
}; | ||
}; | ||
|
||
const templateCfg = vmTemplateConfig( | ||
`tmpl-${provisionConfigContainer.provision.method.toLowerCase()}`, | ||
provisionConfigContainer, | ||
); | ||
const vmTemplate = new VirtualMachineTemplate(templateCfg); | ||
|
||
const vmCfg = vmConfig(`vmfromtmpl-${vmTemplate.name}`, templateCfg); | ||
const vm = new VirtualMachine(vmCfg); | ||
|
||
beforeAll(async () => { | ||
createResource(multusNAD); | ||
await vmTemplate.create(templateCfg); | ||
}, TEMPLATE_ACTIONS_TIMEOUT_SECS); | ||
|
||
afterAll(() => { | ||
deleteResources([multusNAD, vmTemplate.asResource()]); | ||
}); | ||
|
||
describe('Test adding discs/nics to a VM template', () => { | ||
vmCfg.startOnCreation = false; | ||
|
||
beforeAll(async () => { | ||
await vmTemplate.addDisk(hddDisk); | ||
await vmTemplate.addNIC(networkInterface); | ||
await vm.create(vmCfg); | ||
}, TEMPLATE_ACTIONS_TIMEOUT_SECS); | ||
|
||
afterAll(() => { | ||
deleteResource(vm.asResource()); | ||
}); | ||
|
||
it('Adds a disk to a VM template', async () => { | ||
expect(vm.getAttachedDisks()).toContain(hddDisk); | ||
}); | ||
|
||
it('Adds a NIC to a VM template', async () => { | ||
expect(vm.getAttachedNICs()).toContain(networkInterface); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}); | ||
|
||
xit('BZ(1779116) Clones disk defined in VM template', async () => { | ||
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}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 guess deletion should be possible - not entirely sure about that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and that the vm source type equals to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, the naming will change a bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now disabled, will be addressed in a different PR. |
||
}); | ||
}); | ||
|
||
describe('Test removing discs/nics from a VM template', () => { | ||
beforeAll(async () => { | ||
await vmTemplate.removeDisk(hddDisk.name); | ||
await vmTemplate.removeNIC(networkInterface.name); | ||
await vm.create(vmCfg); | ||
}, TEMPLATE_ACTIONS_TIMEOUT_SECS); | ||
|
||
afterAll(() => { | ||
deleteResource(vm.asResource()); | ||
}); | ||
|
||
it('Removes a disk from VM template', async () => { | ||
expect(vm.getAttachedDisks()).not.toContain(hddDisk); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same for the remove |
||
|
||
it('Removes a NIC from VM template', async () => { | ||
expect(vm.getAttachedNICs()).not.toContain(networkInterface); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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
IMO, we should introduce builder pattern here with sensible defaults.
e.g
thoughts?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.