Doron Fediuck has posted comments on this change.
Change subject: bootstrap: send complete bootstrap from engine
......................................................................
Patch Set 15: I would prefer that you didn't submit this
(6 inline comments)
Hi Alon,
There's an issue with the import you introduced, which we cannot have right now.
Also, some concerns about concurrency. See inline.
Last thing, since vds installer is being inherited, did you check these changes
work well with ovirt node?
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
Line 122: }
Line 123: s_bootstrapPackage = new CachedTar(
Line 124: new File(cache, Config.<String>
GetValue(ConfigValues.BootstrapPackageName)),
Line 125: new File(Config.<String>
GetValue(ConfigValues.BootstrapPackageDirectory))
Line 126: );
Will this run on every installation?
ie- what happens if I try to run 56 simultaneous installations?
What if the tar is being refreshed in the middle? Which file will every
instance use?
Line 127: }
Line 128:
Line 129: VdsGroupDAO vdsGroupDao =
DbFacade.getInstance().getVdsGroupDAO();
Line 130: Guid vdsGroupId = vds.getvds_group_id();
Line 143: if (!supportVirt) {
Line 144: _bootstrapCommand =
_bootstrapCommand.replace("{virt-placeholder}", "-V");
Line 145: } else {
Line 146: _bootstrapCommand =
_bootstrapCommand.replace("{virt-placeholder}", "");
Line 147: }
Why not use a simple-
string filler = supportVirt ? "-V" : "";
_bootstrapCommand = _bootstrapCommand.replace("{virt-placeholder}", filler);
and reuse filler in the lines bellow
VVVVV
Line 148:
Line 149: // We pass -g option if gluster is supported on this host
Line 150: if (supportGluster) {
Line 151: _bootstrapCommand =
_bootstrapCommand.replace("{gluster-placeholder}", "-g");
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1424: "-O '{OrganizationName}' -t {utc_time}
{OverrideFirewall} " +
Line 1425: "-S {SSHKey} {EnginePort} -b {virt-placeholder} " +
Line 1426: "{gluster-placeholder} {URL1} {URL1} {vds-server} " +
Line 1427: "{GUID} {RunFlag}"
Line 1428: )
Alon,
Can you please check how backwards compatible should this be?
Line 1429: BootstrapCommand(373),
Line 1430:
Line 1431: @TypeConverterAttribute(Integer.class)
Line 1432: @DefaultValueAttribute("10000")
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/Tar.java
Line 86: *
Line 87: * Only regular files and directories are supported.
Line 88: * Files will be owner rw and optional execute bit.
Line 89: */
Line 90: public static void doTar(
Are you sure this is thread safe / OS safe?
ie- can you avoid concurrency issues by working with tmp file / archinve?
Line 91: OutputStream os,
Line 92: File base
Line 93: ) throws SecurityException, IOException {
Line 94: TarArchiveOutputStream archive = null;
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/VdsInstallerSSH.java
Line 524: }
Line 525:
Line 526: fingerprint = OpenSSHUtils.getKeyString(
Line 527: cert.getPublicKey(),
Line 528: "ovirt-engine"
why is this hard-coded?
What happens if we want to change it to engine2?
Can you please take this from configuration?
Line 529: );
Line 530: }
Line 531: catch (Exception e) {
Line 532: log.error(
....................................................
File pom.xml
Line 24: <commons-logging.version>1.1</commons-logging.version>
Line 25: <junit.version>4.7</junit.version>
Line 26: <commons-codec.version>1.4</commons-codec.version>
Line 27: <commons-lang.version>2.4</commons-lang.version>
Line 28: <commons-compress.version>1.4.1</commons-compress.version>
If this should be used in ovirt engine 3.1,
we cannot add this import.
Line 29: <quartz.version>2.1.2</quartz.version>
Line 30: <postgres.jdbc.version>8.4-702.jdbc4</postgres.jdbc.version>
Line 31: <commons-collections>3.1</commons-collections>
Line 32: <javax.transaction>1.1</javax.transaction>
--
To view, visit http://gerrit.ovirt.org/6963
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4a09ca9e66f0c9f5f4f7b283a5f43986b7e603
Gerrit-PatchSet: 15
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@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: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
