Skip to content

Commit 492a6c4

Browse files
mriedemElod Illes
mriedem
authored and
Elod Illes
committed
Fix compute service set handling for 2.53+
With compute API microversion 2.53 there is a single PUT /os-services/{service_id} API which takes the service id as a UUID. Since the openstack compute service set command only takes --host and --service (binary) to identify the service, this change checks if 2.53 or greater is being used and if so, looks up the service by host and binary and calls the appropriate methods in novaclient. If the command cannot uniquely identify a compute service with the given host and binary, an error is raised. A future change could add an --id option to be used with 2.53+ to pass the service id (as UUID) directly to avoid the host/binary filtering. Conflicts: openstackclient/compute/v2/service.py Note(elod.illes): conflict is due to missing patch on stable/queens: I0a87e02e71ff025d30181fc17ebcd003a590f110 Change-Id: I868e0868e8eb17e7e34eef3d2d58dceedd29c2b0 Story: 2005349 Task: 30302 (cherry picked from commit 4bd53dc) (cherry picked from commit 100d34c) (cherry picked from commit fc5f297)
1 parent 3e4dd4a commit 492a6c4

3 files changed

Lines changed: 164 additions & 11 deletions

File tree

‎openstackclient/compute/v2/service.py‎

Lines changed: 57 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import logging
1919

20+
from novaclient import api_versions
2021
from osc_lib.command import command
2122
from osc_lib import exceptions
2223
from osc_lib import utils
@@ -158,6 +159,33 @@ def get_parser(self, prog_name):
158159
)
159160
return parser
160161

162+
@staticmethod
163+
def _find_service_by_host_and_binary(cs, host, binary):
164+
"""Utility method to find a compute service by host and binary
165+
166+
:param host: the name of the compute service host
167+
:param binary: the compute service binary, e.g. nova-compute
168+
:returns: novaclient.v2.services.Service dict-like object
169+
:raises: CommandError if no or multiple results were found
170+
"""
171+
services = cs.list(host=host, binary=binary)
172+
# Did we find anything?
173+
if not len(services):
174+
msg = _('Compute service for host "%(host)s" and binary '
175+
'"%(binary)s" not found.') % {
176+
'host': host, 'binary': binary}
177+
raise exceptions.CommandError(msg)
178+
# Did we find more than one result? This should not happen but let's
179+
# be safe.
180+
if len(services) > 1:
181+
# TODO(mriedem): If we have an --id option for 2.53+ then we can
182+
# say to use that option to uniquely identify the service.
183+
msg = _('Multiple compute services found for host "%(host)s" and '
184+
'binary "%(binary)s". Unable to proceed.') % {
185+
'host': host, 'binary': binary}
186+
raise exceptions.CommandError(msg)
187+
return services[0]
188+
161189
def take_action(self, parsed_args):
162190
compute_client = self.app.client_manager.compute
163191
cs = compute_client.services
@@ -168,6 +196,20 @@ def take_action(self, parsed_args):
168196
"--disable specified.")
169197
raise exceptions.CommandError(msg)
170198

199+
# Starting with microversion 2.53, there is a single
200+
# PUT /os-services/{service_id} API for updating nova-compute
201+
# services. If 2.53+ is used we need to find the nova-compute
202+
# service using the --host and --service (binary) values.
203+
requires_service_id = (
204+
compute_client.api_version >= api_versions.APIVersion('2.53'))
205+
service_id = None
206+
if requires_service_id:
207+
# TODO(mriedem): Add an --id option so users can pass the service
208+
# id (as a uuid) directly rather than make us look it up using
209+
# host/binary.
210+
service_id = SetService._find_service_by_host_and_binary(
211+
cs, parsed_args.host, parsed_args.service).id
212+
171213
result = 0
172214
enabled = None
173215
try:
@@ -178,14 +220,21 @@ def take_action(self, parsed_args):
178220

