Sheng, Min, The use of the word semantics has triggered a line of though I want to share on this. The word vlanid is being used as a homonym in cloudstack, meaning both vlan-number and broadcast-uri-pertaining-to-a-vlan. This has been so since 4.0 and maybe even longer. As I see it; Assuming a vlanid is a number and then passing it as a string is wrong. Assuming a number is a vlanid is a wrong assumption as well.
I looked at the hypervisorhelper prepareNetwork member methods again and it seems that these expect 1-based 12-bit unsigned integers. They accept anything encoded as a string, however. We could do one of the following things: 1. changing the footprint of those methods or 2. checking and explicitly throwing an exception if this is not the case. 3. is the one I proposed earlier in this thread. This is the most forgiving one. Reverting will lead to runtime exceptions as well. These are why this is in in the first place. regards, Daan On Fri, Nov 8, 2013 at 11: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.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 >>> >> >>> >>
