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