You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2015/12/02 17:58:46 UTC
[GitHub] incubator-brooklyn pull request: Jclouds machines: do not persist ...
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1082
Jclouds machines: do not persist NodeMetadata or Template
Work in progress:
* requires the same to be done for `JcloudsWinrmMachineLocation`
* requires more testing for backwards compatibility (see the unit test included here; but needs tested in the wild).
Previously JcloudsSshMachineLocation would persist the `NodeMetadata` and the `Template`, which pulled in lots of jclouds stuff. This was very brittle: if the NodeMetadata fields changed, or something like `AzureComputeTemplateOptions` fields changed, then rebind could fail.
This changes `JcloudsSshMachineLocation` to not persist node and template. Instead it persists the nodeId + imageId + privateAddresses + publicAddresses + hostname (i.e. the things in NodeMetadata that are most important).
For backwards compatibility, if on rebind it finds the node in the persisted state then it extracts the fields we need, sets those, and then sets the `node` field to null. Same for the `template` field.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn jclouds-do-not-persist-NodeMetadata
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1082.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 #1082
----
commit 353920b76b28511f5c91ea4256e7b369063bf33b
Author: Aled Sage <al...@gmail.com>
Date: 2015-10-21T19:41:50Z
jclouds machine: don’t persist node+template
commit 999f6140dfb7da3f6ab8df8f4cdb84a4b83f1bc2
Author: Aled Sage <al...@gmail.com>
Date: 2015-12-02T16:33:58Z
Fix SshMachineLocation.getSshHostAndPort
----
---
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: Jclouds machines: do not persist ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1082#issuecomment-163554619
+1 ... look forward to having this merged!
---
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: Jclouds machines: do not persist ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1082#discussion_r46522492
--- Diff: locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/RebindJcloudsLocationLiveTest.java ---
@@ -94,6 +101,100 @@ public void testRebindsToJcloudsMachine() throws Exception {
assertJcloudsLocationEquals(newJcloudsLoction, origJcloudsLocation);
}
+ // TODO In jclouds-azure, the AzureComputeTemplateOptions fields changed, which meant old
+ // persisted state could not be deserialized. These files are examples of the old format.
+ @Test(groups={"Live", "WIP"}, enabled=false)
+ public void testRebindsToJcloudsMachineWithInvalidTemplate() throws Exception {
+ ResourceUtils resourceUtils = ResourceUtils.create(this);
+ FileUtils.write(
+ new File(mementoDir, "locations/briByOel"),
+ resourceUtils.getResourceAsString("classpath://org/apache/brooklyn/location/jclouds/persisted-azure-parent-briByOel"));
+ FileUtils.write(
+ new File(mementoDir, "locations/VNapYjwp"),
+ resourceUtils.getResourceAsString("classpath://org/apache/brooklyn/location/jclouds/persisted-azure-machine-VNapYjwp"));
+
+ TestApplication newApp = rebind();
+
+ JcloudsLocation loc = (JcloudsLocation) newApp.getManagementContext().getLocationManager().getLocation("briByOel");
+ JcloudsSshMachineLocation machine = (JcloudsSshMachineLocation) newApp.getManagementContext().getLocationManager().getLocation("VNapYjwp");
+ assertEquals(ImmutableSet.of(loc.getChildren()), ImmutableSet.of(machine));
+ }
+
+ @Test(groups="Live")
+ public void testRebindsToJcloudsMachineWithTemplateAndNode() throws Exception {
+ // Populate the mementoDir with some old-style peristed state
+ ResourceUtils resourceUtils = ResourceUtils.create(this);
+ String origParentXml = resourceUtils.getResourceAsString("classpath://org/apache/brooklyn/location/jclouds/persisted-aws-parent-lCYB3mTb");
+ String origMachineXml = resourceUtils.getResourceAsString("classpath://org/apache/brooklyn/location/jclouds/persisted-aws-machine-aKEcbxKN");
+ File persistedParentFile = new File(mementoDir, "locations/lCYB3mTb");
+ File persistedMachineFile = new File(mementoDir, "locations/aKEcbxKN");
+ FileUtils.write(
+ persistedParentFile,
+ origParentXml);
+ FileUtils.write(
+ persistedMachineFile,
+ origMachineXml);
+
+ assertTrue(origMachineXml.contains("AWSEC2TemplateOptions"), origMachineXml);
+ assertTrue(origMachineXml.contains("NodeMetadataImpl"), origMachineXml);
+
+ // Rebind to the old-style persisted state, which includes the NodeMetadata and the Template objects.
+ // Expect to parse that ok.
+ TestApplication app2 = rebind();
+
+ JcloudsLocation loc2 = (JcloudsLocation) app2.getManagementContext().getLocationManager().getLocation("lCYB3mTb");
+ JcloudsSshMachineLocation machine2 = (JcloudsSshMachineLocation) app2.getManagementContext().getLocationManager().getLocation("aKEcbxKN");
+ assertEquals(ImmutableSet.copyOf(loc2.getChildren()), ImmutableSet.of(machine2));
+
+ String errmsg = "loc="+loc2.toVerboseString()+"; machine="+machine2.toVerboseString();
+ assertEquals(machine2.getId(), "aKEcbxKN", errmsg);
+ assertEquals(machine2.getJcloudsId(), "ap-southeast-1/i-56fd53f2", errmsg);
+ assertEquals(machine2.getSshHostAndPort(), HostAndPort.fromParts("ec2-54-254-23-53.ap-southeast-1.compute.amazonaws.com", 22), errmsg);
+ assertEquals(machine2.getPrivateAddresses(), ImmutableSet.of("10.144.66.5"), errmsg);
+ assertEquals(machine2.getPublicAddresses(), ImmutableSet.of("54.254.23.53"), errmsg);
+ assertEquals(machine2.getPrivateAddress(), Optional.of("10.144.66.5"), errmsg);
+ assertEquals(machine2.getHostname(), "ip-10-144-66-5", errmsg); // TODO would prefer the hostname that works inside and out
+ assertEquals(machine2.getOsDetails().isWindows(), false, errmsg);
+ assertEquals(machine2.getOsDetails().isLinux(), true, errmsg);
+ assertEquals(machine2.getOsDetails().isMac(), false, errmsg);
+ assertEquals(machine2.getOsDetails().getName(), "centos", errmsg);
+ assertEquals(machine2.getOsDetails().getArch(), "x86_64", errmsg);
+ assertEquals(machine2.getOsDetails().getVersion(), "6.5", errmsg);
+ assertEquals(machine2.getOsDetails().is64bit(), true, errmsg);
+
+ // Force it to be persisted again. Expect to pesist without the NodeMetadata and Tempalte.
--- End diff --
typo `Tempalte`
---
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: Jclouds machines: do not persist ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1082#issuecomment-165437369
looks good
---
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: Jclouds machines: do not persist ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1082
---
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: Jclouds machines: do not persist ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1082#issuecomment-165156285
@neykov @ahgittin I've done various updates (including getting this working for `JcloudsWinRmMachineLocation`) and fixed some live tests. Can you take another look 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: Jclouds machines: do not persist ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1082#discussion_r46521913
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocation.java ---
@@ -146,24 +264,48 @@ public JcloudsLocation getParent() {
@Override
public String getHostname() {
- return node.getHostname();
+ return hostname;
}
- /** In most clouds, the public hostname is the only way to ensure VMs in different zones can access each other. */
+ /** In clouds like AWS, the public hostname is the only way to ensure VMs in different zones can access each other. */
@Override
public Set<String> getPublicAddresses() {
- return node.getPublicAddresses();
+ return (publicAddresses == null) ? ImmutableSet.<String>of() : publicAddresses;
}
@Override
public Set<String> getPrivateAddresses() {
- return node.getPrivateAddresses();
+ return (privateAddresses == null) ? ImmutableSet.<String>of() : privateAddresses;
}
@Override
public String getSubnetHostname() {
- String privateHostname = jcloudsParent.getPrivateHostname(node, Optional.<HostAndPort>absent(), config().getBag());
- return privateHostname;
+ if (_privateHostname == null) {
+ Optional<NodeMetadata> node = getOptionalNode();
+ if (node.isPresent()) {
+ // TODO Could infer LoginCredentials in the same way as super.connectSsh().
+ // But the code below falls back to extracting it from privateAddress/publicAddress anyway.
+ _privateHostname = jcloudsParent.getPrivateHostname(node.get(), Optional.of(getSshHostAndPort()), config().getBag());
+ } else {
+ // TODO Duplication of some of jcloudsParent.getPrivateHostnameGeneric(NodeMetadata, ConfigBag).
+ //
+ //prefer the private address to the hostname because hostname is sometimes wrong/abbreviated
+ //(see that javadoc; also e.g. on rackspace/cloudstack, the hostname is not registered with any DNS).
+ //Don't return local-only address (e.g. never 127.0.0.1)
+ for (String p : getPrivateAddresses()) {
+ if (Networking.isLocalOnly(p)) continue;
+ _privateHostname = p;
+ }
+ if (groovyTruth(getPublicAddresses())) {
+ _privateHostname = getPublicAddresses().iterator().next();
+ } else if (groovyTruth(getHostname())) {
+ _privateHostname = getHostname();
+ } else {
+ return null;
+ }
+ }
+ }
--- End diff --
I think having a single code path might be better here, i.e. have only the fallback? Otherwise could be surprised by different behaviour in some edge cases.
---
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: Jclouds machines: do not persist ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1082#issuecomment-161546685
Really happy to see so much brittle state removed from persistence. Looks good so far.
---
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.
---