CLOUDSTACK-10047: DVSwitch fixes and improvements #2293
Conversation
e9826a6 to
825b817
Compare
|
Notes:
|
d1491b1 to
84ef76d
Compare
|
Pinging for review @PaulAngus @nvazquez @DaanHoogland @borisstoyanov @dagsonstebo @resmo @rafaelweingartner @sureshanaparti and others |
|
@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-1150 |
ui/l10n/en.js
Outdated
| @@ -758,6 +758,7 @@ var dictionary = {"ICMP.code":"ICMP Code", | |||
| "label.firewall":"Firewall", | |||
| "label.first.name":"First Name", | |||
| "label.firstname.lower":"firstname", | |||
| "label.forged.trasmits":"Forged Transmits", | |||
ui/scripts/configuration.js
Outdated
| }, | ||
|
|
||
| forgedTransmits: { | ||
| label: 'label.forged.trasmits', |
|
Thanks @resmo fixed :) |
84ef76d to
a490249
Compare
|
@blueorangutan test centos7 vmware-55u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests |
There was a problem hiding this comment.
Hi @rhtyd I have seen some points that might benefit from improvements.
| final NetworkVO network = _networkDao.findByUuid(nicTo.getNetworkUuid()); | ||
| if (network != null) { | ||
| final Map<NetworkOffering.Detail, String> details = networkOfferingDetailsDao.getNtwkOffDetails(network.getNetworkOfferingId()); | ||
| if (details != null) { |
There was a problem hiding this comment.
What about reducing the cyclomatic complexicity here?
it is a matter of inverting the conditional. Instead of if (true){doSomething}, we can do if(!false){continue} doSomething
This would enable to remove one if inception
There was a problem hiding this comment.
We're protecting against NPEs and setting details to a NicTO object when those details are available. We cannot simply continue as the value is being set at lines 161, 163.
There was a problem hiding this comment.
You are right about the details, but I think I clicked on the wrong line (sorry for the mistake). I was talking about the if (network != null) check. if if (network == null) you do not do anything, then it is the same as using a continue. I mean, you do a nics[i++] = nicTo;, but that can go inside the condition as well. Duplicated lines are not that great, so let's see what else could be done...
We can do something else, we could extract lines 149-160 to a method; this would improve the readability of the code and enable unit tests and Java docs.
There was a problem hiding this comment.
The block converts NicProfile to NicTO, the idea of the changes is to override certain security settings on dvswitch's portgroups if they already don't have the settings, using global settings. I'll explore further refactorings if I get time.
There was a problem hiding this comment.
Thanks for both the explanations and for the effort!
BTW: your explanation is great to enrich our code base. This explanation would be awesome in a Javadoc, so people in the future can understand why we are doing this without needing to deeply inspect the code.
| */ | ||
| public static List<Integer> expandVlanUri(final String vlanAuthority) { | ||
| final List<Integer> expandedVlans = new ArrayList<>(); | ||
| if (vlanAuthority == null || vlanAuthority.isEmpty()) { |
There was a problem hiding this comment.
what about StringUtils.isBlank here?
There was a problem hiding this comment.
Fixed with Strings.isNullOrEmpty, thanks
| return expandedVlans; | ||
| } | ||
| for (final String vlanPart: vlanAuthority.split(",")) { | ||
| if (vlanPart == null || vlanPart.isEmpty()) { |
There was a problem hiding this comment.
what about StringUtils.isBlank here?
There was a problem hiding this comment.
Fixed with Strings.isNullOrEmpty, thanks
| } | ||
| } else { | ||
| final Integer value = NumbersUtil.parseInt(range[0], -1); | ||
| if (value > -1) { |
There was a problem hiding this comment.
what about a debug message here to say why we are rejecting this value, and displaying the value of range[0], in case range[0] is not a number and the method returns -1
There was a problem hiding this comment.
This is used for vlan range checks etc, vlans are always > -1.
| public static boolean checkVlanUriOverlap(final String vlanRange1, final String vlanRange2) { | ||
| final List<Integer> vlans1 = expandVlanUri(vlanRange1); | ||
| final List<Integer> vlans2 = expandVlanUri(vlanRange2); | ||
| if (vlans1 == null || vlans2 == null) { |
There was a problem hiding this comment.
if one of them is null, the result is true? This means that they overlap?
There was a problem hiding this comment.
We're checking if two vlan ranges (comma separated ranges or values, such as 100-200,300 or 20-30 etc) overlap, if any of them when expanded (i.e. 1-3 expands to 1,2,3) is null, i.e. there is no overlap.
There was a problem hiding this comment.
I did not understand the explanations :(
Anyways, looking at the code of expandVlanUri, I did not see a way for it to return null. Worst case scenario it returns an empty list. Do we need this check?
There was a problem hiding this comment.
Yes, we need this check.
There was a problem hiding this comment.
Do you mind explaining why if the method expandVlanUri returns an empty list in the worst case?
There was a problem hiding this comment.
I understand that, but I think we should not over code or engineer. If we worry about the method expandVlanUri returning null, we can do something else to catch this.
What about a test case for expandVlanUri that fails if it returns null? Then, we can remove this check. On thing is to be defensive when a null case can happen, the other is to code expecting someone to make a mistake in the future (for that it is better to write unit test cases).
There was a problem hiding this comment.
I think we're over-discussing, not over-engineering :) I'll ping you on respective unit tests.
There was a problem hiding this comment.
Ah that as well ;)
I am sorry to bother, but if I do not understand something I keep asking until I can move along.
There was a problem hiding this comment.
You could also use !Collections.disjoint(vlans1, vlans2)
There was a problem hiding this comment.
@fmaximus fixed.
| private List<DataCenterVnetVO> findOverlappingVnets(final long dcId, final Long physicalNetworkId, final String vnet) { | ||
| final List<Integer> searchVnets = UriUtils.expandVlanUri(vnet); | ||
| final List<DataCenterVnetVO> overlappingVnets = new ArrayList<>(); | ||
| if (searchVnets != null && searchVnets.size() > 0) { |
There was a problem hiding this comment.
what about inverting this conditional?
if( searchVnets == null || searchVnets.size() == 0){return overlappingVnets;}
This helps to reduce the number of IFs inside of IFs
|
Trillian test result (tid-1579)
|
a490249 to
cb09283
Compare
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1152 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
| Assert.assertFalse(UriUtils.checkVlanUriOverlap("10-30,45,50,12,31", "")); | ||
| Assert.assertFalse(UriUtils.checkVlanUriOverlap("10-30,45,50,12,31", "32")); | ||
| Assert.assertFalse(UriUtils.checkVlanUriOverlap("10,22,111", "12")); | ||
| Assert.assertFalse(UriUtils.checkVlanUriOverlap("100-200", "30-40,50,201-250")); |
There was a problem hiding this comment.
@rafaelweingartner unit tests for respective methods in questions are in this file, please see all the above lines.
There was a problem hiding this comment.
I had already seen these tests, I did not say anything because in my option they are properly written ;)
So, about that null case, in my option it is already covered, if for some reason someone alters the method to return null, your test cases will catch it (this is great!). That is why I was saying you do not need those null checks.
There was a problem hiding this comment.
Sure, sometimes we do want to over-engineer™ :)
There was a problem hiding this comment.
Ahaha, sometimes we do, but we should not.
I totally understand when we do, I also exaggerate sometimes, that is why I find it is great a review process to get another set of eyes to look at the problem and the code.
|
Trillian test result (tid-1582)
|
| if (network != null) { | ||
| final Map<NetworkOffering.Detail, String> details = networkOfferingDetailsDao.getNtwkOffDetails(network.getNetworkOfferingId()); | ||
| if (details != null) { | ||
| if (!details.containsKey(NetworkOffering.Detail.PromiscuousMode)) { |
There was a problem hiding this comment.
You might also use putIfAbsent here instead, which was added in Java 8
There was a problem hiding this comment.
Fixed. Thanks, good tip.
| public static boolean checkVlanUriOverlap(final String vlanRange1, final String vlanRange2) { | ||
| final List<Integer> vlans1 = expandVlanUri(vlanRange1); | ||
| final List<Integer> vlans2 = expandVlanUri(vlanRange2); | ||
| if (vlans1 == null || vlans2 == null) { |
There was a problem hiding this comment.
You could also use !Collections.disjoint(vlans1, vlans2)
| @@ -48,6 +51,13 @@ public String getVlan() { | |||
| return vlan; | |||
| } | |||
|
|
|||
| public Boolean getBypassVlanOverlapCheck() { | |||
There was a problem hiding this comment.
Is there a reason why we do return a Boolean instead of boolean? (this function can never return null)
There was a problem hiding this comment.
Yes, because cmd classes set API args to fields using reflections etc. It's easier to not use native type boolean. When arg is not sent part of the API request, it would be set to null; setting null to boolean will throw an exception.
| private List<DataCenterVnetVO> findOverlappingVnets(final long dcId, final Long physicalNetworkId, final String vnet) { | ||
| final List<Integer> searchVnets = UriUtils.expandVlanUri(vnet); | ||
| final List<DataCenterVnetVO> overlappingVnets = new ArrayList<>(); | ||
| if (searchVnets == null || searchVnets.size() == 0) { |
There was a problem hiding this comment.
You could use searchVnets.isEmpty() instead of size()==0?
| networkName = composeCloudNetworkName(namePrefix, vlanId, secondaryvlanId, networkRateMbps, physicalNetwork); | ||
|
|
||
| if (vlanId != null && !UNTAGGED_VLAN_NAME.equalsIgnoreCase(vlanId)) { | ||
| if (vlanId != null && !UNTAGGED_VLAN_NAME.equalsIgnoreCase(vlanId) && !vlanId.contains(",") && !vlanId.contains("-")) { |
There was a problem hiding this comment.
&& !vlanId.contains(",") && !vlanId.contains("-") could be replaced by !StringUtils.containsAny(vlanId, ",-")?
| createGCTag = true; | ||
| vid = Integer.parseInt(vlanId); | ||
| } | ||
| if (vlanId != null && (vlanId.contains(",") || vlanId.contains("-"))) { |
There was a problem hiding this comment.
could be replaced by StringUtils.containsAny(vlanId, ",-")? Maybe also extract it to a separate boolean because it's checked multiple times?
- Accepts security policies while creating network offering - Deployed network will have security policies from the network offering applied on the port group (in vmware environment) - Global settings as fallback when security policies are not defined for a network offering - Default promiscuous mode security policy set to REJECT as it's the default for standard/default vswitch Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
This allows admins to define a network with comma separated vlan id and vlan range such as vlan://200-400,21,30-50 and use the provided vlan range to configure vlan-trunking for a portgroup in dvswitch based environment. VLAN overlap checks are performed for: - isolated network against existing shared and isolated networks - dedicated vlan ranges for the physical/public network for the zone - shared network against existing isolated network Allow shared networks to bypass vlan overlap checks: This allows admins to create shared networks with a `bypassvlanoverlapcheck` API flag which when set to 'true' will create a shared network without performing vlan overlap checks against isolated network and against the vlans allocated to the datacenter's physical network (vlan ranges). Notes: - No vlan-range overlap checks are performed when creating shared networks - Multiple vlan id/ranges should include the vlan:// scheme prefix Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
cb09283 to
f24ced9
Compare
|
I've incorporated feedback from code review, given this has enough LGTMs and test results, I'll merge this as soon as Travis goes green. Thanks everyone for your feedback, review and testings. |
This adds a minor feature to accepts security policies while creating network offering. Changes:
applied on the port group (in vmware environment)
offering
for standard/default vswitch
This also allows admins to define a network with vlan range such as vlan://200-400
and use the range to configure vlan-trunking with the range for a portgroup
in dvswitch.
VLAN overlap checks are performed for:
Notes: