ofri masad has posted comments on this change.
Change subject: enigne:Trusted Compute Pools - Open Attestation integration
with oVirt engine proposal
......................................................................
Patch Set 3: (10 inline comments)
....................................................
File backend/manager/dbscripts/vds_groups_sp.sql
Line 24: v_migrate_on_error INTEGER,
Line 25: v_virt_service BOOLEAN,
Line 26: v_gluster_service BOOLEAN,
Line 27: v_tunnel_migration BOOLEAN,
Line 28: v _trusted_service BOOLEAN)
the space after "v" looks like a mistake
Line 29: RETURNS VOID
Line 30: AS $procedure$
Line 31: BEGIN
Line 32: INSERT INTO vds_groups(vds_group_id,description, name, cpu_name,
selection_algorithm, high_utilization, low_utilization,
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationService.java
Line 69: }
Line 70:
Line 71: public boolean validateHostIsTrusted(VDS vds) {
Line 72: List<String> hosts = new ArrayList<String>();
Line 73: hosts.add(vds.gethost_name());
please make sure you are rebased. this method was renamed on Feb 11.
Line 74: List<AttestationValue> valueList = attestHosts(hosts);
Line 75: return valueList.get(0).getTrustLevel() ==
AttestationResultEnum.TRUSTED;
Line 76: }
Line 77:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestThread.java
Line 12:
Line 13: public class AttestThread extends Thread{
Line 14:
Line 15: private int attestationFirstStageSize = Config.<Integer>
GetValue(ConfigValues.AttestationFirstStageSize);
Line 16: private List<VDS> vdss = new ArrayList<VDS>();
redundant initialization
Line 17:
Line 18: public AttestThread(){
Line 19:
Line 20: }
Line 48: updateCache(valueList1);
Line 49: if (curVdsNames2.size() > 0) {
Line 50: valueList2 =
AttestationService.getInstance().attestHosts(curVdsNames2);
Line 51: updateCache(valueList2);
Line 52: }
Lines 43-52 should be handled in a 'while' loop. what if the hosts list is 10
times bigger than attestationFirstStageSize?
Use the 'while' loop - each time take X names from the big list and send them
over (until the big list is empty).
Line 53: }
Line 54:
Line 55: private void updateCache(List<AttestationValue> valueList){
Line 56: for (AttestationValue value: valueList){
Line 51: updateCache(valueList2);
Line 52: }
Line 53: }
Line 54:
Line 55: private void updateCache(List<AttestationValue> valueList){
encapsulate this in the AttestationCacheManager. if value is found update it,
else add it.
Line 56: for (AttestationValue value: valueList){
Line 57: if
(AttestationCacheManager.getInstance().exists(value.getHostName())){
Line 58:
AttestationCacheManager.getInstance().updateCache(value);
Line 59: }else{
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitVdsOnUpCommand.java
Line 70: super(parameters);
Line 71: setVds(parameters.getVds());
Line 72: }
Line 73:
Line 74: private boolean validateHost() {
please set a more defining name, like validateHostIsTrusted.
Line 75: if
(AttestationService.getInstance().validateHostIsTrusted(getVds())) {
Line 76: return true;
Line 77: } else {
Line 78: setNonOperational(NonOperationalReason.GENERAL, null);
Line 74: private boolean validateHost() {
Line 75: if
(AttestationService.getInstance().validateHostIsTrusted(getVds())) {
Line 76: return true;
Line 77: } else {
Line 78: setNonOperational(NonOperationalReason.GENERAL, null);
please add a new NonOperationalReason "UNTRUSTED"
Line 79: return false;
Line 80: }
Line 81: }
Line 82:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/attestation/AttestationCacheManager.java
Line 43: public void updateCache(AttestationValue value) {
Line 44: AttestationValue cacheValue =
attestationValues.get(value.getHostName());
Line 45: if (cacheValue != null) {
Line 46: cacheValue.setTrustLevel(value.getTrustLevel());
Line 47: }
there are two options here:
a. (the better one) if value is not in cache - call addToCach(value)
b. return a boolean value - was the value cached or not.
Line 48: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/attestation/AttestationValue.java
Line 5:
Line 6: public class AttestationValue {
Line 7:
Line 8: private String hostName;
Line 9: private AttestationResultEnum trustLevel;
Can a host be only trusted/untrusted. otherwise, we may want to use an integer
value (for example: host trust level is 74%). that way the user could configure
the trust threshold.
Line 10:
Line 11: public AttestationValue() {
Line 12: trustLevel = AttestationResultEnum.UNKNOWN;
Line 13: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java
Line 97: &&
ObjectUtils.objectsEqual(vdsGroupCompatibilityVersion,
other.vdsGroupCompatibilityVersion)
Line 98: && ObjectUtils.objectsEqual(vdsGroupCpuName,
other.vdsGroupCpuName)
Line 99: && ObjectUtils.objectsEqual(vdsGroupDescription,
other.vdsGroupDescription)
Line 100: && ObjectUtils.objectsEqual(vdsGroupName,
other.vdsGroupName)
Line 101: $$ trustedService == other.trustedService);
what is $$ ?
please use ObjectUtils.objectsEqual(trustedService, other.trustedService)
Line 102:
Line 103: }
Line 104:
Line 105: public VDS(Guid vds_group_id, String vds_group_name, String
vds_group_description, Guid vds_id, String vds_name,
--
To view, visit http://gerrit.ovirt.org/14605
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8ce3448a821c74521d277f92f2c8d63ba0accfed
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <[email protected]>
Gerrit-Reviewer: Dave Chen <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gang Wei <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
