Juan Hernandez has posted comments on this change.
Change subject: bootstrap: rewrite bootstrap using the new ovirt-host-deploy
package
......................................................................
Patch Set 11: (29 inline comments)
I would suggest to break the 1030 lines of the VdsDeploy class in some smaller
classes.
Comments would also help.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsDeploy.java
Line 52: import org.ovirt.ovirt_host_deploy.constants.GlusterEnv;
Line 53: import org.ovirt.ovirt_host_deploy.constants.VdsmEnv;
Line 54:
Line 55: /**
Line 56: * vdsm bootstrap implementatoin.
s/implementatoin/implementation/
You could elaborate here a bit and explain how this works.
Line 57: *
Line 58: * Executed if:
Line 59: * <ul>
Line 60: * <li>Host install.</li>
Line 65: */
Line 66: public class VdsDeploy implements SSHDialog.Sink {
Line 67:
Line 68: private static final int BUFFER_SIZE = 10 * 1024;
Line 69: private static final int THREAD_JOIN_TIMEOUT = 20 * 1000;
What is the unit?
Line 70: private static final String IPTABLE_CUSTOM_RULES_PLACE_HOLDER =
"@CUSTOM_RULES@";
Line 71: private static final String
BOOTSTRAP_CUSTOM_ENVIRONMENT_PLACE_HOLDER = "@ENVIRONMENT@";
Line 72:
Line 73: private static Log log = LogFactory.getLog(VdsDeploy.class);
Line 66: public class VdsDeploy implements SSHDialog.Sink {
Line 67:
Line 68: private static final int BUFFER_SIZE = 10 * 1024;
Line 69: private static final int THREAD_JOIN_TIMEOUT = 20 * 1000;
Line 70: private static final String IPTABLE_CUSTOM_RULES_PLACE_HOLDER =
"@CUSTOM_RULES@";
s/IPTABLE/IPTABLES/
Line 71: private static final String
BOOTSTRAP_CUSTOM_ENVIRONMENT_PLACE_HOLDER = "@ENVIRONMENT@";
Line 72:
Line 73: private static Log log = LogFactory.getLog(VdsDeploy.class);
Line 74: private static CachedTar s_bootstrapPackage;
Line 69: private static final int THREAD_JOIN_TIMEOUT = 20 * 1000;
Line 70: private static final String IPTABLE_CUSTOM_RULES_PLACE_HOLDER =
"@CUSTOM_RULES@";
Line 71: private static final String
BOOTSTRAP_CUSTOM_ENVIRONMENT_PLACE_HOLDER = "@ENVIRONMENT@";
Line 72:
Line 73: private static Log log = LogFactory.getLog(VdsDeploy.class);
This ^ should be final.
Line 74: private static CachedTar s_bootstrapPackage;
Line 75: private static File s_bootstrapLogDir;
Line 76:
Line 77: private SSHDialog.Control _control;
Line 74: private static CachedTar s_bootstrapPackage;
Line 75: private static File s_bootstrapLogDir;
Line 76:
Line 77: private SSHDialog.Control _control;
Line 78: private Thread _thread;
Thread for what?
Line 79: private EngineSSHDialog _dialog;
Line 80: private MachineDialogParser _parser;
Line 81: private final InstallerMessages _messages;
Line 82:
Line 80: private MachineDialogParser _parser;
Line 81: private final InstallerMessages _messages;
Line 82:
Line 83: private VDS _vds;
Line 84: private boolean _node = false;
I would suggest to rename this to isNode or isOVirtNode.
Line 85: private boolean _reboot = false;
Line 86: private Exception _failException = null;
Line 87: private boolean _resultError = false;
Line 88: private boolean _goingToReboot = false;
Line 96: * set vds object with unique id.
Line 97: * @param vdsmid unique id read from host.
Line 98: *
Line 99: * Check if vdsmid is unique, if not, halt installation, otherwise
Line 100: * update the vds object.
In javadoc comments the description of the parameters should be the last thing,
otherwise the resulting HTML shows the "Check if ..." text as the description
of the parameter.
Line 101: */
Line 102: private void _setVdsmId(String vdsmid) {
Line 103: if (vdsmid == null) {
Line 104: throw new SoftError("Cannot acquire node id");
Line 98: *
Line 99: * Check if vdsmid is unique, if not, halt installation, otherwise
Line 100: * update the vds object.
Line 101: */
Line 102: private void _setVdsmId(String vdsmid) {
If you ever make this public this _ will break the JavaBeans specification.
Line 103: if (vdsmid == null) {
Line 104: throw new SoftError("Cannot acquire node id");
Line 105: }
Line 106:
Line 109: _vds.gethost_name(),
Line 110: vdsmid
Line 111: );
Line 112:
Line 113: List<VDS> list = LinqUtils.filter(
Consider making this local variable final.
Line 114:
DbFacade.getInstance().getVdsDao().getAllWithUniqueId(vdsmid),
Line 115: new Predicate<VDS>() {
Line 116: @Override
Line 117: public boolean eval(VDS vds) {
Line 120: }
Line 121: );
Line 122:
Line 123: if (!list.isEmpty()) {
Line 124: StringBuffer hosts = new StringBuffer(1024);
Use StringBuilder instead of StringBuffer, and consider making it final
Line 125: for (VDS v : list) {
Line 126: if (hosts.length() > 0) {
Line 127: hosts.append(", ");
Line 128: }
Line 176: /**
Line 177: * Set vds object status.
Line 178: * @param status new status.
Line 179: *
Line 180: * For this simple task, no need to go via command mechanism.
Description of the method should go before the parameters.
Line 181: */
Line 182: private void _setVdsStatus(VDSStatus status) {
Line 183: _vds.setstatus(status);
Line 184:
Line 204: in = new FileInputStream(keystoreFile);
Line 205: KeyStore ks = KeyStore.getInstance("PKCS12");
Line 206: ks.load(
Line 207: in,
Line 208:
Config.<String>GetValue(ConfigValues.keystorePass).toCharArray()
Another leaked password.
Line 209: );
Line 210:
Line 211: Certificate cert = ks.getCertificate(alias);
Line 212: if (cert == null) {
Line 288: * Customization vector.
Line 289: * This is tick based vector, every event execute the next
Line 290: * tick.
Line 291: */
Line 292: private Callable _customizationDialog[] = new Callable[] {
Recommended practice in Java when declaring arrays is to put the array
specifier after the type:
private Callable[] _customizationDialog = new Callable[] {
...
}
Line 293: new Callable<Object>() { public Object call() throws
Exception {
Line 294: if (
Line 295: (Boolean)_parser.cliEnvironmentGet(
Line 296: VdsmEnv.OVIRT_NODE
Line 291: */
Line 292: private Callable _customizationDialog[] = new Callable[] {
Line 293: new Callable<Object>() { public Object call() throws
Exception {
Line 294: if (
Line 295: (Boolean)_parser.cliEnvironmentGet(
I would prefer if you can avoid these casts.
Line 296: VdsmEnv.OVIRT_NODE
Line 297: )
Line 298: ) {
Line 299: _messages.post(
Line 297: )
Line 298: ) {
Line 299: _messages.post(
Line 300: InstallerMessages.Severity.INFO,
Line 301: "Host is hypervisor"
Fedora or RHEL hosts are hypervisors as well. Do you mean "Host is oVirt Node"?
Line 302: );
Line 303: _setNode();
Line 304: }
Line 305: return null;
Line 382: }},
Line 383: new Callable<Object>() { public Object call() throws
Exception {
Line 384: _parser.cliEnvironmentSet(
Line 385: VdsmEnv.ENGINE_HOST,
Line 386: new URL(Config.<String>
GetValue(ConfigValues.VdcBootStrapUrl)).getHost()
Replace this with LocalConfig.getInstance().getHost().
Line 387: );
Line 388: return null;
Line 389: }},
Line 390: new Callable<Object>() { public Object call() throws
Exception {
Line 389: }},
Line 390: new Callable<Object>() { public Object call() throws
Exception {
Line 391: _parser.cliEnvironmentSet(
Line 392: VdsmEnv.ENGINE_PORT,
Line 393: new URL(Config.<String>
GetValue(ConfigValues.VdcBootStrapUrl)).getPort()
Replace this with LocalConfig.getInstance().getExternalHttpsPort().
Line 394: );
Line 395: return null;
Line 396: }},
Line 397: new Callable<Object>() { public Object call() throws
Exception {
Line 426: }},
Line 427: new Callable<Object>() { public Object call() throws
Exception {
Line 428: _parser.cliEnvironmentSet(
Line 429: VdsmEnv.CERTIFICATE_ENROLLMENT,
Line 430:
org.ovirt.ovirt_host_deploy.constants.Const.CERTIFICATE_ENROLLMENT_INLINE
While the fully qualified name of the Const class instead of import it?
Line 431: );
Line 432: return null;
Line 433: }},
Line 434: new Callable<Object>() { public Object call() throws
Exception {
Line 436: _vds.getvds_group_id()
Line 437: );
Line 438: _parser.cliEnvironmentSet(
Line 439: GlusterEnv.ENABLE,
Line 440: vdsGroup.supportsGlusterService() != false
What is the point of the "!= false", (x != false) == x.
Line 441: );
Line 442: return null;
Line 443: }},
Line 444: new Callable<Object>() { public Object call() throws
Exception {
Line 453: "Enfocing host reboot"
Line 454: );
Line 455: }
Line 456: _parser.cliEnvironmentSet(
Line 457:
org.ovirt.ovirt_host_deploy.constants.CoreEnv.FORCE_REBOOT,
Why not import Const?
Line 458: reboot
Line 459: );
Line 460: return null;
Line 461: }},
Line 462: new Callable<Object>() { public Object call() throws
Exception {
Line 463: _parser.cliInstall();
Line 464: return null;
Line 465: }},
Line 466: null
Why the null at the end of the array? This is not needed in Java as the length
of the array is an intrinsic property.
Line 467: };
Line 468: /**
Line 469: * Execute the next customization vector entry.
Line 470: */
Line 473: if (_customizationShouldAbort) {
Line 474: _parser.cliAbort();
Line 475: }
Line 476: else {
Line 477: _customizationDialog[_customizationIndex++].call();
This doesn't check the length of the array of for the presence of the null
element at the end. How do you prevent ArrayIndexOutOfBounds or NullPointer?
Line 478: }
Line 479: }
Line 480: catch (SoftError e) {
Line 481: log.errorFormat(
Line 500: * Termination vector.
Line 501: * This is tick based vector, every event execute the next
Line 502: * tick.
Line 503: */
Line 504: private Callable _terminationDialog[] = new Callable[] {
Callable[] _term...
Line 505: new Callable<Object>() { public Object call() throws
Exception {
Line 506: _resultError = (Boolean)_parser.cliEnvironmentGet(
Line 507: BaseEnv.ERROR
Line 508: );
Line 605: new Callable<Object>() { public Object call() throws
Exception {
Line 606: _parser.cliQuit();
Line 607: return null;
Line 608: }},
Line 609: null
This null is not needed in Java.
Line 610: };
Line 611: /**
Line 612: * Execute the next termination vector entry.
Line 613: */
Line 611: /**
Line 612: * Execute the next termination vector entry.
Line 613: */
Line 614: private void _nextTerminationEntry() throws Exception {
Line 615: _terminationDialog[_terminationIndex++].call();
How do you prevent ArrayIndexOutOfBounds and NullPointer?
Line 616: }
Line 617:
Line 618: /**
Line 619: * Dialog implementation.
Line 646: severity =
InstallerMessages.Severity.WARNING;
Line 647: break;
Line 648: default:
Line 649: severity =
InstallerMessages.Severity.ERROR;
Line 650: break;
Adjust the format of the switch to what we use in the rest of the project.
Line 651: }
Line 652: _messages.post(severity, event.record);
Line 653: }
Line 654: else if (bevent instanceof Event.Confirm) {
Line 778: String cachedir = System.getenv("ENGINE_CACHE");
Line 779: if (cachedir == null) {
Line 780: log.warn("ENGINE_CACHE environment not found using
tmpdir");
Line 781: cachedir = System.getProperty("java.io.tmpdir");
Line 782: }
Replace with LocalConfig.getInstance().getCacheDir().
Line 783: s_bootstrapPackage = new CachedTar(
Line 784: new File(cachedir, Config.<String>
GetValue(ConfigValues.BootstrapPackageName)),
Line 785: new File(Config.<String>
GetValue(ConfigValues.BootstrapPackageDirectory))
Line 786: );
Line 790: String logdir = System.getenv("ENGINE_LOG");
Line 791: if (logdir == null) {
Line 792: log.warn("ENGINE_LOG environment not found using
tmpdir");
Line 793: s_bootstrapLogDir = new
File(System.getProperty("java.io.tmpdir"));
Line 794: }
Replace with LocalConfig.getInstance().getLogDir().
Line 795: else {
Line 796: s_bootstrapLogDir = new File(logdir, "bootstrap");
Line 797: if (!s_bootstrapLogDir.exists()) {
Line 798: s_bootstrapLogDir.mkdirs();
Line 806: */
Line 807: @Override
Line 808: public void finalize() {
Line 809: close();
Line 810: }
Don't use finalizers (and don't call them destructors, as they aren't).
Line 811:
Line 812: /**
Line 813: * Release resources.
Line 814: */
--
To view, visit http://gerrit.ovirt.org/9175
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If78c62601231f4729ca95da7653907b37856d672
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
