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