cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "daan Hoogland" <daan.hoogl...@gmail.com>
Subject Re: Review Request 16758: vlan="" should end up as untagged vlan URI
Date Fri, 10 Jan 2014 10:30:57 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16758/#review31514
-----------------------------------------------------------


I am fine with this with the exception of one worry: 'vlan://untagged' may be compared to
'untagged' at some points in the system. I would like to include a change in NetUtils.java
for this:
        if (VLAN_UNTAGGED.equalsIgnoreCase(one) && VLAN_UNTAGGED.equalsIgnoreCase(other))
becomes
        if (one.contains(VLAN_UNTAGGED) && other.contains(VLAN_UNTAGGED))
in isSameIsolationId(String,String)

- daan Hoogland


On Jan. 9, 2014, 6:52 p.m., Marcus Sorensen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16758/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2014, 6:52 p.m.)
> 
> 
> Review request for cloudstack and daan Hoogland.
> 
> 
> Bugs: CLOUDSTACK-5502
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5502
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> See conversation on mailing list and CLOUDSTACK-5502:
> 
> To recap, in 4.2
> the UI would interpret no vlan entered as 'untagged' and pass that as
> a parameter. Undocumented or not, it was being used by the API's
> biggest customer.
> 
> with 4.2, passing 'untagged' resulted in:
> 
> Saving vlan range
> Vlan[untagged|12.12.12.1|255.255.255.0|null|null|12.12.12.10-12.12.12.20||null1004]
> 
> with 4.3 AND 4.2, passing '' fails with 'Vlan id required',
> because untagged was changed to null, while an empty string fell
> through the null check and resulted in
> Saving vlan range
> Vlan[|12.12.12.1|255.255.255.0|null|null|12.12.12.10-12.12.12.20||null1004]
> which breaks later: Networks.java line 323, "Unable to convert to
> isolation URI:"
> 
> with 4.3, minus patch aaf3979cf92518d3dc5587ea0192f4b3ce1e7866,
> passing vlan=untagged results in:
> Saving vlan range
> Vlan[vlan://untagged|12.12.12.1|255.255.255.0|null|null|12.12.12.10-12.12.12.20||null1004]
> 
> Which is exactly the behavior we want to see for both "" and
> "untagged", if we want to maintain compatibility with previous public
> network deployments.
> 
> 
> Diffs
> -----
> 
>   api/src/org/apache/cloudstack/api/command/admin/vlan/CreateVlanIpRangeCmd.java 541da1e

>   server/src/com/cloud/configuration/ConfigurationManagerImpl.java f9d282f 
> 
> Diff: https://reviews.apache.org/r/16758/diff/
> 
> 
> Testing
> -------
> 
> Tested advanced zone deployments, passing both empty string for 'vlan' and 'untagged'
> 
> 
> Thanks,
> 
> Marcus Sorensen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message