Fixing the refresh to remove all physical switches. #17390
Conversation
CharlleDaniel
changed the title
CharlleDaniel
changed the title
|
@miq-bot add_label bug |
|
:( this is a method that I wish we had done differently, the real problem is that this will impact other providers switches. I'd prefer not to make this method even worse by tacking on "not physical switches" and instead fix the problem. I started a PR here to better model distributed and host-local switches for InfraManagers, #17420 we'll see how that goes. If that takes too long we can revisit this, if anything I'd prefer to scope this to just switches under this ems. |
| # delete from switches as s where s.id not in (select switch_id from host_switches) | ||
| # AND s.type not like '%PhysicalInfraManager%' | ||
| switch_ids = HostSwitch.all.collect(&:switch_id) | ||
| Switch.where.not(:id => switch_ids).where.not("type LIKE '%PhysicalInfraManager%'").destroy_all |
There was a problem hiding this comment.
Could you do just Switch.where.not(:id => switch_ids, :type => 'PhysicalSwitch').destroy_all
There was a problem hiding this comment.
hey @agrare the type is a little bigger ManageIQ::Providers::Lenovo::PhysicalInfraManager::PhysicalSwitch and if I use this Switch.where.not(:id => switch_ids, :type => "ManageIQ::Providers::Lenovo::PhysicalInfraManager::PhysicalSwitch").destroy_all I will only to fix Lenovo PhysicalSwitches and not for all PhysicalInfraStructure. What do you think?
There was a problem hiding this comment.
Also, today we have a new PhysicalInfraStructure the RedFish provider.
There was a problem hiding this comment.
Okay how about this, before we save any inventory, up here we collect all switch ids up front by doing ems.switches.pluck(:id) which we then pass into this method. That will give us the scope of switches that we are going to delete here and not impact other providers at all.
|
While I think #17420 is the right way to fix this, I get that is taking a long time and this will solve things in the short term. @CharlleDaniel just the one change and then this looks okay to me |
CharlleDaniel
force-pushed
the
Fix_remove_obsole_switches
branch
from
f880ad7 to
234b64c
Compare
| # delete from switches as s where s.shared is NULL and s.id not in (select switch_id from host_switches) | ||
| # delete from switches as s where s.shared = 't' and s.id not in (select switch_id from host_switches) | ||
| Switch.where.not(:id => HostSwitch.all.collect(&:switch).uniq).destroy_all | ||
| switch_ids = HostSwitch.all.collect(&:switch_id) | ||
| switches.where.not(:id => switch_ids).destroy_all |
There was a problem hiding this comment.
@agrare It's works. Is it ok for you?
There was a problem hiding this comment.
At this point the switches have already been saved so this won't actually disconnect anything.
I suggested collecting the IDs here since that will be the original list. You can then destroy the old_switch_ids - switch_ids
CharlleDaniel
force-pushed
the
Fix_remove_obsole_switches
branch
from
234b64c to
c5afcb6
Compare
| @@ -64,7 +64,7 @@ def save_ems_infra_inventory(ems, hashes, target = nil, disconnect = true) | |||
| :orchestration_templates, | |||
| :orchestration_stacks | |||
| ] | |||
|
|
|||
| old_switch_ids = ems.switches.pluck(:id) | |||
There was a problem hiding this comment.
@agrare Is this your suggestion?
| # delete from switches as s where s.shared is NULL and s.id not in (select switch_id from host_switches) | ||
| # delete from switches as s where s.shared = 't' and s.id not in (select switch_id from host_switches) | ||
| Switch.where.not(:id => HostSwitch.all.collect(&:switch).uniq).destroy_all | ||
| obsolete_switch_ids = old_switch_ids - HostSwitch.all.collect(&:switch_id) |
There was a problem hiding this comment.
Since our old_switch_ids is scoped down to just this ems (as it should be) we should use - ems.switches.reload.pluck(:id)
There was a problem hiding this comment.
@agrare I'm not sure, but think we should consider obsolete only switches that not have relationship with hosts, what do you think?
There was a problem hiding this comment.
Because if we use obsolete_switches - ems.switches.reload.pluck(:id) we are only considering the difference between lists
There was a problem hiding this comment.
Because if we use
obsolete_switches - ems.switches.reload.pluck(:id)we are only considering the difference between lists
Yeah that's the idea, we basically want to replicate the behavior of using the association to do the deletes.
There was a problem hiding this comment.
oh, I got it. I will change this right now.
CharlleDaniel
force-pushed
the
Fix_remove_obsole_switches
branch
from
c5afcb6 to
a3cd885
Compare
|
Checked commit CharlleDaniel@a3cd885 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@CharlleDaniel this looks good I just want to give it a quick test before merging |
|
@agrare ok, thanks. |
|
Thanks for bearing with me @CharlleDaniel ! |
This PR is able to:
PhysicalSwitchesafter an infrastructure provider be refreshed.Issue: #17383 - PhysicalSwitches being deleted after the Infrastructure provider refresh