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

Add 'default_security_group' method to CloudTenant #347

Merged

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Sep 7, 2018

Adds a default_security_group instance method to CloudTenant that returns a security group named default if that CloudTenant has one, or else returns the security group with the highest population of VMs.

@mansam mansam force-pushed the add-cloud-tenant-default-security-group-method branch from 369b00c to bb393fa Compare September 7, 2018 19:13
@mansam mansam force-pushed the add-cloud-tenant-default-security-group-method branch from bb393fa to 2ec415b Compare September 10, 2018 13:16
@miq-bot
Copy link
Member

miq-bot commented Sep 10, 2018

Checked commit mansam@2ec415b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 3 offenses detected

spec/models/manageiq/providers/openstack/cloud_manager/cloud_tenant_spec.rb

@aufi
Copy link
Member

aufi commented Sep 11, 2018

Looks good to me, merging 👍

return default if default
# if there's not a security group named "default",
# then return the security group with the most VMs in it.
security_groups.sort { |sg| sg.vms.count }.last
Copy link
Contributor

Choose a reason for hiding this comment

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

btw. this looks pretty inefficient, there can be a lot of security groups

@kbrock
Copy link
Member

kbrock commented Sep 12, 2018

This method is really bad. the large number of security groups and then the N+1 count for each of the vm counts is really bad.

Also, making this a virtual attribute means it is exposed for everyone - it probably needs to move to the core

It still is a bunch of work, but at least is done on the database:
security_groups.order(:total_vms).last

I'll ping @Ladas

@mansam
Copy link
Contributor Author

mansam commented Sep 12, 2018

@Ladas @kbrock Ordering via security_groups.order(:total_vms) throws an exception -- missing method 'asc' for NilClass, at least in my rails console. I'm working on writing a better join since this indeed isn't efficient enough.

@kbrock
Copy link
Member

kbrock commented Sep 12, 2018

@mansam thanks - will look into that

@kbrock
Copy link
Member

kbrock commented Sep 12, 2018

@mansam you are right - it looks like that is using a :through which currently does not handle this abstraction.

Will try and think up another way to avoid the N+1 queries

@aufi aufi added this to the Sprint 94 Ending Sep 10, 2018 milestone Sep 25, 2018
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