Michael Pasternak has posted comments on this change.
Change subject: restapi: Make adding (virtio-)console to virtual machines
optional
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(17 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1940: <xs:element name="high_availability"
type="HighAvailability" minOccurs="0"/>
Line 1941: <xs:element name="display" type="Display" minOccurs="0"
maxOccurs="1"/>
Line 1942: <xs:element name="stateless" type="xs:boolean"
minOccurs="0"/>
Line 1943: <xs:element name="delete_protected" type="xs:boolean"
minOccurs="0"/>
Line 1944: <xs:element name="console_device_enabled"
type="xs:boolean" minOccurs="0"/>
i'd use this modelling:
<console enabled="true|false"/>
e.g
1. add new type "console"
2. add attribute "console.enabled"
3. refer it from vm/template
this way we will have ability to extend <console/> fields in a future.
Line 1945: <xs:element name="timezone" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 1946: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/>
Line 1947: <xs:element ref="usb" minOccurs="0" maxOccurs="1"/>
Line 1948: <xs:element name="tunnel_migration" type="xs:boolean"
minOccurs="0" maxOccurs="1"/>
Line 2134: <xs:element name="creation_time" type="xs:dateTime"
minOccurs="0"/>
Line 2135: <xs:element name="origin" type="xs:string" minOccurs="0"/>
Line 2136: <xs:element name="stateless" type="xs:boolean"
minOccurs="0"/>
Line 2137: <xs:element name="delete_protected" type="xs:boolean"
minOccurs="0"/>
Line 2138: <xs:element name="console_device_enabled"
type="xs:boolean" minOccurs="0"/>
see comment above
Line 2139: <xs:element name="timezone" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 2140: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/>
Line 2141: <xs:element name="custom_properties"
type="CustomProperties" minOccurs="0"/>
Line 2142: <xs:element name="payloads" type="Payloads" minOccurs="0"/>
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 66: vm.domain.name: xs:string
Line 67: vm.description: xs:string
Line 68: vm.stateless: xs:boolean
Line 69: vm.delete_protected: xs:boolean
Line 70: vm.console_device_enabled: xs:boolean
see required schema changes
Line 71: vm.cpu.topology.sockets: xs:int
Line 72: vm.placement_policy.affinity: xs:string
Line 73: vm.placement_policy.host.id|name: xs:string
Line 74: vm.origin: xs:string
Line 126: vm.domain.name: xs:string
Line 127: vm.description: xs:string
Line 128: vm.stateless: xs:boolean
Line 129: vm.delete_protected: xs:boolean
Line 130: vm.console_device_enabled: xs:boolean
see required schema changes
Line 131: vm.cpu.mode: xs:string
Line 132: vm.cpu.topology.sockets: xs:int
Line 133: vm.placement_policy.affinity: xs:string
Line 134: vm.placement_policy.host.id|name: xs:string
Line 165: vm.domain.name: xs:string
Line 166: vm.description: xs:string
Line 167: vm.stateless: xs:boolean
Line 168: vm.delete_protected: xs:boolean
Line 169: vm.console_device_enabled: xs:boolean
see required schema changes
Line 170: vm.cpu.topology.sockets: xs:int
Line 171: vm.placement_policy.affinity: xs:string
Line 172: vm.placement_policy.host.id|name: xs:string
Line 173: vm.origin: xs:string
Line 2389: template.domain.name: xs:string
Line 2390: template.type: xs:string
Line 2391: template.stateless: 'xs:boolean'
Line 2392: template.delete_protected: xs:boolean
Line 2393: template.console_device_enabled: xs:boolean
see required schema changes
Line 2394: template.placement_policy.affinity: xs:string
Line 2395: template.description: xs:string
Line 2396: template.custom_properties.custom_property--COLLECTION:
{custom_property.name: 'xs:string', custom_property.value: 'xs:string'}
Line 2397: template.os.type: xs:string
Line 2428: template.domain.name: xs:string
Line 2429: template.type: xs:string
Line 2430: template.stateless: 'xs:boolean'
Line 2431: template.delete_protected: xs:boolean
Line 2432: template.console_device_enabled: xs:boolean
see required schema changes
Line 2433: template.placement_policy.affinity: xs:string
Line 2434: template.description: xs:string
Line 2435: template.custom_properties.custom_property--COLLECTION:
{custom_property.name: 'xs:string', custom_property.value: 'xs:string'}
Line 2436: template.os.type: xs:string
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendResource.java
Line 418: return doGetEntity(entityType, query,
getQueryParams(queryParamsClass, id), id.toString());
Line 419: }
Line 420: }
Line 421:
Line 422: protected List<String> getConsoleDevicesForEntity(Guid id) {
this method should not be placed at AbstractBackendResource as it relevant to
vm/template/vmpool only, it should reside in the vm/template/vmpool parent.
Line 423: return getEntity(List.class,
Line 424: VdcQueryType.GetConsoleDevices,
Line 425: new IdQueryParameters(id),
Line 426: null, true);
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendTemplateResource.java
Line 115:
Line 116: UpdateVmTemplateParameters params = new
UpdateVmTemplateParameters(updated);
Line 117: if (incoming.isSetConsoleDeviceEnabled()) {
Line 118:
params.setConsoleEnabled(incoming.isConsoleDeviceEnabled());
Line 119: }
i'd create a dedicated mapper for the UpdateVmTemplateParameters (see [1]),
[1] VmMapper.RunVmOnceParams map(VM vm, RunVmOnceParams template)
Line 120: return params;
Line 121: }
Line 122: }
Line 123:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResource.java
Line 74: // REVISIT: powershell has a IsVmTemlateWithSameNameExist
safety check
Line 75: AddVmTemplateParameters params = new
AddVmTemplateParameters(staticVm,
Line 76: template.getName(),
Line 77: template.getDescription());
Line 78:
params.setConsoleEnabled(!getConsoleDevicesForEntity(staticVm.getId()).isEmpty());
first you should be using value set by user, e.g template.isConsoleEnabled()
and if it's not set only then vm's config
Line 79: boolean isDomainSet = false;
Line 80: if (template.isSetStorageDomain() &&
template.getStorageDomain().isSetId()) {
Line 81:
params.setDestinationStorageDomainId(asGuid(template.getStorageDomain().getId()));
Line 82: isDomainSet = true;
Line 161: }
Line 162:
Line 163: @Override
Line 164: protected Template doPopulate(Template model, VmTemplate entity) {
Line 165: model.setConsoleDeviceEnabled(!getConsoleDevicesForEntity(new
Guid(model.getId())).isEmpty());
you could use id from the VmTemplate entity, this way you won't have to create
new UUID from string
Line 166: return model;
Line 167: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmPoolsResource.java
Line 63:
Line 64: int size = pool.isSetSize() ? pool.getSize() : 1;
Line 65:
Line 66: AddVmPoolWithVmsParameters params = new
AddVmPoolWithVmsParameters(entity, vm, size, -1);
Line 67:
params.setConsoleEnabled(!getConsoleDevicesForEntity(template.getId()).isEmpty());
1. i'd create a dedicated mapper for the AddVmPoolWithVmsParameters (see [1]),
[1] VmMapper.RunVmOnceParams map(VM vm, RunVmOnceParams template)
2. no need to do the same for update() ?
Line 68:
Line 69: return performCreate(VdcActionType.AddVmPoolWithVms,
Line 70: params,
Line 71: new
QueryIdResolver<Guid>(VdcQueryType.GetVmPoolById,
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmResource.java
Line 388: }
Line 389: if (incoming.isSetMemoryPolicy() &&
incoming.getMemoryPolicy().isSetBallooning()) {
Line 390:
params.setBalloonEnabled(incoming.getMemoryPolicy().isBallooning());
Line 391: }
Line 392: if (incoming.isSetConsoleDeviceEnabled()) {
this should be in the doPopulate() not in deprecatedPopulate()
Line 393:
params.setConsoleEnabled(incoming.isConsoleDeviceEnabled());
Line 394: }
Line 395: return params;
Line 396: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmsResource.java
Line 221: params.setDiskInfoDestinationMap(images);
Line 222:
params.setMakeCreatorExplicitOwner(shouldMakeCreatorExplicitOwner());
Line 223: if (vm.isSetConsoleDeviceEnabled()) {
Line 224: params.setConsoleEnabled(vm.isConsoleDeviceEnabled());
Line 225: }
what about 'else', if it was not set by user, you should take it according to
the vmConfiguration mentioned in line 135 (i know it's not in vmstatic, but
you could figure it by vm.template i guess)
Line 226: return performCreate(VdcActionType.AddVmFromSnapshot,
Line 227: params,
Line 228: new
QueryIdResolver<Guid>(VdcQueryType.GetVmByVmId, IdQueryParameters.class));
Line 229: }
Line 235:
params.setBalloonEnabled(vm.getMemoryPolicy().isBallooning());
Line 236: }
Line 237: if (vm.isSetConsoleDeviceEnabled()) {
Line 238: params.setConsoleEnabled(vm.isConsoleDeviceEnabled());
Line 239: }
same here, need "else": ConsoleDeviceEnabled should be set according to template
Line 240:
params.setMakeCreatorExplicitOwner(shouldMakeCreatorExplicitOwner());
Line 241: return performCreate(VdcActionType.AddVmFromTemplate,
Line 242: params,
Line 243: new
QueryIdResolver<Guid>(VdcQueryType.GetVmByVmId, IdQueryParameters.class));
Line 286:
params.setDiskInfoDestinationMap(getDisksToClone(vm.getDisks(), templateId));
Line 287:
params.setMakeCreatorExplicitOwner(shouldMakeCreatorExplicitOwner());
Line 288: if (vm.isSetConsoleDeviceEnabled()) {
Line 289: params.setConsoleEnabled(vm.isConsoleDeviceEnabled());
Line 290: }
same. no "else"
Line 291: return performCreate(VdcActionType.AddVm,
Line 292: params,
Line 293: new
QueryIdResolver<Guid>(VdcQueryType.GetVmByVmId, IdQueryParameters.class));
Line 294: }
....................................................
Commit Message
Line 6:
Line 7: restapi: Make adding (virtio-)console to virtual machines optional
Line 8:
Line 9: This is rest part of a feature that allows users to control attaching
Line 10: virtio console to vm/pools/templates.
do you have a wiki page for it?
Line 11:
Line 12: Change-Id: I733e4c4a9c7d8f5dbe68b6b26a49502b8a40ec9d
Line 13: Bug-Url: https://bugzilla.redhat.com/878459
--
To view, visit http://gerrit.ovirt.org/16454
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I733e4c4a9c7d8f5dbe68b6b26a49502b8a40ec9d
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
