Omer Frenkel has posted comments on this change.
Change subject: Trusted Compute Pools - Open Attestation integration with oVirt
engine proposal
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(11 inline comments)
i think we need to think of a better solution to have this info cached per
host, or somthing similar.
the way it is now, it may cause a major increase in time it will take to run vm
(trust validation communicates with host, might be risky), also in case of
running multiple vms this check will run per vm and per host, so if i run 50
vms i will check each host 50 times for trust.
....................................................
File backend/manager/dbscripts/create_tables.sql
Line 279: migration_support INTEGER NOT NULL default 0,
Line 280: userdefined_properties VARCHAR(4000),
Line 281: predefined_properties VARCHAR(4000),
Line 282: min_allocated_mem INTEGER not null default 0, --indicates how much
memory at least VM need to run, value is in MB
Line 283: trust_lvl VARCHAR(20) default '',
this file should not be changed,
updates to schema are done with upgrade scripts,
please create an upgrade script, you can see an example in
backend/manager/dbscripts/upgrade/03_02_0260_add_delete_protection.sql
Line 284: CONSTRAINT PK_vm_static PRIMARY KEY(vm_guid)
Line 285: ) WITH OIDS;
Line 286:
Line 287:
....................................................
File backend/manager/dbscripts/vms_sp.sql
Line 433: RETURNS VOID
Line 434: AS $procedure$
Line 435: BEGIN
Line 436: INSERT INTO vm_static(description, mem_size_mb, os, vds_group_id,
vm_guid, VM_NAME,
vmt_guid,domain,creation_date,num_of_monitors,allow_console_reconnect,is_initialized,is_auto_suspend,num_of_sockets,cpu_per_socket,usb_policy,
time_zone,auto_startup,is_stateless,dedicated_vm_for_vds, fail_back,
default_boot_sequence, vm_type, nice_level, default_display_type,
priority,iso_path,origin,initrd_url,kernel_url,kernel_params,migration_support,predefined_properties,userdefined_properties,min_allocated_mem,
entity_type, quota_id, cpu_pinning,
is_smartcard_enabled,is_delete_protected,host_cpu_flags,trust_lvl)
Line 437: VALUES(v_description, v_mem_size_mb, v_os, v_vds_group_id,
v_vm_guid, v_vm_name, v_vmt_guid, v_domain, v_creation_date, v_num_of_monitors,
v_allow_console_reconnect, v_is_initialized, v_is_auto_suspend,
v_num_of_sockets, v_cpu_per_socket, v_usb_policy, v_time_zone,
v_auto_startup,v_is_stateless,v_dedicated_vm_for_vds,v_fail_back,
v_default_boot_sequence, v_vm_type, v_nice_level, v_default_display_type,
v_priority,v_iso_path,v_origin,v_initrd_url,v_kernel_url,v_kernel_params,v_migration_support,v_predefined_properties,v_userdefined_properties,v_min_allocated_mem,
'VM', v_quota_id, v_cpu_pinning,
v_is_smartcard_enabled,v_is_delete_protected,v_host_cpu_flags,v_trust_lvl);
missing also changes in functions: UpdateVmStatic, insertVm
Line 438: -- perform deletion from vm_ovf_generations to ensure that no record
exists when performing insert to avoid PK violation.
Line 439: DELETE FROM vm_ovf_generations gen WHERE gen.vm_guid = v_vm_guid;
Line 440: INSERT INTO vm_ovf_generations(vm_guid, storage_pool_id) VALUES
(v_vm_guid, (SELECT storage_pool_id FROM vds_groups vg WHERE vg.vds_group_id =
v_vds_group_id));
Line 441: END; $procedure$
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java
Line 415: private Guid getVdsToRunOn(Iterable<VDS> vdss, boolean isMigrate)
{
Line 416: StringBuilder sb = new StringBuilder();
Line 417: final List<VDS> readyToRun = new ArrayList<VDS>();
Line 418: for (VDS curVds : vdss) {
Line 419: if (validateHostIsReadyToRun(curVds, sb, isMigrate) ==
ValidationResult.VALID&&validateHostIsTrusted(curVds)){
why call to validateHostIsTrusted is not a step inside validateHostIsReadyToRun?
this can cause problems because we may pass canDoAction phase thinking we have
a host to run on, but actually we dont.
Line 420: readyToRun.add(curVds);
Line 421: }
Line 422: }
Line 423:
Line 417: final List<VDS> readyToRun = new ArrayList<VDS>();
Line 418: for (VDS curVds : vdss) {
Line 419: if (validateHostIsReadyToRun(curVds, sb, isMigrate) ==
ValidationResult.VALID&&validateHostIsTrusted(curVds)){
Line 420: readyToRun.add(curVds);
Line 421: }
please use the engine formatter to have missing spaces here and remove the
white space as well
Line 422: }
Line 423:
Line 424: if (readyToRun.isEmpty()) {
Line 425: log.info(sb.toString());
Line 570: }finally {
Line 571: httpclient.getConnectionManager().shutdown();
Line 572: }
Line 573: return flag;
Line 574: }
this code contains lots of hard-coded values that need to be configurable or
held staticaly at least
in addition, code that access the host should reside in the vdsBroker package
and expose a command to run it, i would even aim for a vdsm verb to actually
execute this and just return the result
Line 575: private static final Log log =
LogFactory.getLog(VdsSelector.class);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java
Line 68: @Column(name = "host_cpu_flags", nullable = false)
Line 69: private boolean useHostCpuFlags = false;
Line 70:
Line 71: @Column(name = "trust_lvl")
Line 72: private String trust_lvl="";
please use java naming convention
Line 73:
Line 74: public VmStatic() {
Line 75: setNumOfMonitors(1);
Line 76: initialized = false;
Line 78: setNiceLevel(0);
Line 79: setDefaultBootSequence(BootSequence.C);
Line 80: defaultDisplayType = DisplayType.qxl;
Line 81: setVmType(VmType.Desktop);
Line 82: trust_lvl="trusted";
why setting default to trusted for some of the cases? i think it should be
empty to keep current behavior.
Line 83: }
Line 84:
Line 85: public VmStatic(VmStatic vmStatic) {
Line 86: super(vmStatic.getId(),
Line 263: this.trust_lvl = trust_lvl;
Line 264: }
Line 265:
Line 266: @Override
Line 267: public int hashCode() {
please update this method as well
Line 268: final int prime = 31;
Line 269: int result = super.hashCode();
Line 270: result = prime * result + ((defaultDisplayType == null) ? 0 :
defaultDisplayType.hashCode());
Line 271: result = prime * result + (initialized ? 1231 : 1237);
Line 278: return result;
Line 279: }
Line 280:
Line 281: @Override
Line 282: public boolean equals(Object obj) {
please update this method as well
Line 283: if (this == obj) {
Line 284: return true;
Line 285: }
Line 286: if (!super.equals(obj)) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1333: HardwareInfoEnabled(410),
Line 1334:
Line 1335: @TypeConverterAttribute(String.class)
Line 1336:
@DefaultValueAttribute("https://oat-server:8443/AttestationService/resources";)
Line 1337: AttestationWebServicesUrl(411),
configuration values are saved in the db, please add it in
backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 1338:
Line 1339: @TypeConverterAttribute(String.class)
Line 1340: @DefaultValueAttribute("/etc/pki/ovirt-engine/certs")
Line 1341: TrustStore(412),
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAODbFacadeImpl.java
Line 83: .addValue("quota_id", vm.getQuotaId())
Line 84: .addValue("allow_console_reconnect",
vm.isAllowConsoleReconnect())
Line 85: .addValue("cpu_pinning", vm.getCpuPinning())
Line 86: .addValue("host_cpu_flags", vm.isUseHostCpuFlags())
Line 87: .addValue("trust_lvl", vm.gettrust_lvl());
missing impl in VMStaticRowMapper as well (for getting the value from db..)
also missing changes in VMDAODbFacadeImpl please also update fixtures.xml for
db unit tests
Line 88: }
Line 89:
Line 90: @Override
Line 91: public void remove(Guid id) {
--
To view, visit http://gerrit.ovirt.org/11237
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4de780cd46069638433255f3f9c994575f752e55
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
