CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. #1623
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ProjectMoon
changed the title
|
RPM packages built and available at: http://packages.shapeblue.com/cloudstack/custom/github-1623. |
|
|
||
| for (StaticNat snat : staticNats) { | ||
| userIps.add(_ipAddressDao.findById(snat.getSourceIpAddressId())); | ||
| } |
Contributor
There was a problem hiding this comment.
Executing queries in a for loop in unnecessarily expensive. Could you please refactor this for loop into a new method on IPAddressDao that retrieves a list of IP addresses for a list of IPs?
Author
There was a problem hiding this comment.
This makes sense, yes. Better to have one database query than many.
Contributor
|
@ProjectMoon have you run the Also, is there a JIRA ticket associated with this change? |
Author
|
JIRA ticket is CLOUDSTACK-9317. Have not run the Marvin tests, but will do so. |
Member
|
@blueorangutan kick |
|
A Trillian-Jenkins job has been kicked to build packages and start testing. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1623 |
Contributor
|
If possible, I would like to get this fix into 4.8.2.0/4.9.1.0, but I will be cutting the RC shortly. @ProjectMoon could you please amend your commit message to include the JIRA ticket ID and an explanation of the motivation for the change? @rhtyd @borisstoyanov can we push this PR through Trillian with smoke tests and the following test suites:
@murali-reddy Do you think we need to run any other test VR test cases for proper regression testing? |
|
Trillian test result (trillian-pr1623-38-vmware-55u3-cs48):
Trillian env - trillian-pr1623-38-vmware-55u3-cs48, Job ID 38 |
Author
|
It's been amended and rebased. I still haven't had time to add a new DAO method to the IP address DAO. I can work on that now though. When is the RC being cut? |
Contributor
|
@ProjectMoon I am trying to cut the 4.8.2.0 RC ASAP. This PR looks like it is not going to make it given the test failures. Do you mind re-pointing it to 4.9? We will make this PR a high priority for 4.9.2.0. |
Author
|
Yeah. Unfortunately I have not had any time to work on the pull requests lately. When I have time I will reopen it against 4.9. |
Contributor
|
@ProjectMoon I am extending the date for 4.8 to 25 Sept 2016. Therefore, if we can get the test failure fixed, we can get it into 4.8.2.0, 4.9.1.0, and 4.10.0.0. Also, could you please investigate the Travis and Jenkins build failures? |
Member
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-77 |
Author
|
This one hasn't been rebased in quite a while, so the packages produced will be rather old. Going to rebase this against latest 4.8. |
Member
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-207 |
The previous behavior would send a list of all IPs with a state of add/revoke. This causes concurrency issues when multiple IPs were being added or removed, and could leave networks with wrong IP addresses assigned to the devices. By assigning and removing only the IP being changed per-request, the concurrency issue is removed and no real performance loss occurs.
Author
|
Updated to latest 4.8. |
Member
|
@ProjectMoon thanks |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-231 |
Member
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-420)
|
Member
|
@abhinandanprateek @murali-reddy and others -- can we have review on this, thanks. |
Member
Author
|
Closed in favor of #1908 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New version of #1450, opened against the 4.8 branch.
Original Description
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.