[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66581776
--- Diff:
software/winrm/src/main/java/org/apache/brooklyn/util/core/internal/winrm/WinRmTool.java
---
@@ -46,7 +46,7 @@
ConfigKey PROP_HOST = newStringConfigKey("host", "Host to
connect to (required)", null);
ConfigKey PROP_PORT = ConfigKeys.newIntegerConfigKey("port",
"WinRM port to use when connecting to the remote machine");
ConfigKey USE_HTTPS_WINRM =
ConfigKeys.newBooleanConfigKey("winrm.useHttps", "The parameter tells the
machine sensors whether the winrm port is over https. If the parameter is true
then 5986 will be used as a winrm port.", false);
-ConfigKey RETRIES_OF_NETWORK_FAILURES =
ConfigKeys.newIntegerConfigKey("retriesOfNetworkFailures", "The parameter sets
the number of retries for connection failures. If you use high value, consider
taking care for the machine's network.");
+ConfigKey RETRIES_OF_NETWORK_FAILURES =
ConfigKeys.newIntegerConfigKey("retriesOfNetworkFailures", "The parameter sets
the number of retries for connection failures. If you use high value, consider
taking care for the machine's network.", 4);
--- End diff --
For the record, it looks like this used to default to 16 in winrm4j, if no
value was passed in by Brooklyn:
https://github.com/cloudsoft/winrm4j/blob/0.3.5/client/src/main/java/io/cloudsoft/winrm4j/client/WinRmClient.java#L581
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66580748
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java
---
@@ -49,15 +50,27 @@ public void start() {
@Override
public void install() {
+// TODO: At some point in the future, this should probably be
refactored to get the name of the machine in WinRmMachineLocation and set it as
the hostname sensor
+String hostname = null;
+if
(entity.getConfig(VanillaWindowsProcess.INSTALL_REBOOT_REQUIRED)) {
+WinRmExecuteHelper checkHostnameTask = newScript("Checking
hostname")
--- End diff --
Actually AD blueprint doesn't change the Computer Name, it is changing the
`%UserDnsDomain%` and `%USERDOMAIN%` which breaks the default authentication
where we supply only Windows username without any computer name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66579605
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java
---
@@ -49,15 +50,27 @@ public void start() {
@Override
public void install() {
+// TODO: At some point in the future, this should probably be
refactored to get the name of the machine in WinRmMachineLocation and set it as
the hostname sensor
+String hostname = null;
+if
(entity.getConfig(VanillaWindowsProcess.INSTALL_REBOOT_REQUIRED)) {
+WinRmExecuteHelper checkHostnameTask = newScript("Checking
hostname")
--- End diff --
@bostko @nakomis why do we retrieve this *before* executing the install
command?
My understanding is that we added the hostname lookup to handle "Ability to
perform installation on a Windows Blueprint which changes Computer Name and
requires reboot." What will change the Computer Name? Do we expect it to be the
"install.command"? If so, surely we should retrieve the hostname *after*
executing the install command?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66246994
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java
---
@@ -49,15 +50,29 @@ public void start() {
@Override
public void install() {
+// TODO: At some point in the future, this should probably be
refactored to get the name of the machine in WinRmMachineLocation and set it as
the hostname sensor
+String hostname = null;
+if
(entity.getConfig(VanillaWindowsProcess.INSTALL_REBOOT_REQUIRED)) {
+WinRmExecuteHelper checkHostnameTask = newScript("Checking
hostname")
+.setCommand("hostname")
+.failOnNonZeroResultCode()
+.gatherOutput();
+if (checkHostnameTask.execute() != 0) {
+LOG.warn("Cannot determine hostname for entity {} machine
{}", getEntity(), getLocation());
+}
+hostname =
Strings.trimEnd(checkHostnameTask.getResultStdout());
--- End diff --
@aledsage I find it as the most reasonable way to get the actual hostname.
The `LOG.warn` statement which was there is not valid.
`.failOnNonZeroResultCode()` predicate will be used from the
WinRmExecuteHelper to throw an exception if it is not valid.
I expect that this command should always succeed when winrm-ing to a
working windows machine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66244197
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java
---
@@ -321,12 +332,21 @@ public int copyTo(InputStream source, String
destination) {
}
public void rebootAndWait() {
+rebootAndWait(null);
+}
+
+public void rebootAndWait(String hostname) {
try {
-executePsScriptNoRetry(ImmutableList.of("Restart-Computer
-Force"));
+if (hostname != null) {
+
getLocation().executePsScript(ImmutableMap.of(WinRmTool.COMPUTER_NAME,
hostname), ImmutableList.of("Restart-Computer -Force"));
+} else {
+
getLocation().executePsScript(ImmutableList.of("Restart-Computer -Force"));
+}
} catch (Exception e) {
-// Restarting the computer will cause the command to fail;
ignore the exception and continue
+LOG.warn("Exception when restarting machine: ", e);
--- End diff --
My understanding was that this exception was now *not* expected, @bostko is
that the case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66239387
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java
---
@@ -49,15 +50,29 @@ public void start() {
@Override
public void install() {
+// TODO: At some point in the future, this should probably be
refactored to get the name of the machine in WinRmMachineLocation and set it as
the hostname sensor
+String hostname = null;
+if
(entity.getConfig(VanillaWindowsProcess.INSTALL_REBOOT_REQUIRED)) {
+WinRmExecuteHelper checkHostnameTask = newScript("Checking
hostname")
+.setCommand("hostname")
+.failOnNonZeroResultCode()
+.gatherOutput();
+if (checkHostnameTask.execute() != 0) {
+LOG.warn("Cannot determine hostname for entity {} machine
{}", getEntity(), getLocation());
+}
+hostname =
Strings.trimEnd(checkHostnameTask.getResultStdout());
--- End diff --
Should we really be extracting hostname from stdout if the command failed
to execute (i.e. if we did the above log statement)? Presumably we'll get
either an empty string or some error message. What are the implications of
subsequently passing that to `rebootAndWait(hostname)`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r66239158
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java
---
@@ -321,12 +332,21 @@ public int copyTo(InputStream source, String
destination) {
}
public void rebootAndWait() {
+rebootAndWait(null);
+}
+
+public void rebootAndWait(String hostname) {
try {
-executePsScriptNoRetry(ImmutableList.of("Restart-Computer
-Force"));
+if (hostname != null) {
+
getLocation().executePsScript(ImmutableMap.of(WinRmTool.COMPUTER_NAME,
hostname), ImmutableList.of("Restart-Computer -Force"));
+} else {
+
getLocation().executePsScript(ImmutableList.of("Restart-Computer -Force"));
+}
} catch (Exception e) {
-// Restarting the computer will cause the command to fail;
ignore the exception and continue
+LOG.warn("Exception when restarting machine: ", e);
--- End diff --
I'd prefer this exception message to indicate that it is an expected
exception. If someone looks in the logs and sees this problem, it will look
like there was a problem restarting.
Should we also log at debug? Can we narrow down the kind of exception(s) we
expect? (if we're not 100% certain, that's absolutely fine to just catch
Exception).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r65900734
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java
---
@@ -49,15 +50,28 @@ public void start() {
@Override
public void install() {
+String hostname = null;
--- End diff --
At some point in the future, this should probably be refactored to get the
name of the machine in `WinRmMachineLocation` and set it as the hostname
sensor. Please add a TODO note here suggesting the same
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #150: Ability to perform installation on a Wind...
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/150#discussion_r65900017
--- Diff:
software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java
---
@@ -321,12 +332,22 @@ public int copyTo(InputStream source, String
destination) {
}
public void rebootAndWait() {
+rebootAndWait(null);
+}
+
+public void rebootAndWait(String hostname) {
try {
-executePsScriptNoRetry(ImmutableList.of("Restart-Computer
-Force"));
+if (hostname != null) {
+
getLocation().executePsScript(ImmutableMap.of(WinRmTool.COMPUTER_NAME,
hostname), ImmutableList.of("Restart-Computer -Force"));
+} else {
+
getLocation().executePsScript(ImmutableList.of("Restart-Computer -Force"));
+}
} catch (Exception e) {
// Restarting the computer will cause the command to fail;
ignore the exception and continue
--- End diff --
This comment is no longer true, and the LOG message below should be changed
to something like `LOG.warn("Exception when restarting machine: ", e);`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
