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