CLOUDSTACK-9317: Fixed disable static nat on leaving ips on interface #1908
Conversation
|
|
||
| boolean remove = false; | ||
| // if there is only one static nat then it will be checked for remove at the resource | ||
| if (ipsWithrules == 0 && (ipsStaticNat == 0 || ipsStaticNat == 1)) { |
There was a problem hiding this comment.
Can you combine these two if blocks and eliminate remove variable if this is not getting used at a later place. Even if remove is getting used later, two if blocks can be combined
| public static final String VPC_PRIVATE_GATEWAY = "vpc.gateway.private"; | ||
| public static final String FIREWALL_EGRESS_DEFAULT = "firewall.egress.default"; | ||
| public static final String ROUTER_MONITORING_ENABLE = "router.monitor.enable"; | ||
| public static final String NETWORK_PUB_LAST_IP = "newtork.public.last.ip"; |
setup/db/db/schema-4920to41000.sql
Outdated
| INSERT INTO `cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`, `sort_order`) values (UUID(), 3, 'createSnapshotFromVMSnapshot', 'ALLOW', 302) ON DUPLICATE KEY UPDATE rule=rule; | ||
| INSERT INTO `cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`, `sort_order`) values (UUID(), 4, 'createSnapshotFromVMSnapshot', 'ALLOW', 260) ON DUPLICATE KEY UPDATE rule=rule; | ||
|
|
||
| ALTER TABLE `user_ip_address` ADD COLUMN `staticnat_state` VARCHAR(32) COMMENT 'static rule state while removing' |
There was a problem hiding this comment.
This column name does not match the name of the column in the VO (rule_state).
| @Inject | ||
| private IPAddressDao _ipAddressDao; | ||
| @Inject | ||
| private FirewallRulesDao _firewallsDao; |
There was a problem hiding this comment.
There is already a FirewallRulesDao in the file. No need to inject this one.
|
Question: would #1907 be relevant to this? That change relied on sending all the IPs to the router, but under this change, we will be sending one at a time. |
|
I'm looking at the I think this is a scenario that should definitely be tested as it has the potential to break this whole PR. |
|
We have been testing this PR internally for some time now, and we have seen some strange Specifically, we have seen that IPs left over in the Thus, the code that was added in PR #1907 concerns me: The I will do a bit of manual testing combining both #1908 and #1907 to see what happens. It may just be that after restarting the network (and thus cleaning the router), these issues will vanish. |
|
@ProjectMoon I think the ips.json problem is independent of this PR that need to be fixed. |
|
@jayapalu Quite possible, yes. It could be fixed by adding another condition to the check that was added with #1907. It could also be "fixed" by checkiing if |
|
@ProjectMoon The ip issue can be worked as separate ticket/PR. It is an isolated issue, it is not having any dependency with this PR. So we will get this PR in and create a separate ticket for the ip issue. |
|
@ProjectMoon Here is the output: 4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000 |
|
Hi, that's great news. |
|
@ProjectMoon Can you please review it. |
|
@jayapalu can you change the PR base branch to 4.9? |
| // remove the nic | ||
| if (ipsCount == 1 && !ip.isAdd()) { | ||
| removeVif = true; | ||
| if (lastIp != null && !ip.isAdd()) { |
There was a problem hiding this comment.
Maybe it would make sense to check the value of lastIp being something instead of just checking if it's not null? It works the way it is, but perhaps checking specifically for a value like "true" would make sense. Would prevent someone from updating code later to send "false" for whatever reason and then causing something unexpected. Relying on not null as a truth value is a bit obtuse in my opinion.
ProjectMoon
left a comment
ProjectMoon
left a comment
There was a problem hiding this comment.
Left more comments. Also, it would probably be good to add some tests for the CommandSetupHelper to test generation of the command for adding/removing IPs. Testing the network.public.last.ip detail would be very good.
| // remove the nic | ||
| if (ipsCount == 1 && !ip.isAdd()) { | ||
| removeVif = true; | ||
| if (lastIp != null && lastIp.equalsIgnoreCase("true") && !ip.isAdd()) { |
There was a problem hiding this comment.
StringUtils.equalsIgnoreCase can replace checking both null and the value: https://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/StringUtils.html#equalsIgnoreCase(java.lang.String, java.lang.String)
There was a problem hiding this comment.
In CitrixresourceBase StringUtils is used from the com.cloud.utils.StringUtils. So using StringUtils from java.lang will be ambiguous.
There was a problem hiding this comment.
True, but a lot of the other code is already using StringUtils or is moving to it. Reviewers in other PRs tend to recommend it.
There was a problem hiding this comment.
@ProjectMoon
Updated to use org.apache.commons.lang.StringUtils
Can you please review it.
| if (numOfIps == 1 && !ip.isAdd()) { | ||
| vifHotUnPlug(conn, routerName, ip.getVifMacAddress()); | ||
| networkUsage(routerIp, "deleteVif", "eth" + nicNum); | ||
| if (lastIp != null && lastIp.equalsIgnoreCase("true") && !ip.isAdd()) { |
There was a problem hiding this comment.
StringUtils.equalsIgnoreCase can check both null and the value.
There was a problem hiding this comment.
In CitrixresourceBase StringUtils is used from the com.cloud.utils.StringUtils. So using StringUtils from java.lang will be ambiguous.
| cmd.setAccessDetail(NetworkElementCommand.ZONE_NETWORK_TYPE, dcVo.getNetworkType().toString()); | ||
|
|
||
| // if there 1 static nat then it will be checked for remove at the resource | ||
| if (ipsWithrules == 0 && ipsStaticNat == 0 ) { |
There was a problem hiding this comment.
There's an extraneous space between the 0 and the ) here. Also, this is probably just me misunderstanding the code, but wouldn't you want to check if ipsStaticNat equals 1 in this case? Or maybe you're checking for both to be 0 because if both are 0 we're generating a removal command?
There was a problem hiding this comment.
If we check the ipsStaticNat=1 then we will miss the case of disabling static nat on network with 1 with static nat.
| } | ||
|
|
||
| // check onetoonenat and also check if the ip "add":false. If there are 2 PF remove 1 static nat add | ||
| if (ip.isOneToOneNat() && ip.getRuleState() == null) { |
There was a problem hiding this comment.
Similar to the comments about checking if lastIp isn't null, I find that checking rule state to be null is a bit ... obscure. What is null supposed to mean in this context? It seems the rule_state column is only changed to Releasing when releasing an IP and nothing else. I guess that's intentional?
There was a problem hiding this comment.
In this case rule state is not set. It is null. I will improve the comment
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests:
Skipped tests: Passed test suits: |
ProjectMoon
left a comment
ProjectMoon
left a comment
There was a problem hiding this comment.
Just a few minor things left, with the biggest being the conflict lines that sneaked into the SQL file.
setup/db/db/schema-4920to41000.sql
Outdated
| WHERE (o.cpu is null AND o.speed IS NULL AND o.ram_size IS NULL) AND | ||
| (d.name = 'cpuNumber' OR d.name = 'cpuSpeed' OR d.name = 'memory'); | ||
|
|
||
| <<<<<<< 1c48deefe9b534198cad19b5528ce0dcfa8d04a5 |
There was a problem hiding this comment.
It seems you got a conflict stuck in the file.
There was a problem hiding this comment.
My bad, on multiple times rebase some how I missed it. Corrected now.
setup/db/db/schema-4920to41000.sql
Outdated
| DROP VIEW IF EXISTS `cloud`.`storage_tag_view`; | ||
|
|
||
| ALTER TABLE `user_ip_address` ADD COLUMN `rule_state` VARCHAR(32) COMMENT 'static rule state while removing'; | ||
| ======= |
| int ipsWithrules = 0; | ||
| int ipsStaticNat = 0; | ||
| for (IPAddressVO ip : userIps) { | ||
| if ( _rulesDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active) > 0 ) { |
There was a problem hiding this comment.
Inconsistent formatting of the parentheses in the if. A bit nitpicky, but the code has to adhere to format.
|
Looks good now. Not sure what's up with Jenkins though. |
|
Jenkins got timed out. I am force pushing again to trigger jenkins. |
|
@ProjectMoon Jenkins and CI is passed, Can you please give LGTM |
|
Looks fine to me. |
|
@borisstoyanov Can you please trigger the travis CI on this PR please |
|
@jayapalu Travis is already green, assuming you mean Trillian I'll kick some jobs |
|
@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-655 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1013)
|
|
Two LGTMs and Trillian tests are passed. Marking tag:mergeready |
|
Disable static NAT working fine, IP's are leaving interface as soon as they are disabled. Test results are as expected. LGTM |
borisstoyanov
left a comment
borisstoyanov
left a comment
There was a problem hiding this comment.
LGTM based on test results
|
@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-721 |
|
merge conflicts |
There is 1 static nat rule and 2 PF rule. Removing 2 PF rules was deleting static nat rule.
Fixed this issue in this commit.
|
Merge conflicts addressed. |
|
@jayapalu thanks, can you also squash your changes? |
12 participants
FIxed issue in disabling multiple static nat simultaneously.
This patch has taken changes from the PR 1450 and added the missing parts
#1450