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