Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
Added a jira as a reminder for the release notes - https://issues.apache.org/jira/browse/JCLOUDS-1323. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-316984706
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
BTW, thanks for the heads up @neykov! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-316954844
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
This is worth mentioning explicitly in the release notes as a breaking change. And we already have other comments. We should create a wiki page where we list them all to make sure we don't forget any when it's time to release. This said, @andreaturli is there a clean way to preserve the old behavior too? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-316954774
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
Heads up, the commit changes how security groups are set in `templateOptions`. Previously the human readable name was accepted. Now the UUID of the security group is required. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-316953433
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
merged at [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/aa11765b) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315709952
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
Closed #1117. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#event-1166125075
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
merging to master only -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315707761
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
rebuild please -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315703315
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
rebuild please -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315703226
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
thanks @nacx let's wait for the builder as extra-check. Meanwhile I'm rebasing and squashing -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315695894
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
nacx approved this pull request. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-50258245
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
>
- boolean keyPairExtensionPresent =
novaApi.getKeyPairApi(region).isPresent();
+ List inboundPorts =
Ints.asList(templateOptions.getInboundPorts());
+ if (!templateOptions.getGroups().isEmpty() && !inboundPorts.isEmpty()) {
Ops, yes, thanks
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r127638294
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
> }
+ } else if (templateOptions.getKeyPairName() != null &&
templateOptions.getLoginPrivateKey() != null) {
I was a bit unsure as well, I'll remove the extra check
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r127638157
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
nacx requested changes on this pull request.
I've just looked at the last changes. I'll run a local build too and see what
happens with that slow test.
> }
+ } else if (templateOptions.getKeyPairName() != null &&
templateOptions.getLoginPrivateKey() != null) {
Why checking also the private key here? If users configure a key pair we don't
care if they also configure the private key. Creating a node and not accessing
it is a valid use case. Perhaps users access it in another connection (where
the private key will be configured). I'd remove the private key check from here
and always set the keypair when its name is set.
>
- boolean keyPairExtensionPresent =
novaApi.getKeyPairApi(region).isPresent();
+ List inboundPorts =
Ints.asList(templateOptions.getInboundPorts());
+ if (!templateOptions.getGroups().isEmpty() && !inboundPorts.isEmpty()) {
This should be `||`.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-50241023
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
@andreaturli pushed 1 commit. 7080237 fix unit tests -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1117/files/861275890d57f1488b28984dea49cc57f4db872e..7080237a8a3c8b123e3910ea37b21bbc96a922c8
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
@nacx I think the build issue is related to the PR, need to understand why *AdapterExpectTest is soo slow now, I guess it's because the refactoring involves somehow the injector -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315544747
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
rebuild please -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315525532
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
@andreaturli pushed 1 commit. 1133ddc openstack nova re-adding support for existing keypair -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1117/files/69bcd5c36ee5c3ab83aa6a2922e4f9a75f3b7f57..1133ddc2dc1bdede6e9332b45e7ffeda6488592b
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
>if (templateOptions.getKeyPairName() != null) {
options.keyPairName(templateOptions.getKeyPairName());
- KeyPair keyPair =
keyPairCache.getIfPresent(RegionAndName.fromRegionAndName(template.getLocation().getId(),
templateOptions.getKeyPairName()));
- if (keyPair != null && keyPair.getPrivateKey() != null) {
-privateKey = Optional.of(keyPair.getPrivateKey());
-credentialsBuilder.privateKey(privateKey.get());
- }
right!
I've re-added same logic in
`ApplyNovaTemplateOptionsCreateNodesWithGroupEncodedIntoNameThenAddToSet`
thanks!
Tested with
```
templateOptions.generateKeyPair(true);
```
and with
```
templateOptions.keyPairName("test");
String privateKey = Files.toString(new
File("/Users/andrea/Downloads/test.pem"), Charsets.UTF_8);
templateOptions.overrideLoginPrivateKey(privateKey);
```
looks good to me
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r127580623
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
> - .sharedNameForGroup(group)));
- keyPairCache.asMap().put(RegionAndName.fromRegionAndName(region,
keyPair.getName()), keyPair);
- templateOptions.keyPairName(keyPair.getName());
- tagsBuilder.add(JCLOUDS_KP);
- } else if (templateOptions.getKeyPairName() != null) {
- checkArgument(keyPairExtensionPresent,
- "Key Pairs are required by options, but the extension is not
available! options: %s", templateOptions);
- if (templateOptions.getLoginPrivateKey() != null) {
-String pem = templateOptions.getLoginPrivateKey();
-KeyPair keyPair =
KeyPair.builder().name(templateOptions.getKeyPairName())
-
.fingerprint(fingerprintPrivateKey(pem)).privateKey(pem).build();
-keyPairCache.asMap().put(RegionAndName.fromRegionAndName(region,
keyPair.getName()), keyPair);
+ checkArgument(novaApi.getKeyPairApi(region).isPresent(),
+ "Key Pairs are required by options, but the extension is not
available! options: %s", templateOptions);
+ }
+ if (templateOptions.shouldGenerateKeyPair()) {
good point, changing it now
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r127580589
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
On a side note, we are changing the name of the tags we use and the life cycle of the key pairs. People could be relying on this, so I'm not sure if it is a good idea to backport this to 2.0.x. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315329090
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
nacx requested changes on this pull request.
Thanks @andreaturli! Great work here. Just a couple comments!
> - .sharedNameForGroup(group)));
- keyPairCache.asMap().put(RegionAndName.fromRegionAndName(region,
keyPair.getName()), keyPair);
- templateOptions.keyPairName(keyPair.getName());
- tagsBuilder.add(JCLOUDS_KP);
- } else if (templateOptions.getKeyPairName() != null) {
- checkArgument(keyPairExtensionPresent,
- "Key Pairs are required by options, but the extension is not
available! options: %s", templateOptions);
- if (templateOptions.getLoginPrivateKey() != null) {
-String pem = templateOptions.getLoginPrivateKey();
-KeyPair keyPair =
KeyPair.builder().name(templateOptions.getKeyPairName())
-
.fingerprint(fingerprintPrivateKey(pem)).privateKey(pem).build();
-keyPairCache.asMap().put(RegionAndName.fromRegionAndName(region,
keyPair.getName()), keyPair);
+ checkArgument(novaApi.getKeyPairApi(region).isPresent(),
+ "Key Pairs are required by options, but the extension is not
available! options: %s", templateOptions);
+ }
+ if (templateOptions.shouldGenerateKeyPair()) {
Should we check instead if the user configured inbound ports?
I see that the groups are just passed through to the server options, so n need
for the extension api if the user only sets the security group names?
>if (templateOptions.getKeyPairName() != null) {
options.keyPairName(templateOptions.getKeyPairName());
- KeyPair keyPair =
keyPairCache.getIfPresent(RegionAndName.fromRegionAndName(template.getLocation().getId(),
templateOptions.getKeyPairName()));
- if (keyPair != null && keyPair.getPrivateKey() != null) {
-privateKey = Optional.of(keyPair.getPrivateKey());
-credentialsBuilder.privateKey(privateKey.get());
- }
We need this code. Users using an existing, pre-created keypair won't be able
to login to the node since the private key won't be populated to the login
credentials.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-50014187
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
geomacy approved this pull request. LGTM at a quick read. Still have a feeling it shouldn't throw exceptions if `destroyNode` fails but will defer to you on this. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-49209337
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
@andreaturli pushed 1 commit. 848125a rename cleanupServer to cleanupResources -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1117/files/c1eacfc000239aff5b0070a6a0302d75294dc0e3..848125a39600e2d9e4d791e32b988b73662c33a9
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
geomacy commented on this pull request.
> @@ -95,6 +101,7 @@ public Boolean apply(String id) {
}
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
+ checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in the configured timeout");
No real opinion on that to be honest! I would say though, that if the
`cleanupServer.apply()` is being invoked as per the link above (inside
`cleanUpIncidentalResourcesOfDeadNodes`), then you probably want to avoid the
exception here, as otherwise the first exception when iterating over
`deadNodes` will prevent any remaining nodes even being attempted.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r126630432
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
nacx approved this pull request. > @@ -165,6 +169,14 @@ protected TemplateOptions > provideTemplateOptions(Injector injector, TemplateOpti } @Provides + @com.google.inject.name.Named(TIMEOUT_NODE_TERMINATED) + protected Predicate provideDropletTerminatedPredicate(final NovaApi api, ComputeServiceConstants.Timeouts timeouts, Rename method to `provideServerTerminatedPredicate`. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-48611247
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
> @@ -95,6 +101,7 @@ public Boolean apply(String id) {
}
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
+ checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in the configured timeout");
Thinking about it more, I guess `destroyNode` should not invoke `CleanUpServer`
but simply
```
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in the configured timeout");
```
as `CleanUpServer` will be invoked by
https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeService.java#L99
anyways. I'll test it, meanwhile wdyt?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r126122879
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
> @@ -95,6 +101,7 @@ public Boolean apply(String id) {
}
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
+ checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in the configured timeout");
good question @geomacy, maybe a warning is enough
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r126121858
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
geomacy commented on this pull request.
> @@ -95,6 +101,7 @@ public Boolean apply(String id) {
}
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
+ checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in the configured timeout");
Is it desirable to throw an exception here, or should we just log a warning?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-48575884
[jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
- wait for server deletion, before deleting the security group I'd like this to be backported to 2.0.x as well You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds/pull/1117 -- Commit Summary -- * [JCLOUDS-1318] fix based on nodeTerminatePredicate -- File Changes -- M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/config/NovaComputeServiceContextModule.java (30) M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/functions/CleanupServer.java (11) -- Patch Links -- https://github.com/jclouds/jclouds/pull/1117.patch https://github.com/jclouds/jclouds/pull/1117.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117
