You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2015/05/19 13:28:18 UTC

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

GitHub user nakomis opened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/650

    Support for deployment to Windows

    

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/nakomis/incubator-brooklyn winrm-location

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-brooklyn/pull/650.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #650
    
----
commit ca7dd9bd56e6c4a1d8d9a7aa9f2a583179e931f0
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-06T12:13:18Z

    Basic support for Windows BYON with WinRM pre-configured

commit e19fcf57aa2b723ac455bf6b9112080035eb6a24
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-12T14:33:24Z

    Allows for lists in templateOptions

commit 987cafc523ab0d1d6b72e114e1a4bb73914a1125
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-08T14:41:14Z

    Adds WinRm support for EC2 Windows instances

commit 99ff963eaa579c6ecb649c92dcb9684767e2c98f
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-09T13:21:18Z

    Adds copyTo functions to WinRmMachineLocation

commit e2a63aa857086c1cf8c95affddfc938f194ef53e
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-13T09:48:58Z

    Adds (WIP) outline for VanillaWindowsProcess

commit 7242292017a69ea013e9765668ea0b1e08498506
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-13T12:14:47Z

    Avoids unnecessary Arrays.copyOf in WinRmMachineLocation.copyTo

commit 3a31a5699b217818d216e8b21f900e84ff1017c8
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-15T15:12:44Z

    Implements support for VanillaWindowsProcess

commit 53191e774ac2bce00498c5726317191bb3e41eaf
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-15T15:13:53Z

    Adds support for ${attribute['attribute.name']} sensor access in freemarker templates

commit 0a2aaa08a86ed94c82722698482ef460629949ef
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-22T12:23:18Z

    Removes default launch command from VanillaWindowsProcess, adds ExecutionPolicy to default WinRm usermetadata

commit 0ff9268a44ffd7d1980995dedbfb8e8a8a2c2c8f
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-27T05:06:10Z

    Registers location in vmInstanceIds to allow it to be destroyed after use

commit f9af686856caa4bf3f85892b96b6308299859423
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-27T17:26:56Z

    Fixes destruction of WinRmMachineLocation on stop

commit 630151e18250f02b0e982f41e5947110ded6a7cf
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-04-28T09:57:07Z

    Adds apache header to StaticSensor

commit 02e93cf9df54d8559612f1497dfe4104f9df3045
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-01T12:20:53Z

    Adds retry to WinRm commands

commit 5f24e84e6dc0a7f769b7e8c1a0f33fd3b42ba103
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-06T13:18:13Z

    Fixes broken cluster location logic

commit 1b88098aab8d8154443fb94752ffbeb6d8d0f2ca
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-12T14:56:09Z

    Adds support for getting return from WinRmMachineLocation execute commands

commit e77edd23b9e9c4f21b51b3cccca249e891b04ea1
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-13T07:58:30Z

    Adds performance counters

commit 40a18277b6b36b3a99bc1cedde038450efa6dedf
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-14T12:06:25Z

    Adds latches for pre-install files and pre-install/install/customize reboots

commit a228674a94ad4e824c5d3c78cb36d93ec13b63dd
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-14T12:09:35Z

    Changes DynamicCluster resize behaviour to favour the location defined on the memberspec (if present)

