Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve network manager refresh speed #216

Merged
merged 6 commits into from
Feb 23, 2018

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Jan 31, 2018

@miq-bot miq-bot added the wip label Jan 31, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

This pull request is not mergeable. Please rebase and repush.

@mansam mansam force-pushed the improve-network-manager-refresh-speed branch from 7bcdaba to 119a23b Compare January 31, 2018 21:11
@mansam mansam force-pushed the improve-network-manager-refresh-speed branch from 119a23b to 49475d5 Compare January 31, 2018 21:32
end.compact
end

def cloud_subnets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mansam do we have a targeted refresh spec around these? To make sure it doesn't duplicate any models?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I wonder if targeted refresh for subnets works correctly now, the way it is now, they were nested under cloud networks. So for targeted here https://github.com/Ladas/manageiq-providers-openstack/blob/4b784dec74860e678a660002c5d3fca93ace1e33/app/models/manageiq/providers/openstack/inventory/persister/target_collection.rb#L31

we would need to say subnets have parent_inventory_collections => :cloud_networks and we would need to define :targeted_arel that returns subnets of all referenced cloud_networks

network.maximum_transmission_unit = n["mtu"]
network.cloud_tenant = persister.cloud_tenants.lazy_find(n["tenant_id"])
network.orchestration_stack = persister.orchestration_stacks.lazy_find(collector.orchestration_stack_by_resource_id(n["id"]))
n["subnets"].each do |subnet_id|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to model these as in AWS? Where networks and subnets are 2 separate collections? It would require subnet events for targeted refresh

@miq-bot
Copy link
Member

miq-bot commented Feb 1, 2018

This pull request is not mergeable. Please rebase and repush.

@mansam mansam force-pushed the improve-network-manager-refresh-speed branch from 49475d5 to a8a5990 Compare February 2, 2018 21:30
@mansam mansam force-pushed the improve-network-manager-refresh-speed branch from 670b90b to e7994d5 Compare February 8, 2018 19:35
@miq-bot
Copy link
Member

miq-bot commented Feb 12, 2018

This pull request is not mergeable. Please rebase and repush.

@mansam mansam force-pushed the improve-network-manager-refresh-speed branch from e7994d5 to 780eb6d Compare February 19, 2018 21:27
@miq-bot
Copy link
Member

miq-bot commented Feb 19, 2018

Some comments on commits mansam/manageiq-providers-openstack@e263101~...780eb6d

spec/models/manageiq/providers/openstack/network_manager/cloud_subnet_refresh_spec.rb

  • ⚠️ - 131 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 145 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Feb 19, 2018

Checked commits mansam/manageiq-providers-openstack@e263101~...780eb6d with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
35 files checked, 2 offenses detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

app/models/manageiq/providers/openstack/network_manager/refresh_parser.rb

@mansam mansam changed the title [wip] Improve network manager refresh speed Improve network manager refresh speed Feb 21, 2018
@miq-bot miq-bot removed the wip label Feb 21, 2018
@aufi
Copy link
Member

aufi commented Feb 23, 2018

Not sure about @Ladas comment, otherwise looks good to me 👍

@Ladas
Copy link
Contributor

Ladas commented Feb 23, 2018

@aufi right, I think we should backport it, then everything will be awesome :-)

@aufi aufi merged commit cfc0678 into ManageIQ:master Feb 23, 2018
@aufi aufi added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 23, 2018
@mansam
Copy link
Contributor Author

mansam commented Mar 8, 2018

Since this needs to be backported to Fine (ManageIQ/manageiq#16695) to fix the BZ, it should probably be backported to Gaprindashvili as well.

@simaishi
Copy link
Contributor

@mansam If this PR can go to gaprindashvili as is, please add gaprindashvili/yes label. Not sure if this will conflict on VCR and you'll need to create a separate PR for gaprindashvili as well...?

@mansam
Copy link
Contributor Author

mansam commented Mar 12, 2018

@simaishi It'll probably conflict so I'll open up a new PR for it

@simaishi
Copy link
Contributor

Backported to Gaprindashvili via #251

@simaishi
Copy link
Contributor

Backported to Fine via ManageIQ/manageiq#16695

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants