You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by ProjectMoon <gi...@git.apache.org> on 2016/03/21 17:56:44 UTC
[GitHub] cloudstack pull request: Enable/disable static NAT associates only...
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/1450
Enable/disable static NAT associates only relevant IPs.
This pull request fixes a concurrency issue when disabling static NAT on a bunch of IPs simultaneously. Under the old behavior, executing multiple disable requests would result in invalid IP associations being sent to the virtual router. This commit changes the behavior to apply an IP association for only the IP being added/released, which means that it is impossible for the virtual router to receive invalid data.
This was tested against a virtual router running on KVM and VMware. It would be nice to have some input how this change could affect redundant routers and other static NAT providers.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/greenqloud/cloudstack pr-static-nat-concurrency-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1450.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 #1450
----
commit fb5c8951b8d76018eb3ae235c3dbb11f0acae579
Author: jeff <je...@greenqloud.com>
Date: 2016-03-21T15:20:27Z
Enable/disable static NAT associates only relevant IPs.
----
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57697669
--- Diff: server/src/com/cloud/network/router/CommandSetupHelper.java ---
@@ -778,10 +778,11 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
final boolean add = ipAddr.getState() == IpAddress.State.Releasing ? false : true;
boolean sourceNat = ipAddr.isSourceNat();
- /* enable sourceNAT for the first ip of the public interface */
- if (firstIP) {
- sourceNat = true;
+ /* enable sourceNAT for the first ip of the public interface as long as it's source nat. */
+ if (firstIP && !sourceNat) {
--- End diff --
How do I trigger this case using the CS API? Is it by adding multiple NICs to the virtual router?
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-218839655
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 0
Errors: 0
Duration: 8h 34m 12s
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_12_2016_05_59_28_PU8AAL:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/DeployDataCenter__May_12_2016_05_59_28_PU8AAL/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/DeployDataCenter__May_12_2016_05_59_28_PU8AAL/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/DeployDataCenter__May_12_2016_05_59_28_PU8AAL/runinfo.txt)
**`/tmp/MarvinLogs/test_network_N9BTSG:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/test_network_N9BTSG/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/test_network_N9BTSG/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/test_network_N9BTSG/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_5Y7V1C:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/test_vpc_routers_5Y7V1C/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/test_vpc_routers_5Y7V1C/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1450/tmp/MarvinLogs/test_vpc_routers_5Y7V1C/runinfo.txt)
Uploads will be available until `2016-07-12 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-221846391
Rebased against latest 4.7.
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-202152926
@ProjectMoon According to that comment on line 781 in `CommandSetupHelper` the conditional that follows is there to enable sourceNAT, yet you removed the command setting the sourceNat variable to true. Is that being done somewhere else? You might want to change that comment to clarify your change and avoid confusion in the future. :)
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57313299
--- Diff: server/test/com/cloud/network/IpAddressManagerTest.java ---
@@ -0,0 +1,71 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.network;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import com.cloud.network.dao.IPAddressDao;
+import com.cloud.network.dao.IPAddressVO;
+import com.cloud.network.rules.StaticNat;
+import com.cloud.network.rules.StaticNatImpl;
+import com.cloud.utils.net.Ip;
+
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.List;
+
+import static org.mockito.Mockito.anyLong;
+import static org.mockito.Mockito.mock;
+
+public class IpAddressManagerTest {
+
+ @Mock
+ IPAddressDao _ipAddrDao;
+
+ @InjectMocks
+ IpAddressManagerImpl _ipManager;
+
+ @Before
+ public void setup() {
+ MockitoAnnotations.initMocks(this);
+ }
+
+ @Test
+ public void testGetStaticNatSourceIps() {
+ String publicIpAddress = "192.168.1.3";
+ IPAddressVO vo = mock(IPAddressVO.class);
+ when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
+ when(vo.getId()).thenReturn(1l);
+
+ when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
+ StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
+
+ List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+ Assert.assertNotNull(ips);
+ Assert.assertEquals(1, ips.size());
+
--- End diff --
I would also add a check here to confirm that the ipAddrDao was called only once.
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57031859
--- Diff: server/src/com/cloud/network/IpAddressManagerImpl.java ---
@@ -1714,8 +1714,13 @@ public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean con
return true;
}
- // get the list of public ip's owned by the network
- List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null);
+ // get the list of public ip's that need to be applied for the static NAT. manipulating only these
+ // ips prevents concurrency issues when disabling static nat at the same time.
+ List<IPAddressVO> userIps = new ArrayList<>();
--- End diff --
Hi @ProjectMoon, what about creating a method that returns the userIps list. This new method would contain lines 1719-1722. Then, you would be able to write a unit test and add lines 1717-1718 as its java documentation.
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-218839846
Can I get some code review on this one? Thx...
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-218848727
tag:needsreview
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on the issue:
https://github.com/apache/cloudstack/pull/1450
@ProjectMoon
I am working on it. I will be updating the fix.
---
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] cloudstack pull request #1450: CLOUDSTACK-9317: Enable/disable static NAT as...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/1450
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the issue:
https://github.com/apache/cloudstack/pull/1450
@jayapalu Can you post this in the new version of the fix? (And possibly re-test, if you were using an older version)
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-219037058
It would be useful for someone knowledgeable in the network internals to elaborate on @jayapalu's comment. We currently do not use the feature he's talking about, so it would be helpful if someone could direct me where to find setup instructions for that case.
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57422486
--- Diff: server/src/com/cloud/network/router/CommandSetupHelper.java ---
@@ -778,10 +778,11 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
final boolean add = ipAddr.getState() == IpAddress.State.Releasing ? false : true;
boolean sourceNat = ipAddr.isSourceNat();
- /* enable sourceNAT for the first ip of the public interface */
- if (firstIP) {
- sourceNat = true;
+ /* enable sourceNAT for the first ip of the public interface as long as it's source nat. */
+ if (firstIP && !sourceNat) {
--- End diff --
For additional public subnet case, sourceNat should be set to 'true' to add a source nat rule on VR for the first ip in that subnet. This changes will break that.
If there is no source nat rule for the additional public subnet the traffic to this subnet from he VMs always go through the default source nat interface.
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-200825346
It is always great to see parts of a bigger method being extracted to smaller ones, and then test cases and java docs being used.
I believe the tests like you did are ok, I would only add another assert there as I pointed out at line 67 of the test class.
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on the issue:
https://github.com/apache/cloudstack/pull/1450
@ProjectMoon
Added the new PR https://github.com/apache/cloudstack/pull/1908 for addressing missing issues in this PR
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57313534
--- Diff: server/src/com/cloud/network/IpAddressManagerImpl.java ---
@@ -1698,6 +1698,22 @@ public String acquireGuestIpAddress(Network network, String requestedIp) {
Random _rand = new Random(System.currentTimeMillis());
+ /**
+ * Get the list of public IPs that need to be applied for a static NAT enable/disable operation.
+ * Manipulating only these ips prevents concurrency issues when disabling static nat at the same time.
+ * @param staticNats
+ * @return The list of IPs that need to be applied for the static NAT to work.
+ */
+ public List<IPAddressVO> getStaticNatSourceIps(List<? extends StaticNat> staticNats) {
+ List<IPAddressVO> userIps = new ArrayList<>();
+
+ for (StaticNat snat : staticNats) {
+ userIps.add(_ipAddressDao.findById(snat.getSourceIpAddressId()));
+ }
+
+ return userIps;
+ }
+
@Override
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException {
if (staticNats == null || staticNats.size() == 0) {
--- End diff --
I know this is not your code; but, what about changing "staticNats == null || staticNats.size() == 0" to "CollectionUtils.isEmpty(staticNats)"
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57319355
--- Diff: server/test/com/cloud/network/IpAddressManagerTest.java ---
@@ -0,0 +1,71 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.network;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import com.cloud.network.dao.IPAddressDao;
+import com.cloud.network.dao.IPAddressVO;
+import com.cloud.network.rules.StaticNat;
+import com.cloud.network.rules.StaticNatImpl;
+import com.cloud.utils.net.Ip;
+
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.List;
+
+import static org.mockito.Mockito.anyLong;
+import static org.mockito.Mockito.mock;
+
+public class IpAddressManagerTest {
+
+ @Mock
+ IPAddressDao _ipAddrDao;
+
+ @InjectMocks
+ IpAddressManagerImpl _ipManager;
+
+ @Before
+ public void setup() {
+ MockitoAnnotations.initMocks(this);
+ }
+
+ @Test
+ public void testGetStaticNatSourceIps() {
+ String publicIpAddress = "192.168.1.3";
+ IPAddressVO vo = mock(IPAddressVO.class);
+ when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
+ when(vo.getId()).thenReturn(1l);
+
+ when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
+ StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
+
+ List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+ Assert.assertNotNull(ips);
+ Assert.assertEquals(1, ips.size());
+
--- End diff --
You do not use JUnit for that.
You use your mock API such as easy mock or mockito that we use here.
you can do something like this:
Mockito.verify(mock, Mockito.times(1)).method(parameter);
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1450#discussion_r57317877
--- Diff: server/test/com/cloud/network/IpAddressManagerTest.java ---
@@ -0,0 +1,71 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.network;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import com.cloud.network.dao.IPAddressDao;
+import com.cloud.network.dao.IPAddressVO;
+import com.cloud.network.rules.StaticNat;
+import com.cloud.network.rules.StaticNatImpl;
+import com.cloud.utils.net.Ip;
+
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.List;
+
+import static org.mockito.Mockito.anyLong;
+import static org.mockito.Mockito.mock;
+
+public class IpAddressManagerTest {
+
+ @Mock
+ IPAddressDao _ipAddrDao;
+
+ @InjectMocks
+ IpAddressManagerImpl _ipManager;
+
+ @Before
+ public void setup() {
+ MockitoAnnotations.initMocks(this);
+ }
+
+ @Test
+ public void testGetStaticNatSourceIps() {
+ String publicIpAddress = "192.168.1.3";
+ IPAddressVO vo = mock(IPAddressVO.class);
+ when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
+ when(vo.getId()).thenReturn(1l);
+
+ when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
+ StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
+
+ List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+ Assert.assertNotNull(ips);
+ Assert.assertEquals(1, ips.size());
+
--- End diff --
How is it possible to do that with JUnit?
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-221762730
CI is clean and everything is green. I need some code review on this one. Thanks...
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by jayapalu <gi...@git.apache.org>.
Github user jayapalu commented on the issue:
https://github.com/apache/cloudstack/pull/1450
I tested this patch with below steps. It is not removing the ip addresses on the VR interface. Some times observed that even there is one ip with static nat but the interface got removed.
Steps to test this:
1. Configure additional public subnet and acquire 4 ip addresses
2. Enabled static nat on the 4 public ip addresses.
3. Go to VR console and see the new public interface created and ips are configured on the interface.
4. Disable static on all 4 public addresses (disable from UI one by one quickly)
expected: inteface should get removed from the VR. But the interface is there in VR
In another case in step4 disable static nat on only 3 public ip addresses.
expected: interface with one ip supposed to be there. But the interface is deleted
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the issue:
https://github.com/apache/cloudstack/pull/1450
### ACS CI BVT Run
**Sumarry:**
Build Number 154
Hypervisor xenserver
NetworkType Advanced
Passed=73
Failed=0
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-202811882
@cristofolini The idea was that the source NAT IP is always the first IP. Not sure if the logic is correct. But apparently I need to correct the logic for the case @jayapalu mentioned.
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the issue:
https://github.com/apache/cloudstack/pull/1450
Updated to latest 4.7.
---
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] cloudstack pull request: CLOUDSTACK-9317: Enable/disable static NA...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1450#issuecomment-200345827
I added a basic test now and moved the method out @rafaelweingartner. Maybe there are more scenarios that can be tested?
---
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] cloudstack issue #1450: CLOUDSTACK-9317: Enable/disable static NAT associate...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the issue:
https://github.com/apache/cloudstack/pull/1450
Closing to reopen against 4.8.
---
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.
---