BROOKLYN-259: Fix JcloudsByonLocationResolver #132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aledsage
changed the title
aledsage
force-pushed
the
fix/JcloudsByonLocationResolver
branch
from
bae0726
to
66be0cb
Compare
| public static final ConfigKey<Supplier<? extends List<? extends MachineLocation>>> INITIAL_MACHINES_FACTORY = ConfigKeys.newConfigKey( | ||
| new TypeToken<Supplier<? extends List<? extends MachineLocation>>>() {}, | ||
| "byon.initialMachinesFactory", | ||
| "Factory for creating the machines that should be immediatly instantiated on init", |
|
Looks good, only a few minor comments. Believe @johnmccabe is testing this with a deployment. |
|
Throwing an NPE when deploying an app to a JcloudsByonLocation that was working before this PR: Example location: Not clear why its not getting at the creds yet. |
| assertEquals(machine.getPrivateAddresses(), ImmutableSet.of(nodePrivateAddress)); | ||
|
|
||
| // TODO what user/password should we expect? Fails below because has "dummy": | ||
| // assertEquals(machine.getUser(), "myuser"); |
There was a problem hiding this comment.
This is whats throwing the NPE mentioned earlier (ie machine.getUser() returns null, previously it was these user/password values that were being used.
|
Looks like Theres no support for |
|
Spoke too soon on the location working with privateKey, throws an NPE when deploying a docker infrastructure via Clocker: |
|
@aledsage important comments from @johnmccabe need addressing before can merge |
- Previously, it created locations every time the resolver was called (even if that was just to query about what it would produce), which caused us to leak locations. - Now it defers creating the locations (though doing this with LocationSpec is not practical because of the relationship between the JcloudsLocation and the machines)! - Adds FixedListMachineProvisioningLocation.initialMachinesFactory, for deferring the machine-lookup until the byon location is really being created.
aledsage
force-pushed
the
fix/JcloudsByonLocationResolver
branch
from
66be0cb
to
a927f66
Compare
|
Rebasing fixed the problem in my test where For "NPE when deploying a docker infrastructure via Clocker", that has been fixed in a separate PR by the looks of it (certainly we guard against that NPE). See commit bee832d. I've confirmed that it works with username using @johnmccabe can you review / test again with this latest code when you get a chance? |
|
@aledsage testing this at the moment |
|
@johnmccabe did you get a chance to test this? Any comments? Can we merge it? |
|
Hi @aledsage, sorry for the delay getting back to you, the env I had originally observed this on is still inaccessible so I had to recreate, unfortunately, still seeing an NPE when attempting to deploy to the following location ( I'm going to rebuild to sanity check my install (I've built clocker against the #132 brooklyn-server branch). |
|
Rebuild appears to have done the trick, in the process of deploying a simple test app. |
|
LGTM, thanks @aledsage |
|
Great, thanks @johnmccabe - merging now! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(even if that was just to query about what it would produce), which
caused us to leak locations.
LocationSpec is not practical because of the relationship between
the JcloudsLocation and the machines)!
for deferring the machine-lookup until the byon location is really
being created.