Skip to content

External ip config for winrm byon location #690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
ivanayov wants to merge 4 commits into
base: master
Choose a base branch
from
Open

External ip config for winrm byon location #690

ivanayov wants to merge 4 commits into from
+122 −55

Conversation

@ivanayov
Copy link
Contributor

@ivanayov ivanayov commented May 21, 2017

No description provided.

bostko
bostko reviewed May 22, 2017
View reviewed changes
core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java Outdated
int port;
if (ssh != null) {
userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
Copy link
Contributor

@bostko bostko May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
@iyovcheva what do you think about supporting external provider for ssh address as well. It will be more consistent.

Copy link
Contributor Author

@ivanayov ivanayov May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bostko I agree. Will need to test it.

Copy link
Contributor Author

@ivanayov ivanayov May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External config is not supported for ssh. If we implement it, it's good to go in separate PR.

Copy link
Contributor

@geomacy geomacy May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!(machineConfig.get("winrm") instanceof String) might be better as machineConfig.get("winrm") instanceof DeferredSupplier?

@ivanayov ivanayov force-pushed the feature/external-ip branch from 4ec5eb2 to 7bffcd6 Compare May 23, 2017 08:54
neykov
neykov reviewed May 25, 2017
View reviewed changes
core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java Outdated
int port;
if (ssh != null) {
userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
Copy link
Member

@neykov neykov May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iyovcheva The changes here break the support for specifying winrm as user@host:port. Can you add it to the test cases.
On the other hand your approach is also interesting because it doesn't resolve the value here - at the time of deploying the blueprint. Obtaining the machine could happen much later in the lifecycle where the external values have already changed.

A proper solution would involve moving all of non-essential parsing to the machine location, leaving only minimal sanity checks here.

geomacy
geomacy reviewed May 25, 2017
View reviewed changes
Copy link
Contributor

@geomacy geomacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you only wanting to support external for winrm? If you're adding external support for the address why not do it for ssh too?

.../test/java/org/apache/brooklyn/camp/brooklyn/WinRmMachineLocationExternalConfigYamlTest.java Outdated
waitForApplicationTasks(app);
Asserts.shouldHaveFailedPreviously();
} catch (Exception e) {
Asserts.expectedFailureOfType(e, NullPointerException.class);
Copy link
Contributor

@geomacy geomacy May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's bad if we are throwing null pointer exceptions - these should be caught lower in the code and at least turned into something more useful with a useful error message that you can then test in here. For example use Preconditions.checkArgument on whatever is throwing the NPE.

core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java Outdated
userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
} else {
// TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
Copy link
Contributor

@geomacy geomacy May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a variable here to store the result of the logical expression, it will make it a lot easier to read.

The second clause should be Boolean.parseBoolean(vals.get("winrm.useHttps").toString()).

core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java Outdated
machineConfig.put("port", port);
// If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
port = userAndHostAndPort.getHostAndPort().getPort();
if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
Copy link
Contributor

@geomacy geomacy May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do this if in a separate method, this one is long enough already!

core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java Outdated
machineConfig.put("address", host);
try {
InetAddress.getByName(host);
} catch (Exception e) {
Copy link
Contributor

@geomacy geomacy May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnknownHostException

core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java Outdated
int port;
if (ssh != null) {
userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
Copy link
Contributor

@geomacy geomacy May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!(machineConfig.get("winrm") instanceof String) might be better as machineConfig.get("winrm") instanceof DeferredSupplier?

@geomacy
Copy link
Contributor

geomacy commented Jul 24, 2017

hi @iyovcheva just checking over brooklyn-server open PRs, what's the latest on this one? Do you want to update it as @neykov mentioned above?

@ivanayov
Copy link
Contributor Author

ivanayov commented Aug 1, 2017

@geomacy yep, I'm currently working on it

@ivanayov ivanayov force-pushed the feature/external-ip branch from 39a9cc1 to 58847c1 Compare August 4, 2017 06:28
@ivanayov ivanayov force-pushed the feature/external-ip branch from 30dd296 to 946c0de Compare August 7, 2017 12:02
@ivanayov
Copy link
Contributor Author

ivanayov commented Aug 7, 2017

retest this please

@ivanayov
Copy link
Contributor Author

ivanayov commented Aug 7, 2017

Failing with "cannot create temporary folders in parent /tmp after 1000 attempts" which is not related to the PR

@tbouron
Copy link
Member

tbouron commented Dec 28, 2017

retest this please

1 similar comment
@nakomis
Copy link
Contributor

nakomis commented Nov 26, 2019

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@geomacy geomacy geomacy left review comments

+2 more reviewers

@bostko bostko bostko left review comments

@neykov neykov neykov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ivanayov @geomacy @tbouron @nakomis @bostko @neykov