You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Min Chen <mi...@citrix.com> on 2013/11/06 03:25:44 UTC
Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on
Master
Hi Daan,
Your commit https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;h=25c8cee01a450ee924fe108cafe54b046485ab2b 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
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Daan Hoogland <da...@gmail.com>.
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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
>>>> >>
>>>>
>>>
>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Daan Hoogland <da...@gmail.com>.
my code doesn't work when the vlan comes in as numeric. I don't claim
that. it only works when it comes in as vlan://<number>. It is then
and only then properly parsed into a numeric value.
On Fri, Nov 8, 2013 at 7:23 PM, Sheng Yang <sh...@yasker.org> wrote:
> Here is the part in your commit about SRX:
>
> diff --git
> a/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> b/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> index 3bcbf2d..46ef332 100644
> ---
> a/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> +++
> b/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
> @@ -65,6 +65,7 @@ import com.cloud.agent.api.to.IpAddressTO;
> import com.cloud.agent.api.to.PortForwardingRuleTO;
> import com.cloud.agent.api.to.StaticNatRuleTO;
> import com.cloud.host.Host;
> +import com.cloud.network.Networks.BroadcastDomainType;
> import com.cloud.network.rules.FirewallRule;
> import com.cloud.network.rules.FirewallRule.Purpose;
> import com.cloud.resource.ServerResource;
> @@ -698,8 +699,7 @@ public class JuniperSrxResource implements
> ServerResource {
> Long publicVlanTag = null;
> if (ip.getVlanId() != null &&
> !ip.getVlanId().equals("untagged")) {
> try {
> - // TODO BroadcastDomain.getValue(ip.getVlanId) ???
> - publicVlanTag = Long.parseLong(ip.getVlanId());
> + publicVlanTag =
> Long.parseLong(BroadcastDomainType.getValue(ip.getVlanId()));
> } catch (Exception e) {
> throw new ExecutionException("Could not parse
> public VLAN tag: " + ip.getVlanId());
> }
> @@ -3581,7 +3581,8 @@ public class JuniperSrxResource implements
> ServerResource {
> Long publicVlanTag = null;
> if (!vlan.equals("untagged")) {
> try {
> - publicVlanTag = Long.parseLong(vlan);
> + // make sure this vlan is numeric
> + publicVlanTag =
> Long.parseLong(BroadcastDomainType.getValue(vlan));
> } catch (Exception e) {
> throw new ExecutionException("Unable to parse VLAN
> tag: " + vlan);
> }
>
> In the code, seems you want to make sure vlan is numeric and let
> BroadcastDomainType.getValue() accept it(which would only accept URI)? I
> don't think that would work.
>
> --Sheng
>
>
>
> On Fri, Nov 8, 2013 at 10:15 AM, Sheng Yang <sh...@yasker.org> wrote:
>
>> Hi Daan,
>>
>> Your commit has broken BOTH VMware and JuniperSRX, and these are only two
>> functional codes in your commit. The other two modification are comments.
>>
>> So I don't know how your testing can possibly work.
>>
>> Alena has already fixed the Juniper part at:
>>
>> commit 3ab8d8d8f20c453fdc684f177a612922eae5f415
>> Author: Alena Prokharchyk <al...@citrix.com>
>> Date: Wed Sep 18 16:37:00 2013 -0700
>>
>> Fixed non-oss build broken in Juniper SRX with commit
>> 2614b00c513734ce6b1c29e572fbd7a37d4059fc
>>
>> BTW, I think we can talk about how to improve the format of URI/vlanid
>> definition and how to fix it in the future, but it's another story and
>> shouldn't be mixed with the broken commit issue.
>>
>> --Sheng
>>
>>
>> On Fri, Nov 8, 2013 at 9:37 AM, Min Chen <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
>>> >>> >>
>>> >>>
>>> >>
>>>
>>>
>>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Sheng Yang <sh...@yasker.org>.
Here is the part in your commit about SRX:
diff --git
a/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
b/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
index 3bcbf2d..46ef332 100644
---
a/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
+++
b/plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java
@@ -65,6 +65,7 @@ import com.cloud.agent.api.to.IpAddressTO;
import com.cloud.agent.api.to.PortForwardingRuleTO;
import com.cloud.agent.api.to.StaticNatRuleTO;
import com.cloud.host.Host;
+import com.cloud.network.Networks.BroadcastDomainType;
import com.cloud.network.rules.FirewallRule;
import com.cloud.network.rules.FirewallRule.Purpose;
import com.cloud.resource.ServerResource;
@@ -698,8 +699,7 @@ public class JuniperSrxResource implements
ServerResource {
Long publicVlanTag = null;
if (ip.getVlanId() != null &&
!ip.getVlanId().equals("untagged")) {
try {
- // TODO BroadcastDomain.getValue(ip.getVlanId) ???
- publicVlanTag = Long.parseLong(ip.getVlanId());
+ publicVlanTag =
Long.parseLong(BroadcastDomainType.getValue(ip.getVlanId()));
} catch (Exception e) {
throw new ExecutionException("Could not parse
public VLAN tag: " + ip.getVlanId());
}
@@ -3581,7 +3581,8 @@ public class JuniperSrxResource implements
ServerResource {
Long publicVlanTag = null;
if (!vlan.equals("untagged")) {
try {
- publicVlanTag = Long.parseLong(vlan);
+ // make sure this vlan is numeric
+ publicVlanTag =
Long.parseLong(BroadcastDomainType.getValue(vlan));
} catch (Exception e) {
throw new ExecutionException("Unable to parse VLAN
tag: " + vlan);
}
In the code, seems you want to make sure vlan is numeric and let
BroadcastDomainType.getValue() accept it(which would only accept URI)? I
don't think that would work.
--Sheng
On Fri, Nov 8, 2013 at 10:15 AM, Sheng Yang <sh...@yasker.org> wrote:
> Hi Daan,
>
> Your commit has broken BOTH VMware and JuniperSRX, and these are only two
> functional codes in your commit. The other two modification are comments.
>
> So I don't know how your testing can possibly work.
>
> Alena has already fixed the Juniper part at:
>
> commit 3ab8d8d8f20c453fdc684f177a612922eae5f415
> Author: Alena Prokharchyk <al...@citrix.com>
> Date: Wed Sep 18 16:37:00 2013 -0700
>
> Fixed non-oss build broken in Juniper SRX with commit
> 2614b00c513734ce6b1c29e572fbd7a37d4059fc
>
> BTW, I think we can talk about how to improve the format of URI/vlanid
> definition and how to fix it in the future, but it's another story and
> shouldn't be mixed with the broken commit issue.
>
> --Sheng
>
>
> On Fri, Nov 8, 2013 at 9:37 AM, Min Chen <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
>> >>> >>
>> >>>
>> >>
>>
>>
>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Sheng Yang <sh...@yasker.org>.
Hi Daan,
Your commit has broken BOTH VMware and JuniperSRX, and these are only two
functional codes in your commit. The other two modification are comments.
So I don't know how your testing can possibly work.
Alena has already fixed the Juniper part at:
commit 3ab8d8d8f20c453fdc684f177a612922eae5f415
Author: Alena Prokharchyk <al...@citrix.com>
Date: Wed Sep 18 16:37:00 2013 -0700
Fixed non-oss build broken in Juniper SRX with commit
2614b00c513734ce6b1c29e572fbd7a37d4059fc
BTW, I think we can talk about how to improve the format of URI/vlanid
definition and how to fix it in the future, but it's another story and
shouldn't be mixed with the broken commit issue.
--Sheng
On Fri, Nov 8, 2013 at 9:37 AM, Min Chen <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
> >>> >>
> >>>
> >>
>
>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Daan Hoogland <da...@gmail.com>.
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 <da...@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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
>>> >>
>>>
>>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on
Master
Posted by Min Chen <mi...@citrix.com>.
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" <da...@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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
>>> >>
>>>
>>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Daan Hoogland <da...@gmail.com>.
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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
>> >>
>>
>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Sheng Yang <sh...@yasker.org>.
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 <mi...@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" <da...@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 <mi...@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" <da...@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 <mi...@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
> >>
>
>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on
Master
Posted by Min Chen <mi...@citrix.com>.
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" <da...@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 <mi...@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" <da...@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 <mi...@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
>>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Daan Hoogland <da...@gmail.com>.
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 <mi...@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" <da...@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.fromStri
>>ng(vlanId)));
>>
>>regards,
>>Daan
>>
>>
>>On Wed, Nov 6, 2013 at 3:25 AM, Min Chen <mi...@citrix.com> wrote:
>>> Hi Daan,
>>>
>>> Your commit
>>>
>>>https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;h=25c8c
>>>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
>
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on
Master
Posted by Min Chen <mi...@citrix.com>.
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" <da...@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.fromStri
>ng(vlanId)));
>
>regards,
>Daan
>
>
>On Wed, Nov 6, 2013 at 3:25 AM, Min Chen <mi...@citrix.com> wrote:
>> Hi Daan,
>>
>> Your commit
>>
>>https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;h=25c8c
>>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
Re: Commit 25c8cee01a450ee924fe108cafe54b046485ab2b broke Vmware on Master
Posted by Daan Hoogland <da...@gmail.com>.
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.fromString(vlanId)));
regards,
Daan
On Wed, Nov 6, 2013 at 3:25 AM, Min Chen <mi...@citrix.com> wrote:
> Hi Daan,
>
> Your commit
> https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;h=25c8cee01a450ee924fe108cafe54b046485ab2b
> 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