cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: commit 6523c068695d0431070060667c222eb40d54b14d breaks network removal
Date Mon, 10 Feb 2014 19:51:19 GMT
ok,
I was wondering about side effects of the checks.
thanks,

On Mon, Feb 10, 2014 at 6:46 PM, Alena Prokharchyk
<Alena.Prokharchyk@citrix.com> wrote:
> No big difference. But its better to evaluate if the network is eligible
> for the check, before retrieving DHCP provider. IN your code snippet you
> might retrieve DHCP provider only to discover later that the check is not
> needed at all.
>
> -Alena.
>
> On 2/8/14, 7:07 AM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:
>
>>Alena,
>>
>>I added unit tests and made a review request for you,
>>https://reviews.apache.org/r/17872/
>>
>>I have a question though. I can see some small the semantic differnce
>>between the following two snippits but is only in the evaluation order
>>of the conditions under which to execute, not in the logic.  So what
>>is the importance of using your version? (just curious)
>>
>><mine>
>>        if (vm.getType() == Type.User
>>                &&
>>_networkModel.areServicesSupportedInNetwork(network.getId(),Service.Dhcp))
>>{
>>            // remove the dhcpservice ip if this is the last nic in
>>subnet.
>>            DhcpServiceProvider dhcpServiceProvider =
>>getDhcpServiceProvider(network);
>>            if (dhcpServiceProvider != null
>>                    &&
>>isDhcpAccrossMultipleSubnetsSupported(dhcpServiceProvider)
>>                    && isLastNicInSubnet(nic)
>>                    && network.getTrafficType() == TrafficType.Guest
>>                    && network.getGuestType() == GuestType.Shared) {
>>                removeDhcpServiceInSubnet(nic);
>>            }
>>        }
>></mine>
>>
>><yours>
>>if (vm.getType() == Type.User
>> && network.getTrafficType() == TrafficType.Guest
>> && isLastNicInSubnet(nic)
>> && network.getGuestType() == GuestType.Shared
>> &&
>>_networkModel.areServicesSupportedInNetwork(network.getId(),Service.Dhcp))
>>{
>>  //2) Now get the DHCP provider, and do the rest of the checks
>>  DhcpServiceProvider dhcpServiceProvider =
>>getDhcpServiceProvider(network);
>>  if (dhcpServiceProvider != null
>>     && isDhcpAccrossMultipleSubnetsSupported(dhcpServiceProvider)) {
>>     removeDhcpServiceInSubnet(nic);
>> }
>>}
>></yours>
>>
>>On Fri, Feb 7, 2014 at 8:30 PM, Daan Hoogland <daan.hoogland@gmail.com>
>>wrote:
>>> sure, will try to find a spot asap. and write unit tests to simulate
>>> those two situations.
>>>
>>> On Fri, Feb 7, 2014 at 7:20 PM, Alena Prokharchyk
>>> <Alena.Prokharchyk@citrix.com> wrote:
>>>> Daan,
>>>>
>>>> Here is how it should look:
>>>>
>>>> //1) Make all the checks that used to exist in original code + if DHCP
>>>> service is enabled on the network
>>>> if (vm.getType() == Type.User && network.getTrafficType() ==
>>>> TrafficType.Guest && isLastNicInSubnet(nic) && network.getGuestType()
>>>>==
>>>> GuestType.Shared &&
>>>>
>>>>_networkModel.areServicesSupportedInNetwork(network.getId(),Service.Dhcp
>>>>))
>>>> {
>>>>
>>>>    //2) Now get the DHCP provider, and do the rest of the checks
>>>>    DhcpServiceProvider dhcpServiceProvider =
>>>> getDhcpServiceProvider(network);
>>>>    if (dhcpServiceProvider != null &&
>>>> isDhcpAccrossMultipleSubnetsSupported(dhcpServiceProvider)) {
>>>>       removeDhcpServiceInSubnet(nic);
>>>>   }
>>>> }
>>>>
>>>>
>>>>
>>>> Could you please test it for 2 Shared networks - one with DHCP service,
>>>> and one w/o?
>>>>
>>>> Thank you!
>>>> Alena.
>>>>
>>>>
>>>>
>>>> On 2/7/14, 10:04 AM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:
>>>>
>>>>>H Alena,
>>>>>
>>>>>I am just trying to fix an old contribution that I applied as it
>>>>>seemed not to harm in a basic test. revert didn't work so I am looking
>>>>>for a quick remedy. The original patch does it for shared only. I
>>>>>don't care either way. Lets do the best thing.
>>>>>
>>>>>the code now
>>>>>
>>>>>        if (vm.getType() == Type.User
>>>>>                &&
>>>>>_networkModel.areServicesSupportedInNetwork(network.getId(),
>>>>>Service.Dhcp)
>>>>>                && network.getTrafficType() == TrafficType.Guest
>>>>>                && network.getGuestType() == GuestType.Shared)
{
>>>>>            // remove the dhcpservice ip if this is the last nic in
>>>>>subnet.
>>>>>            DhcpServiceProvider dhcpServiceProvider =
>>>>>getDhcpServiceProvider(network);
>>>>>            if (dhcpServiceProvider != null
>>>>>                    &&
>>>>>isDhcpAccrossMultipleSubnetsSupported(dhcpServiceProvider)
>>>>>                    && isLastNicInSubnet(nic)) {
>>>>>                removeDhcpServiceInSubnet(nic);
>>>>>            }
>>>>>        }
>>>>>
>>>>>What do you sugest?
>>>>>
>>>>>        if (vm.getType() == Type.User
>>>>>                &&
>>>>>_networkModel.areServicesSupportedInNetwork(network.getId(),
>>>>>Service.Dhcp)) {
>>>>>            // remove the dhcpservice ip if this is the last nic in
>>>>>subnet.
>>>>>            DhcpServiceProvider dhcpServiceProvider =
>>>>>getDhcpServiceProvider(network);
>>>>>            if (dhcpServiceProvider != null
>>>>>                    &&
>>>>>isDhcpAccrossMultipleSubnetsSupported(dhcpServiceProvider)
>>>>>                    && isLastNicInSubnet(nic)
>>>>>                && network.getTrafficType() == TrafficType.Guest
>>>>>                && network.getGuestType() == GuestType.Shared)
{
>>>>>                removeDhcpServiceInSubnet(nic);
>>>>>            }
>>>>>        }
>>>>>
>>>>>???
>>>>>
>>>>>On Fri, Feb 7, 2014 at 6:56 PM, Alena Prokharchyk
>>>>><Alena.Prokharchyk@citrix.com> wrote:
>>>>>> Daan,
>>>>>>
>>>>>> 1) What is the reason you execute this code snippet just for Shared
>>>>>> networks?
>>>>>> 2) As I suggested in my prev email, before retrieving Dhcpprovider,
>>>>>>you
>>>>>> should check if dhcp service is enabled on the network. Use method
>>>>>> areServicesSupportedInNetwork
>>>>>>  From NetworkModel to check that.
>>>>>>
>>>>>> -Alena.
>>>>>>
>>>>>> On 2/6/14, 10:04 PM, "Daan Hoogland" <daan.hoogland@gmail.com>
wrote:
>>>>>>
>>>>>>>Alena,
>>>>>>>
>>>>>>>The revert didn't apply. Would the folowing do the trick?
>>>>>>>
>>>>>>>        if (vm.getType() == Type.User
>>>>>>>                && network.getTrafficType() == TrafficType.Guest
>>>>>>>                && network.getGuestType() == GuestType.Shared)
{
>>>>>>>            // remove the dhcpservice ip if this is the last nic
in
>>>>>>>subnet.
>>>>>>>            DhcpServiceProvider dhcpServiceProvider =
>>>>>>>getDhcpServiceProvider(network);
>>>>>>>            if (dhcpServiceProvider != null
>>>>>>>                    &&
>>>>>>>isDhcpAccrossMultipleSubnetsSupported(dhcpServiceProvider)
>>>>>>>                    && isLastNicInSubnet(nic)) {
>>>>>>>                removeDhcpServiceInSubnet(nic);
>>>>>>>            }
>>>>>>>        }
>>>>>>>
>>>>>>>On Fri, Feb 7, 2014 at 6:55 AM, Daan Hoogland
>>>>>>><daan.hoogland@gmail.com>
>>>>>>>wrote:
>>>>>>>> second thought,
>>>>>>>>
>>>>>>>> Soheils mail bounces and the commit does not refer a ticket
from
>>>>>>>>jira.
>>>>>>>> I am going to revert. I should have been more vigilant. sorry.
>>>>>>>>
>>>>>>>> On Fri, Feb 7, 2014 at 6:49 AM, Daan Hoogland
>>>>>>>><daan.hoogland@gmail.com>
>>>>>>>>wrote:
>>>>>>>>> will do Alena,
>>>>>>>>>
>>>>>>>>> thanks for the headsup
>>>>>>>>>
>>>>>>>>> On Thu, Feb 6, 2014 at 10:42 PM, Alena Prokharchyk
>>>>>>>>> <Alena.Prokharchyk@citrix.com> wrote:
>>>>>>>>>> Soheil/Daan,
>>>>>>>>>>
>>>>>>>>>> The commit in the subject breaks network System vms
destroy (VR,
>>>>>>>>>>SSVM,
>>>>>>>>>> CPVM), resulting in the network removal failures.
Following line
>>>>>>>>>>replacement
>>>>>>>>>> causes the failure:
>>>>>>>>>>
>>>>>>>>>> - if (vm.getType() == Type.User &&
>>>>>>>>>> isDhcpAccrossMultipleSubnetsSupported(network) &&
>>>>>>>>>>isLastNicInSubnet(nic) &&
>>>>>>>>>> network.getTrafficType() == TrafficType.Guest
>>>>>>>>>>
>>>>>>>>>> With
>>>>>>>>>>
>>>>>>>>>> +        DhcpServiceProvider dhcpServiceProvider
=
>>>>>>>>>> getDhcpServiceProvider(network);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> When you try to call getDhcpServiceProvider(network),
it throws
>>>>>>>>>>an
>>>>>>>>>>exception
>>>>>>>>>> because DHCP service is not enabled in Public/Control
networks of
>>>>>>>>>>system vms
>>>>>>>>>> nics. So system vm always fails to expunge.
>>>>>>>>>>
>>>>>>>>>> Could you please fix it by checking if DHCP service
is enabled on
>>>>>>>>>>the
>>>>>>>>>> network, before getting the DHCP service provider?
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Alena.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Daan
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Daan
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>--
>>>>>>>Daan
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>--
>>>>>Daan
>>>>
>>>
>>>
>>>
>>> --
>>> Daan
>>
>>
>>
>>--
>>Daan
>



-- 
Daan

Mime
View raw message