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