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

OpenStack Nova enable/disable service #6996

Closed

Conversation

petrblaho
Copy link

This is WIP PR for enabling/disabling nova scheduling for host - which is needed to implement scaling down.

Parts of code needs to be refactored and there are no tests now.

@@ -34,6 +34,7 @@ def fetch(hash)
end

def save_inventory_multi(association, hashes, deletes, find_key, child_keys = [], extra_keys = [])

Copy link
Member

Choose a reason for hiding this comment

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

why this whitespace change in this PR?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that - this is remnant of me playing with that code. I will remove that in next update of PR.

@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from 0605d72 to 47daa52 Compare March 2, 2016 13:25
@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from 47daa52 to 49d9124 Compare March 16, 2016 13:35
@miq-bot
Copy link
Member

miq-bot commented Mar 18, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from 49d9124 to ba688e6 Compare March 31, 2016 17:28
@petrblaho petrblaho changed the title [WIP] OpenStack Nova enable/disable service OpenStack Nova enable/disable service Mar 31, 2016
@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from ba688e6 to 4a474c6 Compare March 31, 2016 18:54
@chessbyte chessbyte removed the wip label Apr 1, 2016
@chessbyte
Copy link
Member

@blomquisg please review

@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from 4a474c6 to 864c8fc Compare April 4, 2016 09:55
@blomquisg blomquisg added wip and removed wip labels Apr 5, 2016
@petrblaho
Copy link
Author

@Ladas please can you look at this?

nova_fog_service.enable
end

def nova_fog_disable_service
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a comment by @blomquisg on my PR suggesting the removal of the fog-related part in the method name -- i.e. ironic_fog_set_node_maintenance shortened to set_node_maintenance. I'm not sure what the convention is for provider API calls that call out to fog, but these method names would be in a similar category -- whatever we do should probably be consistent across these features. Here's a link to the comment:
#7528 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I prefer to not prefix the methods with nova_fog.

Also, is 'nova-compute' going to be the only service that gets disabled/enabled?

@blomquisg
Copy link
Member

I'm a bit confused by this PR. (Some of this repeats what I put in line comments)

  1. I thought @Ladas already captured the services from host smart state, though that might not capture the scheduling status for those services (enabled vs. disabled).
  2. I'm not sure why we'd only allow enabling and disabling of nova-compute.
  3. I'm not sure why we'd only capture the scheduling status of nova-compute.

@Ladas and @petrblaho, thoughts?

@petrblaho
Copy link
Author

@blomquisg

I thought @Ladas already captured the services from host smart state, though that might not capture the scheduling status for those services (enabled vs. disabled).

Enabled/disabled for scheduling cannot be observed by parsing openstack-status report from host b/c this repost only refers to system services on that machine. Scheduling status is apart from active/enabled/disabled services on host and only means if this particular host (and service) will be considered as target when nova goes through decision on which host new VM will be spawned. So this has to be obtained via API call.

I'm not sure why we'd only allow enabling and disabling of nova-compute.
I'm not sure why we'd only capture the scheduling status of nova-compute.

In the future we can enable/disable scheduling even for cinder - so when it is needed this functionality should (and will) be generalized. Currently I do not know of any other services in OpenStack that has enabling/disabling of scheduling feature.

@miq-bot
Copy link
Member

miq-bot commented Apr 7, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from 864c8fc to c97bbde Compare April 8, 2016 11:16
Adds scheduling_status field for SystemService model.
Adds set of methods in Host::Operations to enable and disable scheduling
for Nova Compute service.

Adds system_services to Openstack::CloudManager::RefreshParser with
modification of save_system_services_inventory in
EmsRefresh::SaveInventory - this is ugly but needed to bridge over a fact
that data are coming from Cloud refresh but needed to be stored with
Infra's Host's SystemServices.

Modifies spec for Openstack Infra Refresher to include Infra + Cloud
situation - these tests different number of records in DB after refresh
of both Infra and Cloud is done.

Updates VCR Cassettes to reflect refresh call to get Openstack Services
information.
@petrblaho petrblaho force-pushed the nova-enable-disable-services branch from c97bbde to 76376b9 Compare April 8, 2016 12:32
@miq-bot
Copy link
Member

miq-bot commented Apr 8, 2016

Checked commit petrblaho@76376b9 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 4 offenses detected

app/models/ems_refresh/save_inventory.rb

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

spec/models/manageiq/providers/openstack/infra_manager/refresher_rhos_juno_spec.rb

end

def parse_service(service)
# <Fog::Compute::OpenStack::Service
Copy link
Contributor

Choose a reason for hiding this comment

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

delete these large comments please

def change
add_column :system_services, :scheduling_status, :string
end
end
Copy link
Member

Choose a reason for hiding this comment

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

@petrblaho Can you please pull this migration out into a separate PR so that it can be merged independent of the rest of the changes in this PR? We are approaching the deadline for schema changes for the darga branch, and I don't want the rest of the requested fixes in this PR to hold that up.

@miq-bot
Copy link
Member

miq-bot commented Apr 19, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@petrblaho
Copy link
Author

Closing in favor of schema changes in #8093 and functionality code in #7996

@petrblaho petrblaho closed this Apr 19, 2016
h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 12, 2020
Added and reorganized features under Chargeback Rates node to support changes in non-explorer version of Chargeback Rates screen.

Follow up PR for ManageIQ/manageiq-ui-classic#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 12, 2020
Added and reorganized features under Chargeback Rates node to support changes in non-explorer version of Chargeback Rates screen.

Follow up PR for ManageIQ/manageiq-ui-classic#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 12, 2020
Added and reorganized features under Chargeback Rates node to support changes in non-explorer version of Chargeback Rates screen.

Follow up PR for ManageIQ/manageiq-ui-classic#7016

Fixes ManageIQ#6996
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.

7 participants