[JCLOUDS-1318] fix based on nodeTerminatePredicate #1117

andreaturli · 2017-07-07T09:29:21Z

wait for server deletion, before deleting the security group

I'd like this to be backported to 2.0.x as well

geomacy · 2017-07-07T10:52:27Z

...tack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/functions/CleanupServer.java

@@ -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?

good question @geomacy, maybe a warning is enough

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?

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.

nacx · 2017-07-07T13:51:54Z

...ain/java/org/jclouds/openstack/nova/v2_0/compute/config/NovaComputeServiceContextModule.java

@@ -165,6 +169,14 @@ protected TemplateOptions provideTemplateOptions(Injector injector, TemplateOpti
   }

   @Provides
+   @com.google.inject.name.Named(TIMEOUT_NODE_TERMINATED)
+   protected Predicate<String> provideDropletTerminatedPredicate(final NovaApi api, ComputeServiceConstants.Timeouts timeouts,


Rename method to provideServerTerminatedPredicate.

geomacy

LGTM at a quick read. Still have a feeling it shouldn't throw exceptions if destroyNode fails but will defer to you on this.

nacx

Thanks @andreaturli! Great work here. Just a couple comments!

nacx · 2017-07-14T10:31:02Z

...ompute/strategy/ApplyNovaTemplateOptionsCreateNodesWithGroupEncodedIntoNameThenAddToSet.java

+         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?

good point, changing it now

nacx · 2017-07-14T10:36:23Z

...ck-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/NovaComputeServiceAdapter.java

-         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.

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

nacx · 2017-07-14T10:38:17Z

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.

andreaturli · 2017-07-15T10:34:20Z

rebuild please

andreaturli · 2017-07-15T16:17:16Z

@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

nacx

I've just looked at the last changes. I'll run a local build too and see what happens with that slow test.

nacx · 2017-07-17T06:07:47Z

...ompute/strategy/ApplyNovaTemplateOptionsCreateNodesWithGroupEncodedIntoNameThenAddToSet.java

         }
+      } 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.

I was a bit unsure as well, I'll remove the extra check

nacx · 2017-07-17T06:11:42Z

...ompute/strategy/ApplyNovaTemplateOptionsCreateNodesWithGroupEncodedIntoNameThenAddToSet.java


-      boolean keyPairExtensionPresent = novaApi.getKeyPairApi(region).isPresent();
+      List<Integer> inboundPorts = Ints.asList(templateOptions.getInboundPorts());
+      if (!templateOptions.getGroups().isEmpty() && !inboundPorts.isEmpty()) {


This should be ||.

Ops, yes, thanks

- wait for serer deletion, before deleting the security group - rename cleanupServer to cleanupResources - remove keyPairCache - better usage of tags to remove securityGroups created by jclouds - remove keyPair after the creation of a group - remove test for create unique keypair - openstack nova re-adding support for existing keypair - fix securityGroupApi check - fix other unit tests - remove ServerPredicates as it is now duplicated - remove TemplateOptions.securityGroupNames as deprecated - address commits for ApplyNovaTemplateOptionsCreatesNodesWithGroupEncodedIntoNameAndAddToSet - fix testCreateNodeWhileUserSpecifiesKeyPairAndUserSpecifiedGroups

andreaturli · 2017-07-17T08:34:56Z

thanks @nacx let's wait for the builder as extra-check.
Meanwhile I'm rebasing and squashing

andreaturli · 2017-07-17T09:12:13Z

rebuild please

andreaturli · 2017-07-17T09:31:42Z

merging to master only

andreaturli · 2017-07-17T09:42:26Z

merged at master

neykov · 2017-07-21T09:29:28Z

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.

nacx · 2017-07-21T09:35:18Z

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?

nacx · 2017-07-21T09:35:35Z

BTW, thanks for the heads up @neykov!

neykov · 2017-07-21T12:09:42Z

Added a jira as a reminder for the release notes - https://issues.apache.org/jira/browse/JCLOUDS-1323.

geomacy reviewed Jul 7, 2017

View reviewed changes

nacx approved these changes Jul 7, 2017

View reviewed changes

geomacy approved these changes Jul 11, 2017

View reviewed changes

andreaturli force-pushed the fix/openstack-nova-clean-server branch from 848125a to 69bcd5c Compare July 11, 2017 14:26

nacx suggested changes Jul 14, 2017

View reviewed changes

andreaturli force-pushed the fix/openstack-nova-clean-server branch from 1133ddc to 8612758 Compare July 15, 2017 09:26

nacx suggested changes Jul 17, 2017

View reviewed changes

andreaturli force-pushed the fix/openstack-nova-clean-server branch 2 times, most recently from 9f90964 to 6fa492c Compare July 17, 2017 08:26

nacx approved these changes Jul 17, 2017

View reviewed changes

andreaturli force-pushed the fix/openstack-nova-clean-server branch from 6fa492c to 5f92bf3 Compare July 17, 2017 08:34

andreaturli closed this Jul 17, 2017

andreaturli mentioned this pull request Aug 6, 2017

JCLOUDS-1323: use security group names in openstack nova template options #1128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JCLOUDS-1318] fix based on nodeTerminatePredicate #1117

[JCLOUDS-1318] fix based on nodeTerminatePredicate #1117

andreaturli commented Jul 7, 2017

geomacy Jul 7, 2017

andreaturli Jul 7, 2017

andreaturli Jul 7, 2017

geomacy Jul 11, 2017

nacx Jul 7, 2017

geomacy left a comment

nacx left a comment

nacx Jul 14, 2017

andreaturli Jul 15, 2017

nacx Jul 14, 2017

andreaturli Jul 15, 2017

nacx commented Jul 14, 2017

andreaturli commented Jul 15, 2017

andreaturli commented Jul 15, 2017

nacx left a comment

nacx Jul 17, 2017

andreaturli Jul 17, 2017

nacx Jul 17, 2017

andreaturli Jul 17, 2017

andreaturli commented Jul 17, 2017

andreaturli commented Jul 17, 2017

andreaturli commented Jul 17, 2017

andreaturli commented Jul 17, 2017

neykov commented Jul 21, 2017

nacx commented Jul 21, 2017

nacx commented Jul 21, 2017

neykov commented Jul 21, 2017

		}
		} else if (templateOptions.getKeyPairName() != null && templateOptions.getLoginPrivateKey() != null) {

Navigation Menu

[JCLOUDS-1318] fix based on nodeTerminatePredicate #1117

[JCLOUDS-1318] fix based on nodeTerminatePredicate #1117

Conversation

andreaturli commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geomacy left a comment

Choose a reason for hiding this comment

nacx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nacx commented Jul 14, 2017

andreaturli commented Jul 15, 2017

andreaturli commented Jul 15, 2017

nacx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaturli commented Jul 17, 2017

andreaturli commented Jul 17, 2017

andreaturli commented Jul 17, 2017

andreaturli commented Jul 17, 2017

neykov commented Jul 21, 2017

nacx commented Jul 21, 2017

nacx commented Jul 21, 2017

neykov commented Jul 21, 2017