Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Content-Type: multipart/alternative;
 boundary="===============1034449608474826383=="
MIME-Version: 1.0
Subject: Re: Review Request 16758: vlan="" should end up as untagged vlan URI
From: "daan Hoogland" <daan.hoogland@gmail.com>
To: "daan Hoogland" <daan.hoogland@gmail.com>
Cc: "Marcus Sorensen" <shadowsor@gmail.com>,
 "cloudstack" <dev@cloudstack.apache.org>
Date: Fri, 10 Jan 2014 10:30:57 -0000
Message-ID: <20140110103057.1924.52279@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "daan Hoogland" <noreply@reviews.apache.org>
References: <20140109185233.1924.56400@reviews.apache.org>
In-Reply-To: <20140109185233.1924.56400@reviews.apache.org>
Reply-To: "daan Hoogland" <daan.hoogland@gmail.com>

--===============1034449608474826383==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============1034449608474826383==--