Alon Bar-Lev has posted comments on this change.
Change subject: bootstrap: rewrite bootstrap using the new ovirt-host-deploy
package
......................................................................
Patch Set 11: (27 inline comments)
Thanks for review!
> I would suggest to break the 1030 lines of the VdsDeploy class in some
> smaller classes.
Can you please suggest how to split it?
I don't want to see the customization dialog and termination dialog out...
these are the reason the code is long, but are as if they are written in-line.
Other than that I don't see any atomic logic I can split out.
Thanks!
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsDeploy.java
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;
Added comment.
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@";
fixed (as-is in previous code...)
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);
Done
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;
For handling the dialog.
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;
isNode it is.
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.
Done. This is why I like doxygen better :)
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) {
Right. If it ever get public, then the _ will be removed as it is not private.
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(
list is local variable....
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);
Done
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.
Done
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()
Done
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[] {
Did not know that, I saw both notation. Fixed.
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(
How? I need a boolean here...
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"
As far as I understand both falls into the term "hypervisor".
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()
Why? the config value is kept to it may be changed. Do you want to obsolete the
VdcBootstrapUrl?
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()
Same as above, are we sure we want to change this at this point?
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
Because there is a conflict, and java does not support scope rename.
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
Right, sorry.
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,
Namespace conflict.
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
Yes, but it will make my life easier if I just iterate through elements.
But this is not the real reason... the real reason is that I want to able to
add new elements without touching the ',' of existing code. Patches will be
cleaner.
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();
I don't.
If there is an error of such the exception is perfectly OK. It should exit
because of the sequence.
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[] {
Done
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
Same as above, no need to repeat comments.
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();
Same as above.
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;
Sorry, I missed this one.
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: }
Done, are you sure it will always be exist? (development mode)
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: }
Done
Line 795: else {
Line 796: s_bootstrapLogDir = new File(logdir, "bootstrap");
Line 797: if (!s_bootstrapLogDir.exists()) {
Line 798: s_bootstrapLogDir.mkdirs();
--
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
