my code doesn't work when the vlan comes in as numeric. I don't claim
that. it only works when it comes in as vlan://<number>. It is then
and only then properly parsed into a numeric value.
On Fri, Nov 8, 2013 at 7:23 PM, Sheng Yang <sh...@yasker.org> wrote:
> Here is the part in your commit about SRX:
>
> diff --git
> a/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> b/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> index 3bcbf2d..46ef332 100644
> ---
> a/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> +++
> b/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> @@ -65,6 +65,7 @@ import com.cloud.agent.api.to.IpAddressTO;
> import com.cloud.agent.api.to.PortForwardingRuleTO;
> import com.cloud.agent.api.to.StaticNatRuleTO;
> import com.cloud.host.Host;
> +import com.cloud.network.Networks.BroadcastDomainType;
> import com.cloud.network.rules.FirewallRule;
> import com.cloud.network.rules.FirewallRule.Purpose;
> import com.cloud.resource.ServerResource;
> @@ -698,8 +699,7 @@ public class JuniperSrxResource implements
> ServerResource {
> Long publicVlanTag = null;
> if (ip.getVlanId() != null &&
> !ip.getVlanId().equals("untagged")) {
> try {
> - // TODO BroadcastDomain.getValue(ip.getVlanId) ???
> - publicVlanTag = Long.parseLong(ip.getVlanId());
> + publicVlanTag =
> Long.parseLong(BroadcastDomainType.getValue(ip.getVlanId()));
> } catch (Exception e) {
> throw new ExecutionException("Could not parse
> public VLAN tag: " + ip.getVlanId());
> }
> @@ -3581,7 +3581,8 @@ public class JuniperSrxResource implements
> ServerResource {
> Long publicVlanTag = null;
> if (!vlan.equals("untagged")) {
> try {
> - publicVlanTag = Long.parseLong(vlan);
> + // make sure this vlan is numeric
> + publicVlanTag =
> Long.parseLong(BroadcastDomainType.getValue(vlan));
> } catch (Exception e) {
> throw new ExecutionException("Unable to parse VLAN
> tag: " + vlan);
> }
>
> In the code, seems you want to make sure vlan is numeric and let
> BroadcastDomainType.getValue() accept it(which would only accept URI)? I
> don't think that would work.
>
> --Sheng
>
>
>
> On Fri, Nov 8, 2013 at 10:15 AM, Sheng Yang <sh...@yasker.org> wrote:
>
>> Hi Daan,
>>
>> Your commit has broken BOTH VMware and JuniperSRX, and these are only two
>> functional codes in your commit. The other two modification are comments.
>>
>> So I don't know how your testing can possibly work.
>>
>> Alena has already fixed the Juniper part at:
>>
>> commit 3ab8d8d8f20c453fdc684f177a612922eae5f415
>> Author: Alena Prokharchyk <alena.prokharc...@citrix.com>
>> Date: Wed Sep 18 16:37:00 2013 -0700
>>
>> Fixed non-oss build broken in Juniper SRX with commit
>> 2614b00c513734ce6b1c29e572fbd7a37d4059fc
>>
>> BTW, I think we can talk about how to improve the format of URI/vlanid
>> definition and how to fix it in the future, but it's another story and
>> shouldn't be mixed with the broken commit issue.
>>
>> --Sheng
>>
>>
>> On Fri, Nov 8, 2013 at 9:37 AM, Min Chen <min.c...@citrix.com> wrote:
>>
>>> Daan,
>>>
>>> I am curious about this: you mentioned that your patch has been
>>> running
>>> fine on a mixed xen/vmware cloud environment. How come you didn't
>>> encounter the blocker bug filed by Rayees on our automation environment on
>>> master branch: https://issues.apache.org/jira/browse/CLOUDSTACK-5046. Are
>>> you not using advanced zone? Your change I fixed in commit
>>> 94f9b31c9a4c7ae67feabbe16d2ea753e3183289 will definitely lead to this
>>> NumberFormatException in a vmware advanced zone setup in starting system
>>> vm.
>>> Anyway, my fix 94f9b31c9a4c7ae67feabbe16d2ea753e3183289 is for
>>> resolving
>>> that vmware blocker defect 5046. If you feel that it may break other SDN
>>> implementation, please submit a better fix that can work for both SDN and
>>> Vmware. That is also the original intention I raised in this thread, since
>>> I am not fully aware of the context of your patch change.
>>>
>>> Thanks
>>> -min
>>>
>>> On 11/8/13 2:43 AM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote:
>>>
>>> >H Sheng,
>>> >
>>> >All sdn implementations use the field in one way or another as uri. So
>>> >if this is wrong reverting my patch won't do the trick, I am afraid. I
>>> >actually think the field should be renamed to broadcastUri, as this is
>>> >how it is used at the moment by a lot of elements.
>>> >
>>> >Going back to only allowing vlans as isolation method is not going to
>>> >solve anything as the feature is still desired. The alternative is
>>> >adding a field/parameter all over the code and have all methods check
>>> >both fields for valid values to decide what to do.
>>> >
>>> >The patch runs in production at the moment on a 4.1.1 fork on a xen
>>> >based cloud and on a mixed xen/vmware cloud as well. It has been
>>> >heavily tested in master. I don't value my implementation of the
>>> >requirement and am happy to discuss better implementations and the
>>> >scope of validity of vlan-ids and broadcast uris but the feature it
>>> >fulfills is very needed (and works).
>>> >
>>> >kind regards,
>>> >Daan
>>> >
>>> >On Fri, Nov 8, 2013 at 1:35 AM, Sheng Yang <sh...@yasker.org> wrote:
>>> >> Hi Daan,
>>> >>
>>> >> I think your patch is completely wrong.
>>> >>
>>> >> The BroadcastDomainType.getValue() would accept format of URI, not the
>>> >> number. I don't think your change would work, either in code or by
>>> >>semantic.
>>> >>
>>> >> I think your commit would break all elements your code touched. The
>>> >>current
>>> >> assumption of vlanId and secondaryVlanId are both numbers, not URI.
>>> >>
>>> >> Please revert it.
>>> >>
>>> >> --Sheng
>>> >>
>>> >>
>>> >> On Thu, Nov 7, 2013 at 4:07 PM, Min Chen <min.c...@citrix.com> wrote:
>>> >>>
>>> >>> Then we need to clearly define the semantic of parameter "vlanId" of
>>> >>> HypervisorHostHelper.prepareNetwork is indeed the vlan Id string, not
>>> >>> other format. The invoker method should pass the correct one to this
>>> >>> utility to make it work.
>>> >>>
>>> >>> Thanks
>>> >>> -min
>>> >>>
>>> >>> On 11/7/13 3:43 PM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote:
>>> >>>
>>> >>> >Doesn't sound like a guarantee, If the stack is build otherwise there
>>> >>> >might come in a different (non-integer-representing) string.
>>> >>> >
>>> >>> >On Thu, Nov 7, 2013 at 6:08 PM, Min Chen <min.c...@citrix.com>
>>> wrote:
>>> >>> >> Yes. From callstack,
>>> >>> >> BroadcastDomainType.getValue(BroadcastDomainType.fromString(vlanId)
>>> >>>is
>>> >>> >> already called before going to that routine.
>>> >>> >>
>>> >>> >> Thanks
>>> >>> >> -min
>>> >>> >>
>>> >>> >> On 11/7/13 1:51 AM, "Daan Hoogland" <daan.hoogl...@gmail.com>
>>> wrote:
>>> >>> >>
>>> >>> >>>H Min,
>>> >>> >>>
>>> >>> >>>Your fix will work if you can guarantee that the String passed is a
>>> >>> >>>integer. if it has the chance of being in the form of for instance
>>> >>> >>>vlan://<some_number> , you should use:
>>> >>> >>>vid =
>>> >>>
>>> >>> >>>
>>>
>>> >>>>>>Integer.parseInt(BroadcastDomainType.getValue(BroadcastDomainType.fro
>>> >>>>>>mSt
>>> >>> >>>ri
>>> >>> >>>ng(vlanId)));
>>> >>> >>>
>>> >>> >>>regards,
>>> >>> >>>Daan
>>> >>> >>>
>>> >>> >>>
>>> >>> >>>On Wed, Nov 6, 2013 at 3:25 AM, Min Chen <min.c...@citrix.com>
>>> >>>wrote:
>>> >>> >>>> Hi Daan,
>>> >>> >>>>
>>> >>> >>>> Your commit
>>> >>> >>>>
>>> >>>
>>> >>> >>>>
>>> >>>>>>>
>>> https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;h=
>>> >>>>>>>25c
>>> >>> >>>>8c
>>> >>> >>>>ee01a450ee924fe108cafe54b046485ab2b
>>> >>> >>>> broke Vmware advanced zone setup on Master, where system vm
>>> starts
>>> >>> >>>>failed
>>> >>> >>>> with "NumberFormatException", see details in
>>> >>> >>>> https://issues.apache.org/jira/browse/CLOUDSTACK-5046. I fixed
>>> >>>this
>>> >>> >>>>blocker
>>> >>> >>>> bug (commit 94f9b31c9a4c7ae67feabbe16d2ea753e3183289) by
>>> reverting
>>> >>> >>>>part
>>> >>> >>>>of
>>> >>> >>>> your change on HypervisorHostHelper method, but not sure if there
>>> >>>is
>>> >>> >>>>any
>>> >>> >>>> side effect on your another functionality fixed in your commit.
>>> >>>Can
>>> >>> >>>>you
>>> >>> >>>> please review to make sure that it is ok?
>>> >>> >>>>
>>> >>> >>>> Thanks
>>> >>> >>>> -min
>>> >>> >>
>>> >>>
>>> >>
>>>
>>>
>>
