CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. #1450

ProjectMoon · 2016-03-21T16:56:42Z

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.

rafaelweingartner · 2016-03-22T17:34:00Z

server/src/com/cloud/network/IpAddressManagerImpl.java

-        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<>();


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.

ProjectMoon · 2016-03-23T13:35:54Z

I added a basic test now and moved the method out @rafaelweingartner. Maybe there are more scenarios that can be tested?

rafaelweingartner · 2016-03-24T13:08:50Z

server/test/com/cloud/network/IpAddressManagerTest.java

+        List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+        Assert.assertNotNull(ips);
+        Assert.assertEquals(1, ips.size());
+


I would also add a check here to confirm that the ipAddrDao.findById was called only once.

How is it possible to do that with JUnit?

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);

rafaelweingartner · 2016-03-24T13:13:22Z

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.

jayapalu · 2016-03-25T06:18:54Z

server/src/com/cloud/network/router/CommandSetupHelper.java

-                if (firstIP) {
-                    sourceNat = true;
+                /* enable sourceNAT for the first ip of the public interface as long as it's source nat. */
+                if (firstIP && !sourceNat) {


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.

How do I trigger this case using the CS API? Is it by adding multiple NICs to the virtual router?

cristofolini · 2016-03-27T21:52:59Z

@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. :)

ProjectMoon · 2016-03-29T09:54:36Z

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

swill · 2016-05-12T18:10:47Z

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:

/tmp/MarvinLogs/test_network_N9BTSG:

/tmp/MarvinLogs/test_vpc_routers_5Y7V1C:

Uploads will be available until 2016-07-12 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-12T18:11:21Z

Can I get some code review on this one? Thx...

DaanHoogland · 2016-05-12T18:42:56Z

tag:needsreview

ProjectMoon · 2016-05-13T13:05:41Z

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.

swill · 2016-05-26T02:36:58Z

CI is clean and everything is green. I need some code review on this one. Thanks...

ProjectMoon · 2016-05-26T11:30:16Z

Rebased against latest 4.7.

bvbharatk · 2016-06-07T17:53:22Z

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

ProjectMoon · 2016-08-02T10:29:15Z

Updated to latest 4.7.

ProjectMoon · 2016-08-02T14:29:33Z

Closing to reopen against 4.8.

jayapalu · 2017-01-12T10:18:41Z

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:

Configure additional public subnet and acquire 4 ip addresses
Enabled static nat on the 4 public ip addresses.
Go to VR console and see the new public interface created and ips are configured on the interface.
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

ProjectMoon · 2017-01-12T10:34:24Z

@jayapalu Can you post this in the new version of the fix? (And possibly re-test, if you were using an older version)

jayapalu · 2017-01-13T10:55:03Z

@ProjectMoon
I am working on it. I am working fix, will be updating the fix soon.

jayapalu · 2017-01-17T09:59:26Z

@ProjectMoon
Added the new PR #1908 for addressing missing issues in this PR

ProjectMoon changed the title ~~Enable/disable static NAT associates only relevant IPs.~~ CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. Mar 22, 2016

rafaelweingartner reviewed Mar 22, 2016
View reviewed changes

rafaelweingartner reviewed Mar 24, 2016
View reviewed changes

jayapalu reviewed Mar 25, 2016
View reviewed changes

Enable/disable static NAT associates only relevant IPs.

477adfc

ProjectMoon closed this Aug 2, 2016

ProjectMoon deleted the pr-static-nat-concurrency-fix branch August 2, 2016 14:29

ProjectMoon mentioned this pull request Aug 2, 2016

CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. #1623

Closed

jayapalu mentioned this pull request Jan 17, 2017

CLOUDSTACK-9317: Fixed disable static nat on leaving ips on interface #1908

Merged

CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. #1450

CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. #1450

Uh oh!

Conversation

ProjectMoon commented Mar 21, 2016

Uh oh!

rafaelweingartner Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

ProjectMoon commented Mar 23, 2016

Uh oh!

rafaelweingartner Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

ProjectMoon Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Mar 24, 2016

Uh oh!

jayapalu Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

ProjectMoon Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

cristofolini commented Mar 27, 2016

Uh oh!

ProjectMoon commented Mar 29, 2016

Uh oh!

swill commented May 12, 2016

CI RESULTS

Uh oh!

swill commented May 12, 2016

Uh oh!

DaanHoogland commented May 12, 2016

Uh oh!

ProjectMoon commented May 13, 2016

Uh oh!

swill commented May 26, 2016

Uh oh!

ProjectMoon commented May 26, 2016

Uh oh!

bvbharatk commented Jun 7, 2016

ACS CI BVT Run

Uh oh!

ProjectMoon commented Aug 2, 2016

Uh oh!

ProjectMoon commented Aug 2, 2016

Uh oh!

jayapalu commented Jan 12, 2017

Uh oh!

ProjectMoon commented Jan 12, 2017

Uh oh!

jayapalu commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayapalu commented Jan 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jayapalu commented Jan 13, 2017 •

edited

Loading