179221
if enabled is not None:
180222
if enabled:
181-
cs.enable(parsed_args.host, parsed_args.service)
223+
args = (service_id,) if requires_service_id else (
224+
parsed_args.host, parsed_args.service)
225+
cs.enable(*args)
182226
else:
183227
if parsed_args.disable_reason:
184-
cs.disable_log_reason(parsed_args.host,
185-
parsed_args.service,
186-
parsed_args.disable_reason)
228+
args = (service_id, parsed_args.disable_reason) if \
229+
requires_service_id else (
230+
parsed_args.host,
231+
parsed_args.service,
232+
parsed_args.disable_reason)
233+
cs.disable_log_reason(*args)
187234
else:
188-
cs.disable(parsed_args.host, parsed_args.service)
235+
args = (service_id,) if requires_service_id else (
236+
parsed_args.host, parsed_args.service)
237+
cs.disable(*args)
189238
except Exception:
190239
status = "enabled" if enabled else "disabled"
191240
LOG.error("Failed to set service status to %s", status)
@@ -198,8 +247,9 @@ def take_action(self, parsed_args):
198247
if parsed_args.up:
199248
force_down = False
200249
if force_down is not None:
201-
cs.force_down(parsed_args.host, parsed_args.service,
202-
force_down=force_down)
250+
args = (service_id, force_down) if requires_service_id else (
251+
parsed_args.host, parsed_args.service, force_down)
252+
cs.force_down(*args)
203253
except Exception:
204254
state = "down" if force_down else "up"
205255
LOG.error("Failed to set service state to %s", state)

‎openstackclient/tests/unit/compute/v2/test_service.py‎

Lines changed: 101 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
import mock
1717
from mock import call
1818

19+
from novaclient import api_versions
1920
from osc_lib import exceptions
21+
import six
2022

2123
from openstackclient.compute.v2 import service
2224
from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes
@@ -342,7 +344,7 @@ def test_service_set_state_up(self):
342344
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
343345
result = self.cmd.take_action(parsed_args)
344346
self.service_mock.force_down.assert_called_once_with(
345-
self.service.host, self.service.binary, force_down=False)
347+
self.service.host, self.service.binary, False)
346348
self.assertNotCalled(self.service_mock.enable)
347349
self.assertNotCalled(self.service_mock.disable)
348350
self.assertIsNone(result)
@@ -361,7 +363,7 @@ def test_service_set_state_down(self):
361363
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
362364
result = self.cmd.take_action(parsed_args)
363365
self.service_mock.force_down.assert_called_once_with(
364-
self.service.host, self.service.binary, force_down=True)
366+
self.service.host, self.service.binary, True)
365367
self.assertNotCalled(self.service_mock.enable)
366368
self.assertNotCalled(self.service_mock.disable)
367369
self.assertIsNone(result)
@@ -384,7 +386,7 @@ def test_service_set_enable_and_state_down(self):
384386
self.service_mock.enable.assert_called_once_with(
385387
self.service.host, self.service.binary)
386388
self.service_mock.force_down.assert_called_once_with(
387-
self.service.host, self.service.binary, force_down=True)
389+
self.service.host, self.service.binary, True)
388390
self.assertIsNone(result)
389391

