ok, I was wondering about side effects of the checks. thanks, On Mon, Feb 10, 2014 at 6:46 PM, Alena Prokharchyk <alena.prokharc...@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.hoogl...@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.hoogl...@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.prokharc...@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.hoogl...@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.prokharc...@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.hoogl...@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.hoogl...@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.hoogl...@gmail.com> >>>>>>>>wrote: >>>>>>>>> will do Alena, >>>>>>>>> >>>>>>>>> thanks for the headsup >>>>>>>>> >>>>>>>>> On Thu, Feb 6, 2014 at 10:42 PM, Alena Prokharchyk >>>>>>>>> <alena.prokharc...@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
