Allon Mureinik has posted comments on this change.
Change subject: core: introduce osinfo service instead of Config values
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(7 inline comments)
Some minor suggestions, and a possible bug with calling the shellscript
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidationUtils.java
Line 8: import org.ovirt.engine.core.utils.osinfo.OSInfoService;
Line 9:
Line 10: public class VmValidationUtils {
Line 11: private static final String X64BIT = "x64";
Line 12: private static OSInfoService osinfoService =
LibosinfoClient.getService();
You probably want to make this final
Line 13:
Line 14: /**
Line 15: * Check if the memory size is within the correct limits (as per
the configuration), taking into account the
Line 16: * OS type.
Line 34: * @return The minimum VM memory size allowed (as per
configuration).
Line 35: */
Line 36: public static Integer getMinMemorySizeInMb(VmOsType osType) {
Line 37: long minimumRam =
Line 38:
osinfoService.getMinimumRam(OSIdentifiers.from(osType), osType.getIs64Bit() ?
"x86_64" : "i386");
why should the client (=oVirt engine) be aware of x86/i386 ?
Line 39: return (int) (minimumRam == -1 ? -1 :
bytesToMegaBytes(minimumRam));
Line 40: }
Line 41:
Line 42: private static long bytesToMegaBytes(long value) {
Line 39: return (int) (minimumRam == -1 ? -1 :
bytesToMegaBytes(minimumRam));
Line 40: }
Line 41:
Line 42: private static long bytesToMegaBytes(long value) {
Line 43: return value / (1024 * 1024);
Don't we have a gazilion of those? shouldn't it be in some util?
Line 44: }
Line 45:
Line 46: /**
Line 47: * Get the configured maximum VM memory size for this OS type.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/osinfo/LibosinfoClient.java
Line 68:
Line 69: libosinfoRMIServer = builder.start();
Line 70:
Line 71: } catch (Exception exception) {
Line 72: log.error("Can't start exernal VM.");
Perhaps add an AuditLog too?
Line 73: }
Line 74: }
Line 75:
Line 76:
Line 75:
Line 76:
Line 77: private ProcessBuilder buildShellProcessCmd() {
Line 78: ProcessBuilder builder = new ProcessBuilder();
Line 79: String cmd = LocalConfig.getInstance().getUsrDir().getPath() +
"/bin/engine-osinfo.sh/";
Is the "/" at the end of the command intentional?
Line 80: return builder.command(cmd);
Line 81: }
Line 82:
Line 83: private void newLibosinfoRMIProxy() {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/osinfo/OSIdentifiers.java
Line 247: openSUSE_11_2("http://opensuse.org/opensuse/11.2";),
Line 248: openSUSE_11_3("http://opensuse.org/opensuse/11.3";),
Line 249: openSUSE_11_4("http://opensuse.org/opensuse/11.4";),
Line 250: openSUSE_12_1("http://opensuse.org/opensuse/12.1";),
Line 251: ;
Can this be auto-generated somehow?
Seems like a sizeable overhead to have to maintain it
Line 252: private String id;
Line 253: private List<VmOsType> osTypes;
Line 254:
Line 255: private static Map<VmOsType, OSIdentifiers> osTypeLookup =
Line 252: private String id;
Line 253: private List<VmOsType> osTypes;
Line 254:
Line 255: private static Map<VmOsType, OSIdentifiers> osTypeLookup =
Line 256: new HashMap<VmOsType, OSIdentifiers>();
EnumMap should be faster, AFAIK
Line 257:
Line 258: static {
Line 259: for (OSIdentifiers member : values()) {
Line 260: if (!member.osTypes.contains(VmOsType.Other)) {
--
To view, visit http://gerrit.ovirt.org/12936
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd75679a1a1af5d5a0925e181b5dfd6e87574a75
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