390392
def test_service_set_enable_and_state_down_with_exception(self):
@@ -407,4 +409,99 @@ def test_service_set_enable_and_state_down_with_exception(self):
407409
self.assertRaises(exceptions.CommandError,
408410
self.cmd.take_action, parsed_args)
409411
self.service_mock.force_down.assert_called_once_with(
410-
self.service.host, self.service.binary, force_down=True)
412+
self.service.host, self.service.binary, True)
413+
414+
def test_service_set_2_53_disable_down(self):
415+
# Tests disabling and forcing down a compute service with microversion
416+
# 2.53 which requires looking up the service by host and binary.
417+
arglist = [
418+
'--disable',
419+
'--down',
420+
self.service.host,
421+
self.service.binary,
422+
]
423+
verifylist = [
424+
('disable', True),
425+
('down', True),
426+
('host', self.service.host),
427+
('service', self.service.binary),
428+
]
429+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
430+
self.app.client_manager.compute.api_version = api_versions.APIVersion(
431+
'2.53')
432+
service_id = '339478d0-0b95-4a94-be63-d5be05dfeb1c'
433+
self.service_mock.list.return_value = [mock.Mock(id=service_id)]
434+
result = self.cmd.take_action(parsed_args)
435+
self.service_mock.disable.assert_called_once_with(service_id)
436+
self.service_mock.force_down.assert_called_once_with(service_id, True)
437+
self.assertIsNone(result)
438+
439+
def test_service_set_2_53_disable_reason(self):
440+
# Tests disabling with reason a compute service with microversion
441+
# 2.53 which requires looking up the service by host and binary.
442+
reason = 'earthquake'
443+
arglist = [
444+
'--disable',
445+
'--disable-reason', reason,
446+
self.service.host,
447+
self.service.binary,
448+
]
449+
verifylist = [
450+
('disable', True),
451+
('disable_reason', reason),
452+
('host', self.service.host),
453+
('service', self.service.binary),
454+
]
455+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
456+
self.app.client_manager.compute.api_version = api_versions.APIVersion(
457+
'2.53')
458+
service_id = '339478d0-0b95-4a94-be63-d5be05dfeb1c'
459+
self.service_mock.list.return_value = [mock.Mock(id=service_id)]
460+
result = self.cmd.take_action(parsed_args)
461+
self.service_mock.disable_log_reason.assert_called_once_with(
462+
service_id, reason)
463+
self.assertIsNone(result)
464+
465+
def test_service_set_2_53_enable_up(self):
466+
# Tests enabling and bringing up a compute service with microversion
467+
# 2.53 which requires looking up the service by host and binary.
468+
arglist = [
469+
'--enable',
470+
'--up',
471+
self.service.host,
472+
self.service.binary,
473+
]
474+
verifylist = [
475+
('enable', True),
476+
('up', True),
477+
('host', self.service.host),
478+
('service', self.service.binary),
479+
]
480+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
481+
self.app.client_manager.compute.api_version = api_versions.APIVersion(
482+
'2.53')
483+
service_id = '339478d0-0b95-4a94-be63-d5be05dfeb1c'
484+
self.service_mock.list.return_value = [mock.Mock(id=service_id)]
485+
result = self.cmd.take_action(parsed_args)
486+
self.service_mock.enable.assert_called_once_with(service_id)
487+
self.service_mock.force_down.assert_called_once_with(service_id, False)
488+
self.assertIsNone(result)
489+
490+
def test_service_set_find_service_by_host_and_binary_no_results(self):
491+
# Tests that no compute services are found by host and binary.
492+
self.service_mock.list.return_value = []
493+
ex = self.assertRaises(exceptions.CommandError,
494+
self.cmd._find_service_by_host_and_binary,
495+
self.service_mock, 'fake-host', 'nova-compute')
496+
self.assertIn('Compute service for host "fake-host" and binary '
497+
'"nova-compute" not found.', six.text_type(ex))
498+
499+
def test_service_set_find_service_by_host_and_binary_many_results(self):
500+
# Tests that more than one compute service is found by host and binary.
501+
self.service_mock.list.return_value = [mock.Mock(), mock.Mock()]
502+
ex = self.assertRaises(exceptions.CommandError,
503+
self.cmd._find_service_by_host_and_binary,
504+
self.service_mock, 'fake-host', 'nova-compute')
505+
self.assertIn('Multiple compute services found for host "fake-host" '
506+
'and binary "nova-compute". Unable to proceed.',
507+
six.text_type(ex))

‎releasenotes/notes/story-2005349-compute-service-set-2.53-3d2db875154e633a.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
fixes:
3+
- |
4+
The ``compute service set`` command now properly handles
5+
``--os-compute-api-version`` 2.53 and greater.
6+
[Story `2005349 <https://storyboard.openstack.org/#!/story/2005349>`_]

0 commit comments

Comments
 (0)