cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Date Fri, 08 Nov 2013 18:09:52 GMT
Min,

I realize that you fixed something and didn't just change code because
of no reason. The point is that a vlan might be identified both as
<number> and as vlan://<number>. the patch is part of a series that
addresses these situations. I will look at this case and make sure
either only one is possible or both are accepted.

regards,
Daan

On Fri, Nov 8, 2013 at 6:37 PM, Min Chen <min.chen@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.hoogland@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 <sheng@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.chen@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.hoogland@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.chen@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.hoogland@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.chen@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
>>>> >>
>>>>
>>>
>

Mime
View raw message