Ori Liel has posted comments on this change.
Change subject: core, engine, webadmin: Initial support for alternative
architectures
......................................................................
Patch Set 8:
(5 comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 59: vm.display.smartcard_enabled: xs:boolean
Line 60: vm.display.keyboard_layout: xs:string
Line 61: vm.os.cmdline: xs:string
Line 62: vm.cpu.mode: xs:string
Line 63: vm.cpu.architecture: xs:string
There was a discussion in patch set 3 between you and Michael Pasternak about
whether this parameter is modifiable or not. I didn't understand the bottom
line.
>From your changes in this file, it appears that the parameter is modifiable:
>user may supply cpu architecture when updating a VM, a Cluster or a Template (
>and also when creating).
But in the 'mapper' classes - ClusterMapper, VmMapper, TemplateMapper - you
only wrote the mapping for the direction Engine-->API, which would suggest that
this parameter is non-modifiable.
Even if you only allow setting cpu-architecture in 'add' (without 'update'),
you'd still need mapping: API-->Engine, so the current situation doesn't make
sense to me.
Could you please clarify the matter? If necessary, distinguish between 'add'
and 'update' in your explanation.
Line 64: vm.cpu.topology.cores: xs:int
Line 65: vm.cpu_shares: xs:int
Line 66: vm.memory: xs:long
Line 67: vm.high_availability.priority: xs:int
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmPoolsResource.java
Line 60: if (namedCluster(pool)) {
Line 61: pool.getCluster().setId(getClusterId(pool));
Line 62: }
Line 63:
Line 64: if (template.getArchitecture() != null) {
1) Here the cpu-architecture is taken from the template. About ~10 lines below
'map(pool)' is invoked, and in there cpu-architecture is taken from the
cluster, if user supplied it. Is this the intended behavior? If user supplied
it in the cluster, it would override the value from the template?
2) If this parameter can be given for VmPool creation, this should be stated in
rsdl_metadata.yaml file.
Line 65: if (pool.getCluster().isSetCpu()) {
Line 66:
pool.getCluster().getCpu().setArchitecture(ClusterMapper.map(template.getArchitecture(),
null));
Line 67: }
Line 68: else {
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ClusterMapper.java
Line 95: if (entity.getcpu_name() != null) {
Line 96: CPU cpu = new CPU();
Line 97: cpu.setId(entity.getcpu_name());
Line 98:
Line 99: cpu.setArchitecture(map(entity.getArchitectureType(),
null));
See my comment in rsdl_metadata.yaml file. This is one-sided mapping
(Engine-->API) but in rsdl_metadata.yaml you state that it's possible to supply
cpu architecture on update, which would require API-->Engine mapping.
Line 100:
Line 101: model.setCpu(cpu);
Line 102: }
Line 103: if (entity.getStoragePoolId() != null) {
Line 300: return null;
Line 301: }
Line 302: }
Line 303: return null;
Line 304: }
If parameter is non-updatable, no reason for 2-way mapping of the enum
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/TemplateMapper.java
Line 312:
model.getDisplay().setKeyboardLayout(entity.getVncKeyboardLayout());
Line 313: }
Line 314: if (entity.getArchitecture() != null) {
Line 315:
model.getCpu().setArchitecture(ClusterMapper.map(entity.getArchitecture(),
null));
Line 316: }
See my comment in rsdl_metadata.yaml file. This is one-sided mapping
(Engine-->API) but in rsdl_metadata.yaml you state that it's possible to supply
cpu architecture on update, which would require API-->Engine mapping.
Line 317: if (entity.getCreationDate() != null) {
Line 318:
model.setCreationTime(DateMapper.map(entity.getCreationDate(), null));
Line 319: }
Line 320: if (entity.getDomain()!=null &&
!entity.getDomain().isEmpty()) {
--
To view, visit http://gerrit.ovirt.org/16700
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I33ed9231a6467aa59e8f3223ba9d61b6e68039fa
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vitor de Lima <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Leonardo Bianconi <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vitor de Lima <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
