Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
thanks @nacx for your help and review -- 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/1175#issuecomment-362618718
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
merging it -- 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/1175#issuecomment-362617035
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
Closed #1175. -- 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/1175#event-1455308130
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
merged at [master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;h=09936b5) -- 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/1175#issuecomment-362618900
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
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/1175#pullrequestreview-93641279
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
nacx commented on this pull request.
> - @Provides
- @Singleton
- @Named("SECURITYGROUP_PRESENT")
- protected final Predicate>
securityGroupEventualConsistencyDelay(
-FindSecurityGroupWithNameAndReturnTrue in,
-@Named(TIMEOUT_SECURITYGROUP_PRESENT) long msDelay) {
- return retry(in, msDelay, 100L, MILLISECONDS);
- }
+// @Provides
+// @Singleton
+// @Named("SECURITYGROUP_PRESENT")
+// protected final Predicate>
securityGroupEventualConsistencyDelay(
+//FindSecurityGroupWithNameAndReturnTrue in,
+//@Named(TIMEOUT_SECURITYGROUP_PRESENT) long msDelay) {
+// return retry(in, msDelay, 100L, MILLISECONDS);
+// }
Remove the commented code.
> + * builder.ipPermissions(filter(transform(group.getRules(), new
+ * Function() {
+ *
+ * @Override public IpPermission apply(SecurityGroupRule from) { if
(from.g() ==
+ * RuleDirection.EGRESS) return null; IpPermission.Builder builder =
+ * IpPermission.builder(); if (from.getIpProtocol() != null) {
+ * builder.ipProtocol(from.getIpProtocol()); } else {
+ * builder.ipProtocol(IpProtocol.TCP); } // if (from.getFromPort() !=
null)
+ * builder.fromPort(from.getFromPort()); // if (from.getToPort() != null)
+ * builder.toPort(from.getToPort()); if (from.getRemoteGroupId() !=
null) {
+ * builder.groupId(regionId + "/" + from.getGroup()); } else if
+ * (from.getRemoteIpPrefix() != null){
+ * builder.cidrBlock(from.getRemoteIpPrefix()); }
+ *
+ * return builder.build(); } }), Predicates.notNull())); }
+ */
Remove the commented code.
> + * the same tenant-id-and-name as that of ruleGroup then it is not
> possible with
+ * the Nova /v2/12345/os-security-groups API to know which group is
intended, as
+ * the only information it returns about referred groups is the tenant
id and
+ * name: "group": { "tenant_id": "a0ade3ca76784719845363979dc1014e",
"name":
+ * "jclouds-qa-scheduler-docker-entity" }, Rather than pick one group at
random
+ * and risk using the wrong group, here we fall back to the least-worst
+ * option(?) and refuse to add any rule.
+ *
+ * See https://issues.apache.org/jira/browse/JCLOUDS-1234.
+ *
+ * if (referredGroup.size() != 1) { logger.
+ * warn("Ambiguous group %s used in security rule, refusing to add it to
%s (%s)"
+ * , ruleGroup, owningGroup.getName(), owningGroup.getId()); return
null; }
+ * builder.groupId(group.getRegion() + "/" +
+ * Iterables.getOnlyElement(referredGroup).getId()); }
+ */
Remove the commented code.
> }
- templateOptions.securityGroups(securityGroupName);
- tagsBuilder.add(String.format("%s-%s", JCLOUDS_SG_PREFIX,
securityGroupInRegion.getSecurityGroup().getId()));
+
+ if (securityGroup == null) {
At this point this is always null. Remove the dedundant `if` and declare the
variable here.
--
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/1175#pullrequestreview-93589738
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@andreaturli pushed 1 commit. ff4a725 simplify securitygroup cache -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1175/files/753638f986a376e6abd94ccc57ea35bad91a79af..ff4a7257f537a28788cfdf3d0956808a4115c4c2
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@andreaturli pushed 1 commit. 753638f fix listSecurityGroupsForNode -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1175/files/4e0b06b2510001ccd9a83f3166989d6119255f46..753638f986a376e6abd94ccc57ea35bad91a79af
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
nacx commented on this pull request.
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set listSecurityGroupsForNode(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+ final
FluentIterable allGroups
= securityGroupApi.listSecurityGroups().concat();
+ Location location = locationIndex.get().get(region);
+ return ImmutableSet.copyOf(transform(filter(allGroups, notNull()),
neutronSecurityGroupToSecurityGroup.create(location)));
+ }
+1 to use this. I'd say we need it before merging the PR. Agree?
--
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/1175#discussion_r165030128
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
andreaturli commented on this pull request.
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set listSecurityGroupsForNode(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+ final
FluentIterable allGroups
= securityGroupApi.listSecurityGroups().concat();
+ Location location = locationIndex.get().get(region);
+ return ImmutableSet.copyOf(transform(filter(allGroups, notNull()),
neutronSecurityGroupToSecurityGroup.create(location)));
+ }
I think we should use
https://developer.openstack.org/api-ref/compute/#servers-security-groups-servers-os-security-groups
--
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/1175#discussion_r165012937
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
andreaturli commented on this pull request.
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set listSecurityGroupsForNode(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+ final
FluentIterable allGroups
= securityGroupApi.listSecurityGroups().concat();
+ Location location = locationIndex.get().get(region);
+ return ImmutableSet.copyOf(transform(filter(allGroups, notNull()),
neutronSecurityGroupToSecurityGroup.create(location)));
+ }
good point! thanks
unfortunately also `NovaSecurityGroupExtension` is not really reliable as it is
built on
`Optional serverWithSecurityGroupsApi =
api.getServerWithSecurityGroupsApi(region)` which is most of the time not
working on recent Openstack installations (they claim the extension is there
but it is not working!)
We may need to radically change the implementation but not sure how yet
--
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/1175#discussion_r165008461
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@nacx here's the result of `NeutronSecurityGroupExtensionLiveTest` against a provider with keystone v3 and Neutron support ``` --- T E S T S --- Running org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest Configuring TestNG with: TestNG652Configurator Starting test testCreateSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) Starting test testSecurityGroupCacheInvalidatedWhenDeletedExternally(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) Starting test testSecurityGroupCacheInvalidated(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-1] Test testCreateSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 10013ms Test suite progress: tests succeeded: 1, failed: 0, skipped: 0. Starting test testGetSecurityGroupById(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-4] Test testGetSecurityGroupById(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 560ms Test suite progress: tests succeeded: 2, failed: 0, skipped: 0. Starting test testAddIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-5] Test testAddIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 2908ms Test suite progress: tests succeeded: 3, failed: 0, skipped: 0. Starting test testRemoveIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-3] Test testSecurityGroupCacheInvalidatedWhenDeletedExternally(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 16488ms Test suite progress: tests succeeded: 4, failed: 0, skipped: 0. [pool-2-thread-2] Test testSecurityGroupCacheInvalidated(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 16835ms Test suite progress: tests succeeded: 5, failed: 0, skipped: 0. [pool-2-thread-5] Test testRemoveIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 3464ms Test suite progress: tests succeeded: 6, failed: 0, skipped: 0. Starting test testAddIpPermissionsFromSpec(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-5] Test testAddIpPermissionsFromSpec(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 2700ms Test suite progress: tests succeeded: 7, failed: 0, skipped: 0. Starting test testAddIpPermissionWithCidrExclusionGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-5] Test testAddIpPermissionWithCidrExclusionGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) skipped. Test suite progress: tests succeeded: 7, failed: 0, skipped: 1. [pool-2-thread-5] Test testRemoveIpPermissionWithCidrExclusionGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) skipped. Test suite progress: tests succeeded: 7, failed: 0, skipped: 2. Starting test testDeleteSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) [pool-2-thread-5] Test testDeleteSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest) succeeded: 1202ms Test suite progress: tests succeeded: 8, failed: 0, skipped: 2. Tests run: 10, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 22.939 sec - in org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest Results : Tests run: 10, Failures: 0, Errors: 0, Skipped: 2 ``` -- 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/1175#issuecomment-361878277
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
nacx requested changes on this pull request.
I've debugged the failure and it turns to be a test concurrency issue with the
two tests that verify the behavior of the SG cache. If you apply this patch to
make sure both tests don't use the same security group, tests should pass:
```diff
diff --git
a/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java
b/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java
index 2aa7e66c87..5c39bcb783 100644
---
a/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java
+++
b/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java
@@ -406,17 +406,18 @@ public abstract class BaseSecurityGroupExtensionLiveTest
extends BaseComputeServ
@Test(groups = {"integration", "live"}, singleThreaded = true)
public void testSecurityGroupCacheInvalidatedWhenDeletedExternally() throws
Exception {
+ String testSecurityGroupName = secGroupNameToDelete + "-externally";
ComputeService computeService = view.getComputeService();
Optional securityGroupExtension =
computeService.getSecurityGroupExtension();
assertTrue(securityGroupExtension.isPresent(), "security extension was
not present");
final SecurityGroupExtension security = securityGroupExtension.get();
- final SecurityGroup seedGroup =
security.createSecurityGroup(secGroupNameToDelete,
getNodeTemplate().getLocation());
+ final SecurityGroup seedGroup =
security.createSecurityGroup(testSecurityGroupName,
getNodeTemplate().getLocation());
deleteSecurityGroupFromAnotherView(seedGroup);
boolean deleted = security.removeSecurityGroup(seedGroup.getId());
assertFalse(deleted, "SG deleted externally so should've failed
deletion");
- final SecurityGroup recreatedGroup =
security.createSecurityGroup(secGroupNameToDelete,
getNodeTemplate().getLocation());
+ final SecurityGroup recreatedGroup =
security.createSecurityGroup(testSecurityGroupName,
getNodeTemplate().getLocation());
// Makes sure the security group exists and is re-created and is not
just returned from cache
security.addIpPermission(IpPermission.builder()
```
> + return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set listSecurityGroupsForNode(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+ final
FluentIterable allGroups
= securityGroupApi.listSecurityGroups().concat();
+ Location location = locationIndex.get().get(region);
+ return ImmutableSet.copyOf(transform(filter(allGroups, notNull()),
neutronSecurityGroupToSecurityGroup.create(location)));
+ }
This implementation is still wrong. You have to filter ONLY the security groups
associated with the given node. You're returning all existing ones.
--
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/1175#pullrequestreview-92833226
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
here's my live tests results: - for `NovaSecurityGroupExtension` ``` --- T E S T S --- Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=256m; support was removed in 8.0 Running org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest Configuring TestNG with: TestNG652Configurator Starting test testCreateSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testCreateSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 2618ms Test suite progress: tests succeeded: 1, failed: 0, skipped: 0. Starting test testSecurityGroupCacheInvalidated(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testSecurityGroupCacheInvalidated(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 5762ms Test suite progress: tests succeeded: 2, failed: 0, skipped: 0. Starting test testListSecurityGroups(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) INFO: Loading security groups INFO: Found 4 security groups INFO: Adding security group 1806862 INFO: Loading security groups INFO: Found 5 security groups INFO: Removing jclouds-1806862 INFO: Loading security groups INFO: Found 4 security groups [TestNG] Test testListSecurityGroups(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 3267ms Test suite progress: tests succeeded: 3, failed: 0, skipped: 0. Starting test testSecurityGroupCacheInvalidatedWhenDeletedExternally(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testSecurityGroupCacheInvalidatedWhenDeletedExternally(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 6281ms Test suite progress: tests succeeded: 4, failed: 0, skipped: 0. Starting test testGetSecurityGroupById(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testGetSecurityGroupById(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 474ms Test suite progress: tests succeeded: 5, failed: 0, skipped: 0. Starting test testAddIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testAddIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 2004ms Test suite progress: tests succeeded: 6, failed: 0, skipped: 0. Starting test testRemoveIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testRemoveIpPermission(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 2080ms Test suite progress: tests succeeded: 7, failed: 0, skipped: 0. Starting test testAddIpPermissionsFromSpec(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testAddIpPermissionsFromSpec(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 2295ms Test suite progress: tests succeeded: 8, failed: 0, skipped: 0. Starting test testAddIpPermissionWithCidrExclusionGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testAddIpPermissionWithCidrExclusionGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) skipped. Test suite progress: tests succeeded: 8, failed: 0, skipped: 1. [TestNG] Test testRemoveIpPermissionWithCidrExclusionGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) skipped. Test suite progress: tests succeeded: 8, failed: 0, skipped: 2. Starting test testDeleteSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) [TestNG] Test testDeleteSecurityGroup(org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest) succeeded: 718ms Test suite progress: tests succeeded: 9, failed: 0, skipped: 2. Tests run: 11, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 27.195 sec - in org.jclouds.openstack.nova.v2_0.compute.extensions.NovaSecurityGroupExtensionLiveTest Results : Tests run: 11, Failures: 0, Errors: 0, Skipped: 2 ``` and for `NeutronSecurityGroupExtensions` with Neutron-context attached ``` --- T E S T S --- Running org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtensionLiveTest Configuring TestNG with: TestNG652Configurator Starting test testSecurityGroupCacheInvalidated(org.jclo
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@andreaturli pushed 1 commit. 4e0b06b fix removal from security group cache -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1175/files/acc72a4d46f00b8cb0465d11677f32f9806a2f2f..4e0b06b2510001ccd9a83f3166989d6119255f46
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
nacx commented on this pull request.
Just one comment. Apart from that looks good.
Could you share the results of the live tests for the Nova and Neutron SG
extensions?
> + if (region == null) {
+ return ImmutableSet.of();
+ }
+ return
getSecurityGroupApi(region).listSecurityGroups().concat().transform(neutronSecurityGroupToSecurityGroup.create(location)).toSet();
+ }
+
+ @Override
+ public Set listSecurityGroupsForNode(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+ String instanceId = regionAndId.getId();
+
+ Optional serverApi =
api.getServerWithSecurityGroupsApi(region);
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+ ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
This is not used later. You're always returning all security groups.
--
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/1175#pullrequestreview-92514310
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
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/1175#issuecomment-359433463
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@nacx I think I've addressed most of your comments and now the duplication of `Find/Create` SecurityGroup is gone. I still have some expected tests to fix but please let me know if the code is more readable now. 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/1175#issuecomment-359024085
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@andreaturli pushed 1 commit. 0bdbdbf addressing Nacx comments -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1175/files/7846a2ddf32ed07e6fc394a43d954fc6fe039d28..0bdbdbf29214504f4e676e0afd6fb43f83275e4e
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
@nacx thanks for the review Main reason I have 2 couples of `Create/FindSecurityGroup` now (I want to remove them before merging this PR) is because of `NovaSecurityGroupExtension` and `NeutronSecurityGroupExtension` `NovaSecurityGroupExtension` needs to use `NovaApi` to create security groups, while `NeutronSecurityGroupExtension` uses `NeutronApi` Each couple of `Create/FindSecurityGroup` is a reasonably simple although ugly way to force each SecurityGroupExtension to use the appropriate API Maybe a different way to solve the problem is to decide whether the SecurityGroupExtension needs to re-use the caching mechanism, 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/1175#issuecomment-359014622
Re: [jclouds/jclouds] [JCLOUDS-1377] add support for injectable Neutron Context into Nova (#1175)
nacx requested changes on this pull request.
Thanks @andreaturli! Looks like a great starting point.
Apart from the comments I've made I found it difficult to understand the
duplicated "Create/FindSecurityGroup" classes. For a proper review, could you
please:
* Explain what is each new class, if it is a copy of an existing one and its
purpose and need?
* Explain the main design idea so we have a clear picture of how both
implementations Nova/Neutron will behave together.
* Explain a bit the different execution flows and where the decisions of using
Nova or Neutron are made, so we can properly understand the context around all
these classes and when they are going to take place.
> + bind(new TypeLiteral() {
+ }).toProvider(SecurityGroupExtensionProvider.class);
+
+ }
+
+ @Singleton
+ public static class SecurityGroupExtensionProvider implements
Provider {
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ protected Supplier neutronApiContextSupplier;
+
+ private final NeutronSecurityGroupExtension
neutronSecurityGroupExtension;
+ private final NovaSecurityGroupExtension novaSecurityGroupExtension;
+
+ @Inject
+ public SecurityGroupExtensionProvider(NeutronSecurityGroupExtension
neutronSecurityGroupExtension,
Remove the public modifier
> + */
+public class NeutronSecurityGroupExtension implements SecurityGroupExtension {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final NovaApi api;
+ private final Supplier> regionIds;
+ private final GroupNamingConvention.Factory namingConvention;
+ private final LoadingCache groupCreator;
+ private final Supplier> locationIndex;
+
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ Supplier neutronContextSupplier;
Make it `private`
> + @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final NovaApi api;
+ private final Supplier> regionIds;
+ private final GroupNamingConvention.Factory namingConvention;
+ private final LoadingCache groupCreator;
+ private final Supplier> locationIndex;
+
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ Supplier neutronContextSupplier;
+
+ @Inject
+ public NeutronSecurityGroupExtension(NovaApi api,
Remove the public modifier
> +
+ @Inject(optional = true)
+ @Named("openstack-neutron")
+ Supplier neutronContextSupplier;
+
+ @Inject
+ public NeutronSecurityGroupExtension(NovaApi api,
+@Region Supplier>
regionIds,
+GroupNamingConvention.Factory
namingConvention,
+LoadingCache groupCreator,
+Supplier>
locationIndex) {
+ this.api = api;
+ this.regionIds = checkNotNull(regionIds, "regionIds");
+ this.namingConvention = checkNotNull(namingConvention,
"namingConvention");
+ this.groupCreator = groupCreator;
+ this.locationIndex = checkNotNull(locationIndex, "locationIndex");
Remove the null checks. They're redundant.
> + Location location = locationIndex.get().get(region);
+ return ImmutableSet.copyOf(transform(filter(allGroups, notNull()),
toSecurityGroup(location)));
+ }
+
+ @Override
+ public SecurityGroup getSecurityGroupById(String id) {
+ RegionAndId regionAndId = RegionAndId.fromSlashEncoded(checkNotNull(id,
"id"));
+ String region = regionAndId.getRegion();
+ String groupId = regionAndId.getId();
+
+ SecurityGroupApi securityGroupApi = getSecurityGroupApi(region);
+
+// final
FluentIterable allGroups
= securityGroupApi.listSecurityGroups();
+// SecurityGroupInRegion rawGroup = new
SecurityGroupInRegion(securityGroupApi.get(groupId), region, allGroups);
+
+// return groupConverter.apply(rawGroup);
Remove the commented code.
> + @Override
+ public boolean supportsGroupIds() {
+ return true;
+ }
+
+ @Override
+ public boolean supportsPortRangesForGroups() {
+ return false;
+ }
+
+ @Override
+ public boolean supportsExclusionCidrBlocks() {
+ return false;
+ }
+
+ private Function toSecurityGroup(final Location location) {
Move this to its own type.
The private method is a bit awkward. Create a type for this function and use
google assisted inject and a factory interface to populate the Location field
where needed.
> import org.jclouds.logging.Logger;
import org.jclouds.openstack.nova.v2_0.NovaApi;
-import org.jclouds.openstack.nova.v2_0.domain.SecurityGroup;
+import
org.jclouds.openstack.nova.v2_0.compute.extensions.NeutronSecurityGroupExtension;
Unused?
>}
}
- private void authorizeGroupToItselfAndAllIPsToTCPPort(SecurityGroupApi
securityGroupApi,
- SecurityGroup
securityGroup, int port) {
+ private S