commit 31bef105d21e0da2251bf064f54cdd9d6245c992
Author: Martin Harris <gi...@nakomis.com>
Date:   2015-05-19T11:26:21Z

    Fixes JcloudsLocationTest.testInvokesCustomizerCallbacks

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30894902
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
    @@ -442,11 +442,12 @@ protected final void callStartHooks() {}
          * plus any ports defined with a config keys ending in .port 
          */
         protected Collection<Integer> getRequiredOpenPorts() {
    -        Set<Integer> ports = MutableSet.of(22);
    +        // TODO: Should only open 22 *or* 5985. Perhaps a flag / ConfigKey on SoftwareProcessImpl?
    +        Set<Integer> ports = MutableSet.of(22, 5985, 3389);
    --- End diff --
    
    3389 is RDP; 5985 is WinRM
    3389 isn't used by Brooklyn, but useful for the end-user subsequently.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30726697
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/VanillaWindowsProcess.java ---
    @@ -0,0 +1,54 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.proxying.ImplementedBy;
    +import brooklyn.util.time.Duration;
    +
    +@ImplementedBy(VanillaWindowsProcessImpl.class)
    +public interface VanillaWindowsProcess extends AbstractVanillaProcess {
    +    ConfigKey<String> PRE_INSTALL_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("pre.install.powershell.command",
    +            "powershell command to run during the pre-install phase");
    +    ConfigKey<Boolean> PRE_INSTALL_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("pre.install.reboot.required",
    +            "indicates that a reboot should be performed after the pre-install command is run", false);
    +    ConfigKey<Boolean> INSTALL_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("install.reboot.required",
    +            "indicates that a reboot should be performed after the install command is run", false);
    +    ConfigKey<Boolean> CUSTOMIZE_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("customize.reboot.required",
    +            "indicates that a reboot should be performed after the customize command is run", false);
    +    ConfigKey<String> LAUNCH_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("launch.powershell.command",
    +            "command to run to launch the process");
    +    ConfigKey<String> CHECK_RUNNING_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("checkRunning.powershell.command",
    +            "command to determine whether the process is running");
    +    ConfigKey<String> STOP_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("stop.powershell.command",
    +            "command to run to stop the process");
    +    ConfigKey<String> CUSTOMIZE_COMMAND = ConfigKeys.newStringConfigKey("customize.command",
    +            "command to run during the customization phase");
    +    ConfigKey<String> CUSTOMIZE_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("customize.powershell.command",
    +            "powershell command to run during the customization phase");
    +    ConfigKey<String> INSTALL_COMMAND = ConfigKeys.newStringConfigKey("install.command",
    +            "command to run during the install phase");
    +    ConfigKey<String> INSTALL_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("install.powershell.command",
    +            "powershell command to run during the install phase");
    +    ConfigKey<Duration> REBOOT_UNAVAILABLE_TIMEOUT = ConfigKeys.newDurationConfigKey("reboot.unavailable.timeout",
    --- End diff --
    
    Description is also the same for both.
    
    What does this do? Is there a better name than "unavailable"? e.g. is one a `REBOOT_BEGUN_TIMEOUT`, and the other a `REBOOT_COMPLETED_TIMEOUT`?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#issuecomment-103970506
  
    Build failure was because it couldn't find `io.cloudsoft.windows:winrm4j:jar:0.1.0-SNAPSHOT`.
    
    However, version 0.1.0 is now released to maven central: http://search.maven.org/#search%7Cga%7C1%7Cio.cloudsoft.windows%20winrm4j


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30687294
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -836,6 +910,52 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
             }
         }
     
    +    private void waitForWinRmAvailable(final NodeMetadata node, final LoginCredentials expectedCredentials, ConfigBag setup) {
    +        // TODO: Reduce / remove duplication between this and waitForReachable
    +        String waitForWinRmAvailable = setup.get(WAIT_FOR_WINRM_AVAILABLE);
    --- End diff --
    
    Perhaps worth merging the `WAIT_FOR_WINRM_AVAILABLE` and the `WAIT_FOR_SSHABLE`. They are conceptually the same thing. Would probably have to deprecate the old key, and generalise the 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616222
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    --- End diff --
    
    Why does this return null? Same for `getPrivateAddresses` below?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30686944
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -836,6 +910,52 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
             }
         }
     
    +    private void waitForWinRmAvailable(final NodeMetadata node, final LoginCredentials expectedCredentials, ConfigBag setup) {
    +        // TODO: Reduce / remove duplication between this and waitForReachable
    +        String waitForWinRmAvailable = setup.get(WAIT_FOR_WINRM_AVAILABLE);
    +        checkArgument(!"false".equalsIgnoreCase(waitForWinRmAvailable), "waitForWinRmAvailable called despite waitForWinRmAvailable=%s", waitForWinRmAvailable);
    +
    +        long delayMs = -1;
    +        try {
    +            delayMs = Time.parseTimeString(""+waitForWinRmAvailable);
    +        } catch (Exception e) {
    +            // normal if 'true'; just fall back to default
    +        }
    +        if (delayMs<0)
    +            delayMs = Time.parseTimeString(WAIT_FOR_WINRM_AVAILABLE.getDefaultValue());
    +
    +        // FIXME: remove this
    --- End diff --
    
    Why a FIXME about removing this? What needs removed? Why not just remove it?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30709588
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/AbstractSoftwareProcessWinRmDriver.java ---
    @@ -0,0 +1,192 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import java.io.File;
    +import java.io.InputStream;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.Callable;
    +import java.util.concurrent.TimeUnit;
    +
    +import org.python.core.PyException;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.event.AttributeSensor;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.location.basic.WinRmMachineLocation;
    +import brooklyn.util.exceptions.ReferenceWithError;
    +import brooklyn.util.repeat.Repeater;
    +import brooklyn.util.time.Duration;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.api.client.util.Strings;
    +import com.google.common.collect.ImmutableList;
    +
    +public abstract class AbstractSoftwareProcessWinRmDriver extends AbstractSoftwareProcessDriver {
    +
    +    AttributeSensor<String> WINDOWS_USERNAME = Sensors.newStringSensor("windows.username",
    +            "Default Windows username to be used when connecting to the Entity's VM");
    +    AttributeSensor<String> WINDOWS_PASSWORD = Sensors.newStringSensor("windows.password",
    +            "Default Windows password to be used when connecting to the Entity's VM");
    +
    +    public AbstractSoftwareProcessWinRmDriver(EntityLocal entity, WinRmMachineLocation location) {
    +        super(entity, location);
    +        entity.setAttribute(WINDOWS_USERNAME, location.config().get(WinRmMachineLocation.WINDOWS_USERNAME));
    +        entity.setAttribute(WINDOWS_PASSWORD, location.config().get(WinRmMachineLocation.WINDOWS_PASSWORD));
    +    }
    +
    +    @Override
    +    public void runPreInstallCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void setup() {
    +        // Default to no-op
    +    }
    +
    +    @Override
    +    public void runPostInstallCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void runPreLaunchCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void runPostLaunchCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public WinRmMachineLocation getLocation() {
    +        return (WinRmMachineLocation)super.getLocation();
    +    }
    +
    +    @Override
    +    public String getRunDir() {
    +        // TODO: This needs to be tidied, and read from the appropriate flags (if set)
    +        return "$HOME\\brooklyn-managed-processes\\apps\\" + entity.getApplicationId() + "\\entities\\" + getEntityVersionLabel()+"_"+entity.getId();
    +    }
    +
    +    @Override
    +    public String getInstallDir() {
    +        // TODO: This needs to be tidied, and read from the appropriate flags (if set)
    +        return "$HOME\\brooklyn-managed-processes\\installs\\" + entity.getApplicationId() + "\\" + getEntityVersionLabel()+"_"+entity.getId();
    +    }
    +
    +    @Override
    +    public int copyResource(Map<Object, Object> sshFlags, String source, String target, boolean createParentDir) {
    +        if (createParentDir) {
    +            createDirectory(getDirectory(target), "Creating resource directory");
    +        }
    +        return copyTo(new File(source), new File(target));
    +    }
    +
    +    @Override
    +    public int copyResource(Map<Object, Object> sshFlags, InputStream source, String target, boolean createParentDir) {
    +        if (createParentDir) {
    +            createDirectory(getDirectory(target), "Creating resource directory");
    +        }
    +        return copyTo(source, new File(target));
    +    }
    +
    +    @Override
    +    protected void createDirectory(String directoryName, String summaryForLogging) {
    +        getLocation().executePsScript("New-Item -path \"" + directoryName + "\" -type directory -ErrorAction SilentlyContinue");
    +    }
    +
    +    protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
    +        String regularCommand = getEntity().getConfig(regularCommandKey);
    +        String powershellCommand = getEntity().getConfig(powershellCommandKey);
    +        if (Strings.isNullOrEmpty(regularCommand) && Strings.isNullOrEmpty(powershellCommand)) {
    +            if (allowNoOp) {
    +                return new WinRmToolResponse("", "", 0);
    +            } else {
    +                throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
    +            }
    +        } else if (!Strings.isNullOrEmpty(regularCommand) && !Strings.isNullOrEmpty(powershellCommand)) {
    +            throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
    +        }
    +
    +        if (Strings.isNullOrEmpty(regularCommand)) {
    +            return getLocation().executePsScript(ImmutableList.of(powershellCommand));
    +        } else {
    +            return getLocation().executeScript(ImmutableList.of(regularCommand));
    +        }
    +    }
    +
    +    public int execute(List<String> script) {
    +        return getLocation().executeScript(script).getStatusCode();
    +    }
    +
    +    public int executePowerShellNoRetry(List<String> psScript) {
    +        return getLocation().executePsScriptNoRetry(psScript).getStatusCode();
    +    }
    +
    +    public int executePowerShell(List<String> psScript) {
    +        return getLocation().executePsScript(psScript).getStatusCode();
    +    }
    +
    +    public int copyTo(File source, File destination) {
    --- End diff --
    
    As commented in WinRmMachineLocation.java, not sure should use `File destination`, rather than `String destination`, because the OS of the brooklyn server and target VM may well be different. Thoughts?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30726054
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
    @@ -442,11 +442,12 @@ protected final void callStartHooks() {}
          * plus any ports defined with a config keys ending in .port 
          */
         protected Collection<Integer> getRequiredOpenPorts() {
    -        Set<Integer> ports = MutableSet.of(22);
    +        // TODO: Should only open 22 *or* 5985. Perhaps a flag / ConfigKey on SoftwareProcessImpl?
    +        Set<Integer> ports = MutableSet.of(22, 5985, 3389);
    --- End diff --
    
    Hm, tricky to figure out whether it's going to be Windows or Linux from the location. I think the entity impl needs to tell us (e.g. with a method that is overridden by the windows sub-class(es)).
    
    Can you at least add to the TODO what 5985 and 3389 are for.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615433
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -251,10 +258,71 @@ public T call() throws Exception {
             }
         }
     
    +    private static class SendPerfCountersToSensors implements PollHandler<WinRmToolResponse> {
    +
    +        private final EntityLocal entity;
    +        private final List<WindowsPerformanceCounterPollConfig<?>> polls;
    +
    +        public SendPerfCountersToSensors(EntityLocal entity, List<WindowsPerformanceCounterPollConfig<?>> polls) {
    +            this.entity = entity;
    +            this.polls = polls;
    +        }
    +
    +        @Override
    +        public boolean checkSuccess(WinRmToolResponse val) {
    +            if (val.getStatusCode() != 0) return false;
    +            String stderr = val.getStdErr();
    +            if (stderr == null || stderr.length() != 0) return false;
    --- End diff --
    
    Can we not just rely on the statusCode?
    Worth a TODO to explain why treating presence of stderr as a failure.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30683449
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -622,6 +624,14 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                 if (node == null)
                     throw new IllegalStateException("No nodes returned by jclouds create-nodes in " + setup.getDescription());
     
    +            boolean windows = node.getOperatingSystem().getFamily().equals(OsFamily.WINDOWS);
    +
    +            if (windows) {
    +                // FIXME: Tidy this up and allow for user-specified credentials
    +                setup.put(USER, node.getCredentials().getUser());
    +                setup.put(PASSWORD, node.getCredentials().getOptionalPassword().orNull());
    +            }
    +
                 // Setup port-forwarding, if required
                 Optional<HostAndPort> sshHostAndPortOverride;
    --- End diff --
    
    We should generalise `sshHostAndPortOverride`, perhaps to `connectionHostAndPortOverride`. And we should ensure it is setup for WinRM port in the windows case.
    
    Can do that in separate PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30655542
  
    --- Diff: core/src/main/java/brooklyn/util/flags/TypeCoercions.java ---
    @@ -709,6 +710,13 @@ public QuorumCheck apply(final String input) {
                     return QuorumChecks.of(input);
                 }
             });
    +        registerAdapter(ArrayList.class, String[].class, new Function<ArrayList, String[]>() {
    --- End diff --
    
    Could we declare this as `List.class`, rather than `ArrayList.class`? Or does it need to be the concrete type?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30709010
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/AbstractSoftwareProcessDriver.java ---
    @@ -251,6 +264,183 @@ public InputStream getResource(String url) {
             return resource.getResourceFromUrl(url);
         }
     
    +    /**
    +     * Files and templates to be copied to the server <em>before</em> pre-install. This allows the {@link #preInstall()}
    +     * process to have access to all required resources.
    +     * <p>
    +     * Will be prefixed with the entity's {@link #getInstallDir() install directory} if relative.
    +     *
    +     * @see SoftwareProcess#PRE_INSTALL_FILES
    +     * @see SoftwareProcess#PRE_INSTALL_TEMPLATES
    +     * @see #copyRuntimeResources()
    +     */
    +    public void copyPreInstallResources() {
    --- End diff --
    
    I presume these were copied unmodified from AbstractSoftwareProcessSshDriver? Therefore not reviewing thoroughly. If anything (beyond calling abstract methods at the appropriate points) has changed, then let me know and I'll review more thoroughly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30894302
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -251,10 +258,71 @@ public T call() throws Exception {
             }
         }
     
    +    private static class SendPerfCountersToSensors implements PollHandler<WinRmToolResponse> {
    +
    +        private final EntityLocal entity;
    +        private final List<WindowsPerformanceCounterPollConfig<?>> polls;
    +
    +        public SendPerfCountersToSensors(EntityLocal entity, List<WindowsPerformanceCounterPollConfig<?>> polls) {
    +            this.entity = entity;
    +            this.polls = polls;
    +        }
    +
    +        @Override
    +        public boolean checkSuccess(WinRmToolResponse val) {
    +            if (val.getStatusCode() != 0) return false;
    +            String stderr = val.getStdErr();
    +            if (stderr == null || stderr.length() != 0) return false;
    --- End diff --
    
    Status code is unreliable (it seems, empirically) - returns 0 sometimes even when failed (but never returns non-zero on success; but if the connection drops, then we'll get an exception though the command might continue executing on the Windows machine to completion).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30614352
  
    --- Diff: pom.xml ---
    @@ -205,6 +205,7 @@
             <jsr311-api.version>1.1.1</jsr311-api.version>
             <maxmind.version>0.8.1</maxmind.version>
             <jna.version>4.0.0</jna.version>
    +        <winrm4j.version>0.1.0-SNAPSHOT</winrm4j.version>
    --- End diff --
    
    Use 0.1.0, which is in the process of being released to maven central.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30684054
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -712,80 +737,105 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                     List<String> setupScripts = setup.get(JcloudsLocationConfig.CUSTOM_MACHINE_SETUP_SCRIPT_URL_LIST);
                     Collection<String> allScripts = new MutableList<String>().appendIfNotNull(setupScript).appendAll(setupScripts);
                     for (String setupScriptItem : allScripts) {
    -                    if (Strings.isNonBlank(setupScriptItem)) {
    -                        customisationForLogging.add("custom setup script "+setupScriptItem);
    +                    if (Strings.isNonBlank(setupScript)) {
    +                        customisationForLogging.add("custom setup script " + setupScript);
     
                             String setupVarsString = setup.get(JcloudsLocationConfig.CUSTOM_MACHINE_SETUP_SCRIPT_VARS);
                             Map<String, String> substitutions = (setupVarsString != null)
                                     ? Splitter.on(",").withKeyValueSeparator(":").split(setupVarsString)
                                     : ImmutableMap.<String, String>of();
    -                        String scriptContent =  ResourceUtils.create(this).getResourceAsString(setupScriptItem);
    +                        String scriptContent = ResourceUtils.create(this).getResourceAsString(setupScriptItem);
                             String script = TemplateProcessor.processTemplateContents(scriptContent, getManagementContext(), substitutions);
    -                        sshMachineLocation.execCommands("Customizing node " + this + " with script " + setupScriptItem, ImmutableList.of(script));
    +                        if (windows) {
    +                            winRmMachineLocation.executeScript(ImmutableList.copyOf((script.replace("\r", "").split("\n"))));
    +                        } else {
    +                            jcloudsSshMachineLocation.execCommands("Customizing node " + this, ImmutableList.of(script));
    +                        }
                         }
                     }
     
                     if (setup.get(JcloudsLocationConfig.MAP_DEV_RANDOM_TO_DEV_URANDOM)) {
    -                    customisationForLogging.add("point /dev/random to urandom");
    +                    if (windows) {
    +                        LOG.warn("Ignoring flag MAP_DEV_RANDOM_TO_DEV_URANDOM on Windows location {}", winRmMachineLocation);
    +                    } else {
    +                        customisationForLogging.add("point /dev/random to urandom");
     
    -                    sshMachineLocation.execCommands("using urandom instead of random",
    -                            Arrays.asList("sudo mv /dev/random /dev/random-real", "sudo ln -s /dev/urandom /dev/random"));
    +                        jcloudsSshMachineLocation.execCommands("using urandom instead of random",
    +                                Arrays.asList("sudo mv /dev/random /dev/random-real", "sudo ln -s /dev/urandom /dev/random"));
    +                    }
                     }
     
     
                     if (setup.get(GENERATE_HOSTNAME)) {
    -                    customisationForLogging.add("configure hostname");
    +                    if (windows) {
    +                        // TODO: Generate Windows Hostname
    +                        LOG.warn("Ignoring flag GENERATE_HOSTNAME on Windows location {}", winRmMachineLocation);
    --- End diff --
    
    We should list (in docs / limitations) the main config options that are not supported (yet) on windows (rather than the user/customer discovering it by trying to configure it and eventually finding this log message, or just it being ignored silently (as would be the case for user/password configuration).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#issuecomment-103970114
  
    Thanks @nakomis !
    
    I've finished reading through the code. A lot of comments, mostly minor. Quite a few things can be deferred to future PRs I think.
    
    Could do with some more tests wherever possible as well (again another PR could potentially do that, as long as we're confident that PR will follow soon after!)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30679752
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -622,6 +624,14 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                 if (node == null)
                     throw new IllegalStateException("No nodes returned by jclouds create-nodes in " + setup.getDescription());
     
    +            boolean windows = node.getOperatingSystem().getFamily().equals(OsFamily.WINDOWS);
    +
    +            if (windows) {
    +                // FIXME: Tidy this up and allow for user-specified credentials
    --- End diff --
    
    Am I reading this right, that we currently don't support creating a new user or support setting the existing (e.g. administrator) password?
    
    If so, we should include that in the list of things to do for being ready for production.
    
    How does the Brooklyn user discover the user/password of their VM for if they want to subsequently log in? I presume that won't be displayed in the Brooklyn web console at all currently?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615235
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -179,50 +186,50 @@ protected WindowsPerformanceCounterFeed(Builder builder) {
     
         @Override
         protected void preStart() {
    -        Set<WindowsPerformanceCounterPollConfig<?>> polls = getConfig(POLLS);
    +        List<WindowsPerformanceCounterPollConfig<?>> polls = getConfig(POLLS);
             
             long minPeriod = Integer.MAX_VALUE;
    -        SortedSet<String> performanceCounterNames = Sets.newTreeSet();
    +        List<String> performanceCounterNames = Lists.newArrayList();
             for (WindowsPerformanceCounterPollConfig<?> config : polls) {
                 minPeriod = Math.min(minPeriod, config.getPeriod());
                 performanceCounterNames.add(config.getPerformanceCounterName());
             }
             
    -        SshMachineLocation machine = EffectorTasks.getSshMachine(getEntity());
             Iterable<String> allParams = ImmutableList.<String>builder()
    -                .add("typeperf")
    -                .addAll(Iterables.transform(performanceCounterNames, QuoteStringFunction.INSTANCE))
    -                .add("-sc")
    -                .add("1")
    +                .add("Get-Counter")
    +                .add("-Counter")
    +                .add(JOINER_ON_COMMA.join(Iterables.transform(performanceCounterNames, QuoteStringFunction.INSTANCE)))
    +                .add("-SampleInterval")
    +                .add("2") // TODO: This should be a config key
    --- End diff --
    
    What does the "2" mean? Can you add some more description to the TODO


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r31204391
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/VanillaWindowsProcessImpl.java ---
    @@ -0,0 +1,39 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +public class VanillaWindowsProcessImpl extends SoftwareProcessImpl implements VanillaWindowsProcess {
    +    @Override
    +    public Class getDriverInterface() {
    --- End diff --
    
    instead of creating an entire new entity that duplicated the VanillaProcessImpl, wouldn't be possibile to load the appropriate driver for the type of location where the entity will be installed? ie SshDriver vs WinRmDriver etc?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30684201
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -795,24 +845,48 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
     
                 // Apply any optional app-specific customization.
                 for (JcloudsLocationCustomizer customizer : getCustomizers(setup)) {
    -                customizer.customize(this, computeService, sshMachineLocation);
    +                if (windows) {
    +                    LOG.warn("Ignoring customizer {} on Windows location {}", customizer, winRmMachineLocation);
    --- End diff --
    
    Tricky; we should update the customizer's interface to take a MachineLocation instead of an ssh machine location.
    
    Let's do that in a separate PR.
    
    Can you keep a list of what we're deferring to later PRs please.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by richardcloudsoft <gi...@git.apache.org>.
Github user richardcloudsoft commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r31115419
  
    --- Diff: docs/guide/yaml/winrm/re-authentication.md ---
    @@ -0,0 +1,38 @@
    +---
    +title: Re-authenticating within a powershell script
    +title_in_menu: Re-authentication
    +layout: website-normal
    +---
    +
    +## How and Why to re-authenticate withing a powershell script
    +
    +Brooklyn will run powershell scripts by making a WinRM call over HTTP. For most scripts this will work, however for
    +some scripts (e.g. MSSQL installation), this will fail even if the script can be run locally (e.g. by using RDP to
    +connect to the machine and running the script manually)
    +
    +In the case of MS SQL server installation, there was no clear indication of why this would not work. The only clue was
    +a security exception in the installation log
    +
    +It appears that when a script is run over WinRM over HTTP, the credentials under which the script are run are marked as
    +'remote' credentials, which are prohibited from running certain security-related operations. The solution was to obtain
    +a new set of credentials within the script and use those credentials to exeute the installer, so this:
    +
    +```
    +( $driveLetter + "setup.exe") /ConfigurationFile=C:\ConfigurationFile.ini
    +```
    +
    +became this:
    +
    +$pass = '${attribute['windows.password']}'
    --- End diff --
    
    Fence with ``` otherwise the formatting is mangled.
    
    (In fact, fence with ```powershell to get syntax highlighting)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30687013
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -836,6 +910,52 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
             }
         }
     
    +    private void waitForWinRmAvailable(final NodeMetadata node, final LoginCredentials expectedCredentials, ConfigBag setup) {
    +        // TODO: Reduce / remove duplication between this and waitForReachable
    +        String waitForWinRmAvailable = setup.get(WAIT_FOR_WINRM_AVAILABLE);
    +        checkArgument(!"false".equalsIgnoreCase(waitForWinRmAvailable), "waitForWinRmAvailable called despite waitForWinRmAvailable=%s", waitForWinRmAvailable);
    +
    +        long delayMs = -1;
    +        try {
    +            delayMs = Time.parseTimeString(""+waitForWinRmAvailable);
    +        } catch (Exception e) {
    +            // normal if 'true'; just fall back to default
    +        }
    +        if (delayMs<0)
    +            delayMs = Time.parseTimeString(WAIT_FOR_WINRM_AVAILABLE.getDefaultValue());
    +
    +        // FIXME: remove this
    +        LOG.info("Address: " + node.getPublicAddresses().iterator().next());
    +        LOG.info("User: " + expectedCredentials.getUser());
    +        LOG.info("Password: " + expectedCredentials.getOptionalPassword().get());
    +        final Session session = WinRMFactory.INSTANCE.createSession(node.getPublicAddresses().iterator().next(),
    +                expectedCredentials.getUser(), expectedCredentials.getOptionalPassword().get());
    --- End diff --
    
    Previously you have used `getOptionalPassword().orNull()` - can we assume it will exist here?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30727148
  
    --- Diff: software/base/src/main/java/brooklyn/entity/software/StaticSensor.java ---
    @@ -0,0 +1,44 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.software;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.basic.EntityLocal;
    +import brooklyn.entity.effector.AddSensor;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.util.config.ConfigBag;
    +
    +public class StaticSensor<T> extends AddSensor<Integer> {
    +
    +    public static final ConfigKey<Integer> STATIC_VALUE = ConfigKeys.newConfigKey(Integer.class, "static.value");
    +
    +    private final Integer value;
    --- End diff --
    
    Can we generalise this so it is for more than just integer?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616595
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +                WinRmToolResponse response = winRmTool.executeScript(script);
    +                return response;
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying:", e);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute shell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScript(String psScript) {
    +        return executePsScript(ImmutableList.of(psScript));
    +    }
    +
    +    public WinRmToolResponse executePsScript(List<String> psScript) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                return executePsScriptNoRetry(psScript);
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying after 5 seconds:", e);
    +                Time.sleep(Duration.FIVE_SECONDS);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute powershell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScriptNoRetry(List<String> psScript) {
    --- End diff --
    
    I'd have expected the same pattern of methods to be used for executePsScript and executeScript. Is there a reason not to?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615039
  
    --- Diff: core/src/main/java/brooklyn/entity/group/DynamicClusterImpl.java ---
    @@ -588,16 +588,22 @@ protected Entity replaceMember(Entity member, Location memberLoc, Map<?, ?> extr
     
             // choose locations to be deployed to
             List<Location> chosenLocations;
    -        if (isAvailabilityZoneEnabled()) {
    -            List<Location> subLocations = getNonFailedSubLocations();
    -            Multimap<Location, Entity> membersByLocation = getMembersByLocation();
    -            chosenLocations = getZonePlacementStrategy().locationsForAdditions(membersByLocation, subLocations, delta);
    -            if (chosenLocations.size() != delta) {
    -                throw new IllegalStateException("Node placement strategy chose " + Iterables.size(chosenLocations)
    -                        + ", when expected delta " + delta + " in " + this);
    +        chosenLocations = getMemberSpec() == null ? null : getMemberSpec().getLocations();
    --- End diff --
    
    We need to think about this more. Feels wrong to disable the availability zone behaviour entirely.
    
    Similarly would prefer us not to merge with `FIXME: Tidy this up!` in the code. That at least should be fleshed out to a TODO that describes how it could be improved. Or ideally improve the code.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30655960
  
    --- Diff: core/src/test/java/brooklyn/location/basic/ByonLocationResolverTest.java ---
    @@ -307,12 +308,41 @@ public void testEmptySpec() throws Exception {
                     "name", "foo",
                     "user", "myuser"
             );
    -        MachineProvisioningLocation<SshMachineLocation> provisioner = resolve(spec, flags);
    -        SshMachineLocation location1 = provisioner.obtain(ImmutableMap.of());
    +        MachineProvisioningLocation<MachineLocation> provisioner = resolve(spec, flags);
    +        SshMachineLocation location1 = (SshMachineLocation)provisioner.obtain(ImmutableMap.of());
             Assert.assertEquals("myuser", location1.getUser());
             Assert.assertEquals("1.1.1.1", location1.getAddress().getHostAddress());
         }
     
    +    @Test
    +    public void testWindowsMachines() throws Exception {
    +        brooklynProperties.put("brooklyn.location.byon.windows.username", "myuser");
    +        brooklynProperties.put("brooklyn.location.byon.windows.password", "mypassword");
    +        String spec = "byon";
    +        Map<String, ?> flags = ImmutableMap.of(
    +                "hosts", ImmutableList.of("1.1.1.1", "2.2.2.2"),
    +                "osfamily", "windows"
    +        );
    +        MachineProvisioningLocation<MachineLocation> provisioner = resolve(spec, flags);
    +        MachineLocation location = provisioner.obtain(ImmutableMap.of());
    +
    +        assertEquals(location.config().get(WinRmMachineLocation.WINDOWS_USERNAME), "myuser");
    +        assertEquals(location.config().get(WinRmMachineLocation.WINDOWS_PASSWORD), "mypassword");
    +        Assert.assertNotNull(provisioner);
    --- End diff --
    
    why assertNotNull here? We've already called methods on it. Suggest delete this line?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30726190
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/AbstractVanillaProcessDriver.java ---
    @@ -0,0 +1,22 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +public interface AbstractVanillaProcessDriver extends SoftwareProcessDriver {
    --- End diff --
    
    What does this interface buy us? Or is it just a marker interface?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30727316
  
    --- Diff: software/base/src/main/java/brooklyn/entity/software/winrm/WindowsPerformanceCounterSensors.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.software.winrm;
    +
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.basic.EntityInternal;
    +import brooklyn.entity.basic.EntityLocal;
    +import brooklyn.entity.proxying.EntityInitializer;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.event.feed.windows.WindowsPerformanceCounterFeed;
    +import brooklyn.util.config.ConfigBag;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.reflect.TypeToken;
    +
    +public class WindowsPerformanceCounterSensors implements EntityInitializer {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WindowsPerformanceCounterSensors.class);
    +
    +    public final static ConfigKey<Set<Map<String, String>>> PERFORMANCE_COUNTERS = ConfigKeys.newConfigKey(new TypeToken<Set<Map<String, String>>>(){}, "performance.counters");
    +
    +    protected final Set<Map<String, String>> sensors;
    +
    +    public WindowsPerformanceCounterSensors(ConfigBag params) {
    +        sensors = params.get(PERFORMANCE_COUNTERS);
    +    }
    +
    +    public WindowsPerformanceCounterSensors(Map<String, String> params) {
    +        this(ConfigBag.newInstance(params));
    +    }
    +
    +    @Override
    +    public void apply(EntityLocal entity) {
    +        WindowsPerformanceCounterFeed.Builder builder = WindowsPerformanceCounterFeed.builder()
    +                .entity(entity);
    +        for (Map<String, String> sensorConfig : sensors) {
    +            String sensorType = sensorConfig.get("sensorType");
    +            Class<?> clazz;
    +            try {
    +                clazz = Strings.isNonEmpty(sensorType) ?
    +                ((EntityInternal)entity).getManagementContext().getCatalog().getRootClassLoader().loadClass(sensorType) : String.class;
    +            } catch (ClassNotFoundException e) {
    +                LOG.warn("Could not load type {} for sensor {}", sensorType, sensorConfig.get("name"));
    +                clazz = String.class;
    --- End diff --
    
    Do we want to fall back to `String`, or do we want to fail?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616169
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    --- End diff --
    
    Let's use config keys rather than `@SetFromFlag` anywhere here. The old SetFromFlag approach in SshMachineLocation should really be refactored.
    
    Same goes for `address` below.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#issuecomment-106107390
  
    @nakomis please close this PR, in favour of https://github.com/apache/incubator-brooklyn/pull/664


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30894172
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -179,50 +186,50 @@ protected WindowsPerformanceCounterFeed(Builder builder) {
     
         @Override
         protected void preStart() {
    -        Set<WindowsPerformanceCounterPollConfig<?>> polls = getConfig(POLLS);
    +        List<WindowsPerformanceCounterPollConfig<?>> polls = getConfig(POLLS);
             
             long minPeriod = Integer.MAX_VALUE;
    -        SortedSet<String> performanceCounterNames = Sets.newTreeSet();
    +        List<String> performanceCounterNames = Lists.newArrayList();
             for (WindowsPerformanceCounterPollConfig<?> config : polls) {
                 minPeriod = Math.min(minPeriod, config.getPeriod());
                 performanceCounterNames.add(config.getPerformanceCounterName());
             }
             
    -        SshMachineLocation machine = EffectorTasks.getSshMachine(getEntity());
             Iterable<String> allParams = ImmutableList.<String>builder()
    -                .add("typeperf")
    -                .addAll(Iterables.transform(performanceCounterNames, QuoteStringFunction.INSTANCE))
    -                .add("-sc")
    -                .add("1")
    +                .add("Get-Counter")
    +                .add("-Counter")
    +                .add(JOINER_ON_COMMA.join(Iterables.transform(performanceCounterNames, QuoteStringFunction.INSTANCE)))
    +                .add("-SampleInterval")
    +                .add("2") // TODO: This should be a config key
    --- End diff --
    
    Means poll every 2 seconds


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30726553
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/VanillaWindowsProcess.java ---
    @@ -0,0 +1,54 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.proxying.ImplementedBy;
    +import brooklyn.util.time.Duration;
    +
    +@ImplementedBy(VanillaWindowsProcessImpl.class)
    +public interface VanillaWindowsProcess extends AbstractVanillaProcess {
    +    ConfigKey<String> PRE_INSTALL_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("pre.install.powershell.command",
    +            "powershell command to run during the pre-install phase");
    +    ConfigKey<Boolean> PRE_INSTALL_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("pre.install.reboot.required",
    +            "indicates that a reboot should be performed after the pre-install command is run", false);
    +    ConfigKey<Boolean> INSTALL_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("install.reboot.required",
    +            "indicates that a reboot should be performed after the install command is run", false);
    +    ConfigKey<Boolean> CUSTOMIZE_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("customize.reboot.required",
    +            "indicates that a reboot should be performed after the customize command is run", false);
    +    ConfigKey<String> LAUNCH_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("launch.powershell.command",
    +            "command to run to launch the process");
    +    ConfigKey<String> CHECK_RUNNING_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("checkRunning.powershell.command",
    +            "command to determine whether the process is running");
    +    ConfigKey<String> STOP_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("stop.powershell.command",
    +            "command to run to stop the process");
    +    ConfigKey<String> CUSTOMIZE_COMMAND = ConfigKeys.newStringConfigKey("customize.command",
    +            "command to run during the customization phase");
    +    ConfigKey<String> CUSTOMIZE_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("customize.powershell.command",
    +            "powershell command to run during the customization phase");
    +    ConfigKey<String> INSTALL_COMMAND = ConfigKeys.newStringConfigKey("install.command",
    +            "command to run during the install phase");
    +    ConfigKey<String> INSTALL_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("install.powershell.command",
    +            "powershell command to run during the install phase");
    +    ConfigKey<Duration> REBOOT_UNAVAILABLE_TIMEOUT = ConfigKeys.newDurationConfigKey("reboot.unavailable.timeout",
    --- End diff --
    
    Same name for config key in REBOOT_UNAVAILABLE_TIMEOUT and REBOOT_AVAILABLE_TIMEOUT (always look out for that - consequences are really bad! it ends up treating both keys as the same thing, which is very hard to debug!)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615834
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -251,10 +258,71 @@ public T call() throws Exception {
             }
         }
     
    +    private static class SendPerfCountersToSensors implements PollHandler<WinRmToolResponse> {
    +
    +        private final EntityLocal entity;
    +        private final List<WindowsPerformanceCounterPollConfig<?>> polls;
    +
    +        public SendPerfCountersToSensors(EntityLocal entity, List<WindowsPerformanceCounterPollConfig<?>> polls) {
    +            this.entity = entity;
    +            this.polls = polls;
    +        }
    +
    +        @Override
    +        public boolean checkSuccess(WinRmToolResponse val) {
    +            if (val.getStatusCode() != 0) return false;
    +            String stderr = val.getStdErr();
    +            if (stderr == null || stderr.length() != 0) return false;
    +            String out = val.getStdOut();
    +            if (out == null || out.length() == 0) return false;
    +            return true;
    +        }
    +
    +        @Override
    +        public void onSuccess(WinRmToolResponse val) {
    +            String[] values = val.getStdOut().split("\r\n");
    +            for (int i = 0; i < polls.size(); i++) {
    +                Class<?> clazz = polls.get(i).getSensor().getType();
    +                Maybe<? extends Object> maybeValue = TypeCoercions.tryCoerce(values[i], TypeToken.of(clazz));
    +                Object value = maybeValue.isAbsent() ? null : maybeValue.get();
    +                AttributeSensor<Object> attribute = (AttributeSensor<Object>) Sensors.newSensor(clazz, polls.get(i).getSensor().getName(), polls.get(i).getDescription());
    +                entity.setAttribute(attribute, value);
    +            }
    +        }
    +
    +        @Override
    +        public void onFailure(WinRmToolResponse val) {
    +            log.error("Windows Performance Counter query did not respond as expected. exitcode={} stdout={} stderr={}",
    +                    new Object[]{val.getStatusCode(), val.getStdOut(), val.getStdErr()});
    +            for (WindowsPerformanceCounterPollConfig<?> config : polls) {
    +                entity.setAttribute(Sensors.newSensor(config.getSensor().getClass(), config.getPerformanceCounterName(), config.getDescription()), null);
    --- End diff --
    
    Above (for `onSuccess`) we did `polls.get(i).getSensor().getName()`. Why are we using `config.getPerformanceCounterName()` here instead? Same below for `onException`.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30655418
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +                WinRmToolResponse response = winRmTool.executeScript(script);
    +                return response;
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying:", e);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute shell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScript(String psScript) {
    +        return executePsScript(ImmutableList.of(psScript));
    +    }
    +
    +    public WinRmToolResponse executePsScript(List<String> psScript) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                return executePsScriptNoRetry(psScript);
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying after 5 seconds:", e);
    +                Time.sleep(Duration.FIVE_SECONDS);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute powershell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScriptNoRetry(List<String> psScript) {
    +        WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +        WinRmToolResponse response = winRmTool.executePs(psScript);
    +        return response;
    +    }
    +
    +    public int copyTo(File source, File destination) {
    +        try {
    +            return copyTo(new FileInputStream(source), destination);
    +        } catch (FileNotFoundException e) {
    +            throw Exceptions.propagate(e);
    +        }
    +    }
    +
    +    public int copyTo(InputStream source, File destination) {
    +        executePsScript(ImmutableList.of("rm -ErrorAction SilentlyContinue " + destination.getPath()));
    +        try {
    +            int chunkSize = getConfig(COPY_FILE_CHUNK_SIZE_BYTES);
    +            byte[] inputData = new byte[chunkSize];
    +            int bytesRead;
    +            int expectedFileSize = 0;
    +            while ((bytesRead = source.read(inputData)) > 0) {
    +                byte[] chunk;
    +                if (bytesRead == chunkSize) {
    +                    chunk = inputData;
    +                } else {
    +                    chunk = Arrays.copyOf(inputData, bytesRead);
    +                }
    +                executePsScript(ImmutableList.of("If ((!(Test-Path " + destination.getPath() + ")) -or ((Get-Item '" + destination.getPath() + "').length -eq " +
    +                        expectedFileSize + ")) {Add-Content -Encoding Byte -path " + destination.getPath() +
    +                        " -value ([System.Convert]::FromBase64String(\"" + new String(Base64.encodeBase64(chunk)) + "\"))}"));
    +                expectedFileSize += bytesRead;
    +            }
    +
    +            return 0;
    +        } catch (java.io.IOException e) {
    +            throw Exceptions.propagate(e);
    +        }
    +    }
    +
    +    public String getUsername() {
    +        return config().get(WINDOWS_USERNAME);
    +    }
    +
    +    private String getPassword() {
    +        return config().get(WINDOWS_PASSWORD);
    +    }
    +
    +    public static String getDefaultUserMetadataString() {
    --- End diff --
    
    Worth some javadoc to say what this default user metadata will do.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30708221
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsWinRmMachineLocation.java ---
    @@ -0,0 +1,25 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.jclouds;
    +
    +import brooklyn.location.basic.WinRmMachineLocation;
    +
    +public class JcloudsWinRmMachineLocation extends WinRmMachineLocation {
    --- End diff --
    
    I wonder if it would be useful to have a `JcloudsMachineLocation` interface, with methods like `getNode()` and `getTemplate()`. That would also extend the `HasSubnetHostname` interface, with `getSubnetIp()` etc.
    
    Ignore for this PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30683870
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -712,80 +737,105 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                     List<String> setupScripts = setup.get(JcloudsLocationConfig.CUSTOM_MACHINE_SETUP_SCRIPT_URL_LIST);
                     Collection<String> allScripts = new MutableList<String>().appendIfNotNull(setupScript).appendAll(setupScripts);
                     for (String setupScriptItem : allScripts) {
    -                    if (Strings.isNonBlank(setupScriptItem)) {
    -                        customisationForLogging.add("custom setup script "+setupScriptItem);
    +                    if (Strings.isNonBlank(setupScript)) {
    --- End diff --
    
    Why change this to `setupScript`, rather than `setupScriptItem`? See the for loop above.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615918
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -251,10 +258,71 @@ public T call() throws Exception {
             }
         }
     
    +    private static class SendPerfCountersToSensors implements PollHandler<WinRmToolResponse> {
    +
    +        private final EntityLocal entity;
    +        private final List<WindowsPerformanceCounterPollConfig<?>> polls;
    +
    +        public SendPerfCountersToSensors(EntityLocal entity, List<WindowsPerformanceCounterPollConfig<?>> polls) {
    +            this.entity = entity;
    +            this.polls = polls;
    +        }
    +
    +        @Override
    +        public boolean checkSuccess(WinRmToolResponse val) {
    +            if (val.getStatusCode() != 0) return false;
    +            String stderr = val.getStdErr();
    +            if (stderr == null || stderr.length() != 0) return false;
    +            String out = val.getStdOut();
    +            if (out == null || out.length() == 0) return false;
    +            return true;
    +        }
    +
    +        @Override
    +        public void onSuccess(WinRmToolResponse val) {
    +            String[] values = val.getStdOut().split("\r\n");
    +            for (int i = 0; i < polls.size(); i++) {
    +                Class<?> clazz = polls.get(i).getSensor().getType();
    +                Maybe<? extends Object> maybeValue = TypeCoercions.tryCoerce(values[i], TypeToken.of(clazz));
    +                Object value = maybeValue.isAbsent() ? null : maybeValue.get();
    +                AttributeSensor<Object> attribute = (AttributeSensor<Object>) Sensors.newSensor(clazz, polls.get(i).getSensor().getName(), polls.get(i).getDescription());
    +                entity.setAttribute(attribute, value);
    +            }
    +        }
    +
    +        @Override
    +        public void onFailure(WinRmToolResponse val) {
    +            log.error("Windows Performance Counter query did not respond as expected. exitcode={} stdout={} stderr={}",
    +                    new Object[]{val.getStatusCode(), val.getStdOut(), val.getStdErr()});
    +            for (WindowsPerformanceCounterPollConfig<?> config : polls) {
    +                entity.setAttribute(Sensors.newSensor(config.getSensor().getClass(), config.getPerformanceCounterName(), config.getDescription()), null);
    +            }
    +        }
    +
    +        @Override
    +        public void onException(Exception exception) {
    +            log.error("Detected exception while retrieving Windows Performance Counters from entity " +
    +                    entity.getDisplayName(), exception);
    +            for (WindowsPerformanceCounterPollConfig<?> config : polls) {
    +                entity.setAttribute(Sensors.newSensor(config.getSensor().getClass(), config.getPerformanceCounterName(), config.getDescription()), null);
    +            }
    +        }
    +
    +        @Override
    +        public String getDescription() {
    +            return "" + polls;
    +        }
    +
    +        @Override
    +        public String toString() {
    +            return super.toString()+"["+getDescription()+"]";
    +        }
    +    }
    +
         /**
          * A poll handler that takes the result of the <tt>typeperf</tt> invocation and sets the appropriate sensors.
          */
    -    private static class SendPerfCountersToSensors implements PollHandler<SshPollValue> {
    +    private static class SendPerfCountersToSensors2 implements PollHandler<SshPollValue> {
    --- End diff --
    
    Do we need this class? It appears to never be used.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30902153
  
    --- Diff: software/database/src/main/resources/brooklyn/entity/database/mssql/installmssql.ps1 ---
    @@ -0,0 +1,49 @@
    +[#ftl]
    +#!ps1
    +#
    +# Licensed to the Apache Software Foundation (ASF) under one
    +# or more contributor license agreements.  See the NOTICE file
    +# distributed with this work for additional information
    +# regarding copyright ownership.  The ASF licenses this file
    +# to you under the Apache License, Version 2.0 (the
    +# "License"); you may not use this file except in compliance
    +# with the License.  You may obtain a copy of the License at
    +#
    +#  http://www.apache.org/licenses/LICENSE-2.0
    +#
    +# Unless required by applicable law or agreed to in writing,
    +# software distributed under the License is distributed on an
    +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +# KIND, either express or implied.  See the License for the
    +# specific language governing permissions and limitations
    +# under the License.
    +#
    +
    +$Url = "${config['mssql.download.url']}"
    +$Path = "C:\sql2008.iso"
    +$Username = "${config['mssql.download.user']}"
    +$Password = '${config['mssql.download.password']}'
    +
    +
    +$WebClient = New-Object System.Net.WebClient
    +$WebClient.Credentials = New-Object System.Net.Networkcredential($Username, $Password)
    +$WebClient.DownloadFile( $url, $path )
    +
    +$mountResult = Mount-DiskImage $Path -PassThru
    +$driveLetter = (($mountResult | Get-Volume).DriveLetter) + ":\"
    +
    +New-Item -ItemType Directory -Force -Path "C:\Program Files (x86)\Microsoft SQL Server\DReplayClient\ResultDir"
    +New-Item -ItemType Directory -Force -Path "C:\Program Files (x86)\Microsoft SQL Server\DReplayClient\WorkingDir"
    +
    +Install-WindowsFeature NET-Framework-Core
    +
    +$pass = '${attribute['windows.password']}'
    +$secpasswd = ConvertTo-SecureString $pass -AsPlainText -Force
    +$mycreds = New-Object System.Management.Automation.PSCredential ($($env:COMPUTERNAME + "\Administrator"), $secpasswd)
    +
    +Invoke-Command -ComputerName localhost -credential $mycreds -scriptblock {
    +    param($driveLetter)
    +    Start-Process ( $driveLetter + "setup.exe") -ArgumentList "/ConfigurationFile=C:\ConfigurationFile.ini" -RedirectStandardOutput "C:\sqlout.txt" -RedirectStandardError "C:\sqlerr.txt" -Wait
    --- End diff --
    
    NOTE: The $driveLetter defined on line 33 is unavailable in this scriptblock, and so is passed into the scriptblock via the argumentlist on line 47. The param($driveLetter) directive on line 45 indicates that the zeroth argument in the argumentlist on line 47 should be referenced as "$driveLetter" within the script block


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30687518
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -938,7 +1058,11 @@ public void apply(TemplateOptions t, ConfigBag props, Object v) {
                         public void apply(TemplateOptions t, ConfigBag props, Object v) {
                             if (t instanceof EC2TemplateOptions) {
                                 if (v==null) return;
    -                            ((EC2TemplateOptions)t).userData(v.toString().getBytes());
    +                            String data = v.toString();
    +                            if (!(data.startsWith("<script>") || data.startsWith("<powershell>"))) {
    +                                data = "<script> " + data + " </script>";
    --- End diff --
    
    Do we have a test that this works for linux and for windows? (haven't reached your tests in the PR review yet).
    
    Out of interest, where in jclouds is this documented?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30687242
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -836,6 +910,52 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
             }
         }
     
    +    private void waitForWinRmAvailable(final NodeMetadata node, final LoginCredentials expectedCredentials, ConfigBag setup) {
    +        // TODO: Reduce / remove duplication between this and waitForReachable
    --- End diff --
    
    Agree feels like a fair amount of duplication between this and `waitForReachable`. Worth considering if can have a common impl, e.g. passing in the checker, and the config key for wait time.



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615966
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -98,10 +104,11 @@
         // This pattern matches CSV line(s) with the date in the first field, and at least one further field.
         protected static final Pattern lineWithPerfData = Pattern.compile("^\"[\\d:/\\-. ]+\",\".*\"$", Pattern.MULTILINE);
         private static final Joiner JOINER_ON_SPACE = Joiner.on(' ');
    +    private static final Joiner JOINER_ON_COMMA = Joiner.on(',');
     
         @SuppressWarnings("serial")
    -    public static final ConfigKey<Set<WindowsPerformanceCounterPollConfig<?>>> POLLS = ConfigKeys.newConfigKey(
    -            new TypeToken<Set<WindowsPerformanceCounterPollConfig<?>>>() {},
    +    public static final ConfigKey<List<WindowsPerformanceCounterPollConfig<?>>> POLLS = ConfigKeys.newConfigKey(
    --- End diff --
    
    Could/should we use Collection rather than List?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616639
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +                WinRmToolResponse response = winRmTool.executeScript(script);
    +                return response;
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying:", e);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute shell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScript(String psScript) {
    +        return executePsScript(ImmutableList.of(psScript));
    +    }
    +
    +    public WinRmToolResponse executePsScript(List<String> psScript) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                return executePsScriptNoRetry(psScript);
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying after 5 seconds:", e);
    +                Time.sleep(Duration.FIVE_SECONDS);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute powershell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScriptNoRetry(List<String> psScript) {
    --- End diff --
    
    Suggest making this protected; same for other methods you don't expect people to call.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30709820
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/AbstractSoftwareProcessWinRmDriver.java ---
    @@ -0,0 +1,192 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import java.io.File;
    +import java.io.InputStream;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.Callable;
    +import java.util.concurrent.TimeUnit;
    +
    +import org.python.core.PyException;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.event.AttributeSensor;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.location.basic.WinRmMachineLocation;
    +import brooklyn.util.exceptions.ReferenceWithError;
    +import brooklyn.util.repeat.Repeater;
    +import brooklyn.util.time.Duration;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.api.client.util.Strings;
    +import com.google.common.collect.ImmutableList;
    +
    +public abstract class AbstractSoftwareProcessWinRmDriver extends AbstractSoftwareProcessDriver {
    +
    +    AttributeSensor<String> WINDOWS_USERNAME = Sensors.newStringSensor("windows.username",
    +            "Default Windows username to be used when connecting to the Entity's VM");
    +    AttributeSensor<String> WINDOWS_PASSWORD = Sensors.newStringSensor("windows.password",
    +            "Default Windows password to be used when connecting to the Entity's VM");
    +
    +    public AbstractSoftwareProcessWinRmDriver(EntityLocal entity, WinRmMachineLocation location) {
    +        super(entity, location);
    +        entity.setAttribute(WINDOWS_USERNAME, location.config().get(WinRmMachineLocation.WINDOWS_USERNAME));
    +        entity.setAttribute(WINDOWS_PASSWORD, location.config().get(WinRmMachineLocation.WINDOWS_PASSWORD));
    +    }
    +
    +    @Override
    +    public void runPreInstallCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void setup() {
    +        // Default to no-op
    +    }
    +
    +    @Override
    +    public void runPostInstallCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void runPreLaunchCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void runPostLaunchCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public WinRmMachineLocation getLocation() {
    +        return (WinRmMachineLocation)super.getLocation();
    +    }
    +
    +    @Override
    +    public String getRunDir() {
    +        // TODO: This needs to be tidied, and read from the appropriate flags (if set)
    +        return "$HOME\\brooklyn-managed-processes\\apps\\" + entity.getApplicationId() + "\\entities\\" + getEntityVersionLabel()+"_"+entity.getId();
    +    }
    +
    +    @Override
    +    public String getInstallDir() {
    +        // TODO: This needs to be tidied, and read from the appropriate flags (if set)
    +        return "$HOME\\brooklyn-managed-processes\\installs\\" + entity.getApplicationId() + "\\" + getEntityVersionLabel()+"_"+entity.getId();
    +    }
    +
    +    @Override
    +    public int copyResource(Map<Object, Object> sshFlags, String source, String target, boolean createParentDir) {
    +        if (createParentDir) {
    +            createDirectory(getDirectory(target), "Creating resource directory");
    +        }
    +        return copyTo(new File(source), new File(target));
    +    }
    +
    +    @Override
    +    public int copyResource(Map<Object, Object> sshFlags, InputStream source, String target, boolean createParentDir) {
    +        if (createParentDir) {
    +            createDirectory(getDirectory(target), "Creating resource directory");
    +        }
    +        return copyTo(source, new File(target));
    +    }
    +
    +    @Override
    +    protected void createDirectory(String directoryName, String summaryForLogging) {
    +        getLocation().executePsScript("New-Item -path \"" + directoryName + "\" -type directory -ErrorAction SilentlyContinue");
    +    }
    +
    +    protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
    +        String regularCommand = getEntity().getConfig(regularCommandKey);
    +        String powershellCommand = getEntity().getConfig(powershellCommandKey);
    +        if (Strings.isNullOrEmpty(regularCommand) && Strings.isNullOrEmpty(powershellCommand)) {
    +            if (allowNoOp) {
    +                return new WinRmToolResponse("", "", 0);
    +            } else {
    +                throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
    +            }
    +        } else if (!Strings.isNullOrEmpty(regularCommand) && !Strings.isNullOrEmpty(powershellCommand)) {
    +            throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
    +        }
    +
    +        if (Strings.isNullOrEmpty(regularCommand)) {
    +            return getLocation().executePsScript(ImmutableList.of(powershellCommand));
    +        } else {
    +            return getLocation().executeScript(ImmutableList.of(regularCommand));
    +        }
    +    }
    +
    +    public int execute(List<String> script) {
    +        return getLocation().executeScript(script).getStatusCode();
    +    }
    +
    +    public int executePowerShellNoRetry(List<String> psScript) {
    +        return getLocation().executePsScriptNoRetry(psScript).getStatusCode();
    +    }
    +
    +    public int executePowerShell(List<String> psScript) {
    +        return getLocation().executePsScript(psScript).getStatusCode();
    +    }
    +
    +    public int copyTo(File source, File destination) {
    +        return getLocation().copyTo(source, destination);
    +    }
    +
    +    public int copyTo(InputStream source, File destination) {
    +        return getLocation().copyTo(source, destination);
    +    }
    +
    +    public void rebootAndWait() {
    +        try {
    +            executePowerShellNoRetry(ImmutableList.of("Restart-Computer -Force"));
    +        } catch (PyException e) {
    +            // Restarting the computer will cause the command to fail; ignore the exception and continue
    --- End diff --
    
    Can we be more specific - e.g. what exception do you expect. And log if not exactly right. There's a risk we'll pretend the VM is rebooting when actually we got back an error about permissions or something like that.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30709291
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/AbstractSoftwareProcessWinRmDriver.java ---
    @@ -0,0 +1,192 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import java.io.File;
    +import java.io.InputStream;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.Callable;
    +import java.util.concurrent.TimeUnit;
    +
    +import org.python.core.PyException;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.event.AttributeSensor;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.location.basic.WinRmMachineLocation;
    +import brooklyn.util.exceptions.ReferenceWithError;
    +import brooklyn.util.repeat.Repeater;
    +import brooklyn.util.time.Duration;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.api.client.util.Strings;
    +import com.google.common.collect.ImmutableList;
    +
    +public abstract class AbstractSoftwareProcessWinRmDriver extends AbstractSoftwareProcessDriver {
    +
    +    AttributeSensor<String> WINDOWS_USERNAME = Sensors.newStringSensor("windows.username",
    --- End diff --
    
    Is inside the driver the best place to declare these sensors?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616459
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    --- End diff --
    
    Worth extracting this as a config key.
    
    Also worth doing some smarter logging. e.g. saying attempt 1 of 10, and not logging "retrying" on the final failure when it's not going to retry.
    
    I wonder if worth creating a wrapper of WinrmTool that gives us any customer behaviour that we want. Having this code here feels quite different from the `SshMachineLocation` implementation. But let's not worry about that in this PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616515
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +                WinRmToolResponse response = winRmTool.executeScript(script);
    +                return response;
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying:", e);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute shell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScript(String psScript) {
    +        return executePsScript(ImmutableList.of(psScript));
    +    }
    +
    +    public WinRmToolResponse executePsScript(List<String> psScript) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    --- End diff --
    
    See comment about retries in executeScript


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30655571
  
    --- Diff: core/src/main/java/brooklyn/util/flags/TypeCoercions.java ---
    @@ -709,6 +710,13 @@ public QuorumCheck apply(final String input) {
                     return QuorumChecks.of(input);
                 }
             });
    +        registerAdapter(ArrayList.class, String[].class, new Function<ArrayList, String[]>() {
    +            @Nullable
    +            @Override
    +            public String[] apply(@Nullable ArrayList arrayList) {
    +                return (String[]) arrayList.toArray(new String[]{});
    --- End diff --
    
    If marking as `@Nullable`, then should handle that: return null if `arrayList == null`?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30616849
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +                WinRmToolResponse response = winRmTool.executeScript(script);
    +                return response;
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying:", e);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute shell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScript(String psScript) {
    +        return executePsScript(ImmutableList.of(psScript));
    +    }
    +
    +    public WinRmToolResponse executePsScript(List<String> psScript) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                return executePsScriptNoRetry(psScript);
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying after 5 seconds:", e);
    +                Time.sleep(Duration.FIVE_SECONDS);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute powershell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScriptNoRetry(List<String> psScript) {
    +        WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +        WinRmToolResponse response = winRmTool.executePs(psScript);
    +        return response;
    +    }
    +
    +    public int copyTo(File source, File destination) {
    --- End diff --
    
    Not sure you should use `File` as the destination. Problem is that the destination will be a windows path, but File here could be constructed with Brooklyn running on Linux.
    
    Does that matter? Will it have used the right separator for the destination File?!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30683180
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -635,10 +645,12 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                     sshHostAndPortOverride = Optional.absent();
                 }
     
    -            if (waitForSshable && skipJcloudsSshing) {
    +            if (waitForSshable && skipJcloudsSshing && !windows) {
                     // once that host:port is definitely reachable, we can create the user
                     waitForReachable(computeService, node, sshHostAndPortOverride, node.getCredentials(), setup);
                     userCredentials = createUser(computeService, node, sshHostAndPortOverride, setup);
    +            } else if (windows) {
    --- End diff --
    
    We want to be able to defer/never waitForWinRmAvailable. That was intent of `waitForSshable` - to turn that off. 
    
    Historically, reason for that was to let the caller set up port forwarding. However, that is now covered in code above (but not for windows case!). So we generally don't make use of the `waitForSshable` much these days. But given it's there...
    
    I wonder if we should generalise it to a "waitForReachable" or some such.
    
    Don't worry about it for this PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30901589
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -938,7 +1058,11 @@ public void apply(TemplateOptions t, ConfigBag props, Object v) {
                         public void apply(TemplateOptions t, ConfigBag props, Object v) {
                             if (t instanceof EC2TemplateOptions) {
                                 if (v==null) return;
    -                            ((EC2TemplateOptions)t).userData(v.toString().getBytes());
    +                            String data = v.toString();
    +                            if (!(data.startsWith("<script>") || data.startsWith("<powershell>"))) {
    +                                data = "<script> " + data + " </script>";
    --- End diff --
    
    This is documented in the AWS documentation here: http://docs.aws.amazon.com/AWSEC2/latest/WindowsGuide/UsingConfig_WinAMI.html#user-data-execution


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30655262
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    +            "Username to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<String> WINDOWS_PASSWORD = ConfigKeys.newStringConfigKey("windows.password",
    +            "Password to use when connecting to the remote machine");
    +
    +    public static final ConfigKey<Integer> COPY_FILE_CHUNK_SIZE_BYTES = ConfigKeys.newIntegerConfigKey("windows.copy.file.size.bytes",
    +            "Size of file chunks (in bytes) to be used when copying a file to the remote server", 1024);
    +
    +    @SetFromFlag
    +    protected String user;
    +
    +    @SetFromFlag(nullable = false)
    +    protected InetAddress address;
    +
    +    @Override
    +    public InetAddress getAddress() {
    +        return address;
    +    }
    +
    +    @Override
    +    public OsDetails getOsDetails() {
    +        return null;
    +    }
    +
    +    @Override
    +    public MachineDetails getMachineDetails() {
    +        return null;
    +    }
    +
    +    @Nullable
    +    @Override
    +    public String getHostname() {
    +        return address.getHostAddress();
    +    }
    +
    +    @Override
    +    public Set<String> getPublicAddresses() {
    +        return null;
    +    }
    +
    +    @Override
    +    public Set<String> getPrivateAddresses() {
    +        return null;
    +    }
    +
    +    public WinRmToolResponse executeScript(String script) {
    +        return executeScript(ImmutableList.of(script));
    +    }
    +
    +    public WinRmToolResponse executeScript(List<String> script) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +                WinRmToolResponse response = winRmTool.executeScript(script);
    +                return response;
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying:", e);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute shell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScript(String psScript) {
    +        return executePsScript(ImmutableList.of(psScript));
    +    }
    +
    +    public WinRmToolResponse executePsScript(List<String> psScript) {
    +        Collection<Throwable> exceptions = Lists.newArrayList();
    +        for (int i = 0; i < 10; i++) {
    +            try {
    +                return executePsScriptNoRetry(psScript);
    +            } catch (Exception e) {
    +                LOG.warn("ignoring winrm exception and retrying after 5 seconds:", e);
    +                Time.sleep(Duration.FIVE_SECONDS);
    +                exceptions.add(e);
    +            }
    +        }
    +        throw Exceptions.propagate("failed to execute powershell script", exceptions);
    +    }
    +
    +    public WinRmToolResponse executePsScriptNoRetry(List<String> psScript) {
    +        WinRmTool winRmTool = WinRmTool.connect(getHostname(), getUsername(), getPassword());
    +        WinRmToolResponse response = winRmTool.executePs(psScript);
    +        return response;
    +    }
    +
    +    public int copyTo(File source, File destination) {
    +        try {
    +            return copyTo(new FileInputStream(source), destination);
    +        } catch (FileNotFoundException e) {
    +            throw Exceptions.propagate(e);
    +        }
    +    }
    +
    +    public int copyTo(InputStream source, File destination) {
    +        executePsScript(ImmutableList.of("rm -ErrorAction SilentlyContinue " + destination.getPath()));
    +        try {
    +            int chunkSize = getConfig(COPY_FILE_CHUNK_SIZE_BYTES);
    +            byte[] inputData = new byte[chunkSize];
    +            int bytesRead;
    +            int expectedFileSize = 0;
    +            while ((bytesRead = source.read(inputData)) > 0) {
    +                byte[] chunk;
    +                if (bytesRead == chunkSize) {
    +                    chunk = inputData;
    +                } else {
    +                    chunk = Arrays.copyOf(inputData, bytesRead);
    +                }
    +                executePsScript(ImmutableList.of("If ((!(Test-Path " + destination.getPath() + ")) -or ((Get-Item '" + destination.getPath() + "').length -eq " +
    +                        expectedFileSize + ")) {Add-Content -Encoding Byte -path " + destination.getPath() +
    +                        " -value ([System.Convert]::FromBase64String(\"" + new String(Base64.encodeBase64(chunk)) + "\"))}"));
    --- End diff --
    
    Worth a comment describing this approach to file-copy


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/650


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30655884
  
    --- Diff: core/src/main/java/brooklyn/location/basic/WinRmMachineLocation.java ---
    @@ -0,0 +1,266 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.basic;
    +
    +import java.io.File;
    +import java.io.FileInputStream;
    +import java.io.FileNotFoundException;
    +import java.io.InputStream;
    +import java.net.InetAddress;
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.List;
    +import java.util.Set;
    +
    +import javax.annotation.Nullable;
    +
    +import org.apache.commons.codec.binary.Base64;
    +import org.python.core.PyException;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.location.MachineDetails;
    +import brooklyn.location.MachineLocation;
    +import brooklyn.location.OsDetails;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.time.Duration;
    +import brooklyn.util.time.Time;
    +import io.cloudsoft.winrm4j.winrm.WinRmTool;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.common.base.Charsets;
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Lists;
    +
    +public class WinRmMachineLocation extends AbstractLocation implements MachineLocation {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(WinRmMachineLocation.class);
    +
    +    public static final ConfigKey<String> WINDOWS_USERNAME = ConfigKeys.newStringConfigKey("windows.username",
    --- End diff --
    
    Why use the name `windows.username`, rather than having it match the names used in `SshMachineLocation` (i.e. "user" and "password")? I expect it will give a more consistent feel to Brooklyn if the names match.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30901940
  
    --- Diff: software/database/src/main/resources/brooklyn/entity/database/mssql/installmssql.ps1 ---
    @@ -0,0 +1,49 @@
    +[#ftl]
    +#!ps1
    +#
    +# Licensed to the Apache Software Foundation (ASF) under one
    +# or more contributor license agreements.  See the NOTICE file
    +# distributed with this work for additional information
    +# regarding copyright ownership.  The ASF licenses this file
    +# to you under the Apache License, Version 2.0 (the
    +# "License"); you may not use this file except in compliance
    +# with the License.  You may obtain a copy of the License at
    +#
    +#  http://www.apache.org/licenses/LICENSE-2.0
    +#
    +# Unless required by applicable law or agreed to in writing,
    +# software distributed under the License is distributed on an
    +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +# KIND, either express or implied.  See the License for the
    +# specific language governing permissions and limitations
    +# under the License.
    +#
    +
    +$Url = "${config['mssql.download.url']}"
    +$Path = "C:\sql2008.iso"
    +$Username = "${config['mssql.download.user']}"
    +$Password = '${config['mssql.download.password']}'
    +
    +
    +$WebClient = New-Object System.Net.WebClient
    +$WebClient.Credentials = New-Object System.Net.Networkcredential($Username, $Password)
    +$WebClient.DownloadFile( $url, $path )
    +
    +$mountResult = Mount-DiskImage $Path -PassThru
    +$driveLetter = (($mountResult | Get-Volume).DriveLetter) + ":\"
    +
    +New-Item -ItemType Directory -Force -Path "C:\Program Files (x86)\Microsoft SQL Server\DReplayClient\ResultDir"
    +New-Item -ItemType Directory -Force -Path "C:\Program Files (x86)\Microsoft SQL Server\DReplayClient\WorkingDir"
    +
    +Install-WindowsFeature NET-Framework-Core
    +
    --- End diff --
    
    Simply calling "D:\setup.exe" at this point will work if the script is run locally, but will fail if the script is run via winrm over http as the credentials used for execution will be marked as remote credentials. To work around this, we re-authenticate within the script to obtain a new set of 'local' credentials and call setup.exe using the 'Invoke-Command scriptlet which allows us to pass in the newly-obtained credentials


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30727178
  
    --- Diff: software/base/src/main/java/brooklyn/entity/software/StaticSensor.java ---
    @@ -0,0 +1,44 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.software;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.basic.EntityLocal;
    +import brooklyn.entity.effector.AddSensor;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.util.config.ConfigBag;
    +
    +public class StaticSensor<T> extends AddSensor<Integer> {
    --- End diff --
    
    There is no test for `StaticSensor`. Can we add one?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30726439
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/VanillaWindowsProcess.java ---
    @@ -0,0 +1,54 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.proxying.ImplementedBy;
    +import brooklyn.util.time.Duration;
    +
    +@ImplementedBy(VanillaWindowsProcessImpl.class)
    +public interface VanillaWindowsProcess extends AbstractVanillaProcess {
    +    ConfigKey<String> PRE_INSTALL_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("pre.install.powershell.command",
    --- End diff --
    
    Medium term, would be good to think more about how an entity author (working in yaml) can control the different phases. This is good enough for now, but we'll no doubt hit a use-case where one needs to reboot at a different phase, or where more phases would be useful (particularly for the equivalent of "sub-classing" within the yaml).
    
    @alasdairhodge has some good thoughts on this, as has @ahgittin - worth a bigger discussion on that.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by richardcloudsoft <gi...@git.apache.org>.
Github user richardcloudsoft commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r31114432
  
    --- Diff: docs/guide/yaml/winrm/index.md ---
    @@ -0,0 +1,17 @@
    +---
    +title: Windows blueprints using WinRM
    +layout: website-normal
    +children:
    +- about-winrm.md
    +- re-authentication.md
    +- stdout-and-stderr.md
    +---
    +
    +This guide describes how Brooklyn entities can be easily created from Chef cookbooks.
    --- End diff --
    
    This text and link is c&p from the Chef pages and needs re-written for Windows!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30686780
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -795,24 +845,48 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
     
                 // Apply any optional app-specific customization.
                 for (JcloudsLocationCustomizer customizer : getCustomizers(setup)) {
    -                customizer.customize(this, computeService, sshMachineLocation);
    +                if (windows) {
    +                    LOG.warn("Ignoring customizer {} on Windows location {}", customizer, winRmMachineLocation);
    +                    // TODO: WinRm based JcloudsLocationCustomizer
    +                } else {
    +                    customizer.customize(this, computeService, jcloudsSshMachineLocation);
    +                }
                 }
     
                 customizedTimestamp = Duration.of(provisioningStopwatch);
     
    -            LOG.info("Finished VM "+setup.getDescription()+" creation:"
    -                    + " "+sshMachineLocation.getUser()+"@"+sshMachineLocation.getAddress()+":"+sshMachineLocation.getPort()
    -                    + (Boolean.TRUE.equals(setup.get(LOG_CREDENTIALS))
    -                              ? "password=" + userCredentials.getOptionalPassword().or("<absent>")
    -                                      + " && key=" + userCredentials.getOptionalPrivateKey().or("<absent>")
    -                              : "")
    -                    + " ready after "+Duration.of(provisioningStopwatch).toStringRounded()
    -                    + " ("+template+" template built in "+Duration.of(templateTimestamp).toStringRounded()+";"
    -                    + " "+node+" provisioned in "+Duration.of(provisionTimestamp).subtract(templateTimestamp).toStringRounded()+";"
    -                    + " "+sshMachineLocation+" ssh usable in "+Duration.of(usableTimestamp).subtract(provisionTimestamp).toStringRounded()+";"
    -                    + " and os customized in "+Duration.of(customizedTimestamp).subtract(usableTimestamp).toStringRounded()+" - "+Joiner.on(", ").join(customisationForLogging)+")");
    -
    -            return sshMachineLocation;
    +            String logMessage;
    +
    +            if (windows) {
    +                // TODO: More complete logging for WinRmMachineLocation
    +                // FIXME: Remove try-catch!
    +                try {
    +                    logMessage = "Finished VM " + setup.getDescription() + " creation:"
    +                            + " " + winRmMachineLocation.getConfig(WinRmMachineLocation.WINDOWS_USERNAME) + "@" + machineLocation.getAddress()
    +                            + " username=" + winRmMachineLocation.getUsername()
    +                            + " ready after " + Duration.of(provisioningStopwatch).toStringRounded()
    +                            + " (" + template + " template built in " + Duration.of(templateTimestamp).toStringRounded() + ";"
    +                            + " " + node + " provisioned in " + Duration.of(provisionTimestamp).subtract(templateTimestamp).toStringRounded() + ";";
    +                } catch (Exception e){
    +                    logMessage = Arrays.toString(e.getStackTrace());
    --- End diff --
    
    What exception are you expecting here? Can we just program more defensively when building the message?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30725831
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/VanillaWindowsProcessDriver.java ---
    @@ -0,0 +1,23 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +public interface VanillaWindowsProcessDriver extends AbstractVanillaProcessDriver {
    --- End diff --
    
    Not sure about the name `VanillaWindowsProcessDriver` versus `VanillaSoftwareProcessDriver`. They are both software processes.
    
    But I don't have a better suggestion just now!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30683932
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -712,80 +737,105 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                     List<String> setupScripts = setup.get(JcloudsLocationConfig.CUSTOM_MACHINE_SETUP_SCRIPT_URL_LIST);
                     Collection<String> allScripts = new MutableList<String>().appendIfNotNull(setupScript).appendAll(setupScripts);
                     for (String setupScriptItem : allScripts) {
    -                    if (Strings.isNonBlank(setupScriptItem)) {
    -                        customisationForLogging.add("custom setup script "+setupScriptItem);
    +                    if (Strings.isNonBlank(setupScript)) {
    +                        customisationForLogging.add("custom setup script " + setupScript);
     
                             String setupVarsString = setup.get(JcloudsLocationConfig.CUSTOM_MACHINE_SETUP_SCRIPT_VARS);
                             Map<String, String> substitutions = (setupVarsString != null)
                                     ? Splitter.on(",").withKeyValueSeparator(":").split(setupVarsString)
                                     : ImmutableMap.<String, String>of();
    -                        String scriptContent =  ResourceUtils.create(this).getResourceAsString(setupScriptItem);
    +                        String scriptContent = ResourceUtils.create(this).getResourceAsString(setupScriptItem);
                             String script = TemplateProcessor.processTemplateContents(scriptContent, getManagementContext(), substitutions);
    -                        sshMachineLocation.execCommands("Customizing node " + this + " with script " + setupScriptItem, ImmutableList.of(script));
    +                        if (windows) {
    +                            winRmMachineLocation.executeScript(ImmutableList.copyOf((script.replace("\r", "").split("\n"))));
    +                        } else {
    +                            jcloudsSshMachineLocation.execCommands("Customizing node " + this, ImmutableList.of(script));
    --- End diff --
    
    This changes the execCommands description. I take it that's deliberate? Was the full script being logged multiple times before?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30726902
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/VanillaWindowsProcess.java ---
    @@ -0,0 +1,54 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.proxying.ImplementedBy;
    +import brooklyn.util.time.Duration;
    +
    +@ImplementedBy(VanillaWindowsProcessImpl.class)
    +public interface VanillaWindowsProcess extends AbstractVanillaProcess {
    +    ConfigKey<String> PRE_INSTALL_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("pre.install.powershell.command",
    +            "powershell command to run during the pre-install phase");
    +    ConfigKey<Boolean> PRE_INSTALL_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("pre.install.reboot.required",
    +            "indicates that a reboot should be performed after the pre-install command is run", false);
    +    ConfigKey<Boolean> INSTALL_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("install.reboot.required",
    +            "indicates that a reboot should be performed after the install command is run", false);
    +    ConfigKey<Boolean> CUSTOMIZE_REBOOT_REQUIRED = ConfigKeys.newBooleanConfigKey("customize.reboot.required",
    +            "indicates that a reboot should be performed after the customize command is run", false);
    +    ConfigKey<String> LAUNCH_POWERSHELL_COMMAND = ConfigKeys.newStringConfigKey("launch.powershell.command",
    --- End diff --
    
    Why do others have `CUSTOMIZE_COMMAND` and `CUSTOMIZE_POWERSHELL_COMMAND`, but launch only has `LAUNCH_POWERSHELL_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30683744
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -665,42 +677,55 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                 setup.putIfNotNull(JcloudsLocationConfig.PRIVATE_KEY_DATA, userCredentials.getOptionalPrivateKey().orNull());
     
                 // Wait for the VM to be reachable over SSH
    -            if (waitForSshable) {
    +            if (waitForSshable && !windows) {
                     waitForReachable(computeService, node, sshHostAndPortOverride, userCredentials, setup);
                 } else {
                     LOG.debug("Skipping ssh check for {} ({}) due to config waitForSshable=false", node, setup.getDescription());
                 }
                 usableTimestamp = Duration.of(provisioningStopwatch);
     
    +            JcloudsSshMachineLocation jcloudsSshMachineLocation = null;
    --- End diff --
    
    I wonder about just having the `machineLocation` variable, and casting it where required. Rather than having 3 variables where 1 will be null. Not sure what the best pattern is for that. The casting would force us to think about it more, to decide if we can generalise the method to take a machineLocation instead (and thus work with ssh and windows).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#issuecomment-104680433
  
    A sample brooklyn.properties setup for a Windows AWS location is as follows:
    
    ```
    # WinRM AWS Oregon
    brooklyn.location.named.AWS\ Oregon\ Win=jclouds:aws-ec2:us-west-2
    brooklyn.location.named.AWS\ Oregon\ Win.displayName = AWS Oregon (Windows)
    brooklyn.location.named.AWS\ Oregon\ Win.imageId = us-west-2/ami-8fd3f9bf
    brooklyn.location.named.AWS\ Oregon\ Win.hardwareId = m3.medium
    brooklyn.location.named.AWS\ Oregon\ Win.useJcloudsSshInit=false
    brooklyn.location.named.AWS\ Oregon\ Win.templateOptions={ subnetId: subnet-a10e96c4 , securityGroupIds: [['sg-a2d0c2c7']], mapNewVolumeToDeviceName: ["/dev/sda1", 100, true]}
    ```
    
    NOTE: The subnetId and securityGroupIds are only required if a specific subnet is required (e.g. if you are to join a domain, and require a subnet where Domain Services have been configured). The mapNewVolumeToDeviceName gives a larger `C:\` drive than the default 30Gb (often needed for MS installations)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30615641
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -251,10 +258,71 @@ public T call() throws Exception {
             }
         }
     
    +    private static class SendPerfCountersToSensors implements PollHandler<WinRmToolResponse> {
    +
    +        private final EntityLocal entity;
    +        private final List<WindowsPerformanceCounterPollConfig<?>> polls;
    +
    +        public SendPerfCountersToSensors(EntityLocal entity, List<WindowsPerformanceCounterPollConfig<?>> polls) {
    +            this.entity = entity;
    +            this.polls = polls;
    +        }
    +
    +        @Override
    +        public boolean checkSuccess(WinRmToolResponse val) {
    +            if (val.getStatusCode() != 0) return false;
    +            String stderr = val.getStdErr();
    +            if (stderr == null || stderr.length() != 0) return false;
    +            String out = val.getStdOut();
    +            if (out == null || out.length() == 0) return false;
    +            return true;
    +        }
    +
    +        @Override
    +        public void onSuccess(WinRmToolResponse val) {
    +            String[] values = val.getStdOut().split("\r\n");
    +            for (int i = 0; i < polls.size(); i++) {
    +                Class<?> clazz = polls.get(i).getSensor().getType();
    +                Maybe<? extends Object> maybeValue = TypeCoercions.tryCoerce(values[i], TypeToken.of(clazz));
    --- End diff --
    
    Does use of `tryCoerce` mean that we'll ignore it if the returned value was of an unexpected format? Would seem good to log at least once to say that (e.g. if get "1.2" when we expect an integer, then we should log that somewhere sensible).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30709704
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/AbstractSoftwareProcessWinRmDriver.java ---
    @@ -0,0 +1,192 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.basic;
    +
    +import java.io.File;
    +import java.io.InputStream;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.Callable;
    +import java.util.concurrent.TimeUnit;
    +
    +import org.python.core.PyException;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.event.AttributeSensor;
    +import brooklyn.event.basic.Sensors;
    +import brooklyn.location.basic.WinRmMachineLocation;
    +import brooklyn.util.exceptions.ReferenceWithError;
    +import brooklyn.util.repeat.Repeater;
    +import brooklyn.util.time.Duration;
    +import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
    +
    +import com.google.api.client.util.Strings;
    +import com.google.common.collect.ImmutableList;
    +
    +public abstract class AbstractSoftwareProcessWinRmDriver extends AbstractSoftwareProcessDriver {
    +
    +    AttributeSensor<String> WINDOWS_USERNAME = Sensors.newStringSensor("windows.username",
    +            "Default Windows username to be used when connecting to the Entity's VM");
    +    AttributeSensor<String> WINDOWS_PASSWORD = Sensors.newStringSensor("windows.password",
    +            "Default Windows password to be used when connecting to the Entity's VM");
    +
    +    public AbstractSoftwareProcessWinRmDriver(EntityLocal entity, WinRmMachineLocation location) {
    +        super(entity, location);
    +        entity.setAttribute(WINDOWS_USERNAME, location.config().get(WinRmMachineLocation.WINDOWS_USERNAME));
    +        entity.setAttribute(WINDOWS_PASSWORD, location.config().get(WinRmMachineLocation.WINDOWS_PASSWORD));
    +    }
    +
    +    @Override
    +    public void runPreInstallCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void setup() {
    +        // Default to no-op
    +    }
    +
    +    @Override
    +    public void runPostInstallCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void runPreLaunchCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public void runPostLaunchCommand(String command) {
    +        execute(ImmutableList.of(command));
    +    }
    +
    +    @Override
    +    public WinRmMachineLocation getLocation() {
    +        return (WinRmMachineLocation)super.getLocation();
    +    }
    +
    +    @Override
    +    public String getRunDir() {
    +        // TODO: This needs to be tidied, and read from the appropriate flags (if set)
    +        return "$HOME\\brooklyn-managed-processes\\apps\\" + entity.getApplicationId() + "\\entities\\" + getEntityVersionLabel()+"_"+entity.getId();
    +    }
    +
    +    @Override
    +    public String getInstallDir() {
    +        // TODO: This needs to be tidied, and read from the appropriate flags (if set)
    +        return "$HOME\\brooklyn-managed-processes\\installs\\" + entity.getApplicationId() + "\\" + getEntityVersionLabel()+"_"+entity.getId();
    +    }
    +
    +    @Override
    +    public int copyResource(Map<Object, Object> sshFlags, String source, String target, boolean createParentDir) {
    +        if (createParentDir) {
    +            createDirectory(getDirectory(target), "Creating resource directory");
    +        }
    +        return copyTo(new File(source), new File(target));
    +    }
    +
    +    @Override
    +    public int copyResource(Map<Object, Object> sshFlags, InputStream source, String target, boolean createParentDir) {
    +        if (createParentDir) {
    +            createDirectory(getDirectory(target), "Creating resource directory");
    +        }
    +        return copyTo(source, new File(target));
    +    }
    +
    +    @Override
    +    protected void createDirectory(String directoryName, String summaryForLogging) {
    +        getLocation().executePsScript("New-Item -path \"" + directoryName + "\" -type directory -ErrorAction SilentlyContinue");
    +    }
    +
    +    protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
    +        String regularCommand = getEntity().getConfig(regularCommandKey);
    +        String powershellCommand = getEntity().getConfig(powershellCommandKey);
    +        if (Strings.isNullOrEmpty(regularCommand) && Strings.isNullOrEmpty(powershellCommand)) {
    +            if (allowNoOp) {
    +                return new WinRmToolResponse("", "", 0);
    +            } else {
    +                throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
    +            }
    +        } else if (!Strings.isNullOrEmpty(regularCommand) && !Strings.isNullOrEmpty(powershellCommand)) {
    +            throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
    +        }
    +
    +        if (Strings.isNullOrEmpty(regularCommand)) {
    +            return getLocation().executePsScript(ImmutableList.of(powershellCommand));
    +        } else {
    +            return getLocation().executeScript(ImmutableList.of(regularCommand));
    +        }
    +    }
    +
    +    public int execute(List<String> script) {
    +        return getLocation().executeScript(script).getStatusCode();
    +    }
    +
    +    public int executePowerShellNoRetry(List<String> psScript) {
    --- End diff --
    
    Why the different naming: "executePowerShell" versus "executePsScript"?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30707762
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -1729,6 +1876,27 @@ protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeServi
             }
         }
     
    +    protected WinRmMachineLocation registerWinRmMachineLocation(String vmHostname, ConfigBag setup, String nodeId) {
    +        WinRmMachineLocation winRmMachineLocation = createWinRmMachineLocation(vmHostname, setup);
    +        winRmMachineLocation.setParent(this);
    --- End diff --
    
    I wonder why we don't set the parent inside `registerJcloudsSshMachineLocation`. Ignore this comment for this PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30682920
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -622,6 +624,14 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
                 if (node == null)
                     throw new IllegalStateException("No nodes returned by jclouds create-nodes in " + setup.getDescription());
     
    +            boolean windows = node.getOperatingSystem().getFamily().equals(OsFamily.WINDOWS);
    +
    +            if (windows) {
    +                // FIXME: Tidy this up and allow for user-specified credentials
    --- End diff --
    
    I'd prefer FIXME/TODO comments to be more specific than "Tidy this up". The second part of comment is very clear. If there is more tidying than that, then please say what specifically you're thinking of.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30686856
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -795,24 +845,48 @@ protected JcloudsSshMachineLocation obtainOnce(ConfigBag setup) throws NoMachine
     
                 // Apply any optional app-specific customization.
                 for (JcloudsLocationCustomizer customizer : getCustomizers(setup)) {
    -                customizer.customize(this, computeService, sshMachineLocation);
    +                if (windows) {
    +                    LOG.warn("Ignoring customizer {} on Windows location {}", customizer, winRmMachineLocation);
    +                    // TODO: WinRm based JcloudsLocationCustomizer
    +                } else {
    +                    customizer.customize(this, computeService, jcloudsSshMachineLocation);
    +                }
                 }
     
                 customizedTimestamp = Duration.of(provisioningStopwatch);
     
    -            LOG.info("Finished VM "+setup.getDescription()+" creation:"
    -                    + " "+sshMachineLocation.getUser()+"@"+sshMachineLocation.getAddress()+":"+sshMachineLocation.getPort()
    -                    + (Boolean.TRUE.equals(setup.get(LOG_CREDENTIALS))
    -                              ? "password=" + userCredentials.getOptionalPassword().or("<absent>")
    -                                      + " && key=" + userCredentials.getOptionalPrivateKey().or("<absent>")
    -                              : "")
    -                    + " ready after "+Duration.of(provisioningStopwatch).toStringRounded()
    -                    + " ("+template+" template built in "+Duration.of(templateTimestamp).toStringRounded()+";"
    -                    + " "+node+" provisioned in "+Duration.of(provisionTimestamp).subtract(templateTimestamp).toStringRounded()+";"
    -                    + " "+sshMachineLocation+" ssh usable in "+Duration.of(usableTimestamp).subtract(provisionTimestamp).toStringRounded()+";"
    -                    + " and os customized in "+Duration.of(customizedTimestamp).subtract(usableTimestamp).toStringRounded()+" - "+Joiner.on(", ").join(customisationForLogging)+")");
    -
    -            return sshMachineLocation;
    +            String logMessage;
    +
    +            if (windows) {
    +                // TODO: More complete logging for WinRmMachineLocation
    +                // FIXME: Remove try-catch!
    +                try {
    +                    logMessage = "Finished VM " + setup.getDescription() + " creation:"
    --- End diff --
    
    Would be good to remove duplication - perhaps having a `String.format("...", ...)` with particular bits of string built differently for windows and linux.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Support for deployment to Windows

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/650#discussion_r30687606
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -1071,15 +1195,32 @@ public boolean apply(@Nullable Method input) {
                                       // can coerce to
                                       if (input == null) return false;
                                       if (!input.getName().equals(option.getKey())) return false;
    +                                  int numOptionParams = option.getValue() instanceof List ? ((List)option.getValue()).size() : 1;
                                       Type[] parameterTypes = input.getGenericParameterTypes();
    -                                  return parameterTypes.length == 1
    -                                          && TypeCoercions.tryCoerce(option.getValue(), TypeToken.of(parameterTypes[0])).isPresentAndNonNull();
    +                                  if (parameterTypes.length != numOptionParams) return false;
    --- End diff --
    
    This looks familiar for another PR (https://github.com/apache/incubator-brooklyn/pull/639) - will assume that is going to be reviewed+merged first, and this PR rebased.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---