Return-Path: X-Original-To: apmail-cloudstack-dev-archive@www.apache.org Delivered-To: apmail-cloudstack-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id F27191048F for ; Fri, 8 Nov 2013 18:11:09 +0000 (UTC) Received: (qmail 12829 invoked by uid 500); 8 Nov 2013 18:10:51 -0000 Delivered-To: apmail-cloudstack-dev-archive@cloudstack.apache.org Received: (qmail 12745 invoked by uid 500); 8 Nov 2013 18:10:48 -0000 Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cloudstack.apache.org Delivered-To: mailing list dev@cloudstack.apache.org Received: (qmail 12540 invoked by uid 99); 8 Nov 2013 18:10:39 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 08 Nov 2013 18:10:39 +0000 X-ASF-Spam-Status: No, hits=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of daan.hoogland@gmail.com designates 209.85.223.170 as permitted sender) Received: from [209.85.223.170] (HELO mail-ie0-f170.google.com) (209.85.223.170) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 08 Nov 2013 18:10:33 +0000 Received: by mail-ie0-f170.google.com with SMTP id at1so3912173iec.29 for ; Fri, 08 Nov 2013 10:10:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=QDTvPvWCaoQGju8nY19XNpKYgDaoSJusnxAYuXUQd18=; b=jI4u97oSjLShSFUop2kHrBrtsfuSKNs2kSzUHPTCF0azXZ/eOyghpxJ8DFylSvNCr5 now4ij6exeBKkUWRC9XQ04T9h+Gx84RMSn0Otjr1Rae3t38y6tT6P9ODfe20ppAJ/RrF agXfsW9+QsOQjOAaNfZepZ7egVXitctOy1Y6220jk2m6hb1Dysrlr0eQJlL12MNOEuST YZYLoqk5dPGPN3xfQx+PrGhgR6QPbvBB+qLrhwIPjMkjlHHgYtXck1QNHydgM9riOMwI +ASkGn4Af9n0QK4vNi8liZkOWPBJg9556Uk9WSTEB8FuvE+4rRSPFq5a+40OvqpwRoLq 9j6g== X-Received: by 10.50.1.102 with SMTP id 6mr3459398igl.0.1383934212524; Fri, 08 Nov 2013 10:10:12 -0800 (PST) MIME-Version: 1.0 Received: by 10.64.226.135 with HTTP; Fri, 8 Nov 2013 10:09:52 -0800 (PST) In-Reply-To: References: From: Daan Hoogland Date: Fri, 8 Nov 2013 19:09:52 +0100 Message-ID: Subject: Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master To: dev Cc: Sheng Yang , Hugo Trippaers , Roeland Kuipers Content-Type: text/plain; charset=ISO-8859-1 X-Virus-Checked: Checked by ClamAV on apache.org 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 and as vlan://. 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 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" 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 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 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" 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 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" 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:// , 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 >>>>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 >>>> >> >>>> >>> >