openstack/puppet-cinder
Code Issues Proposed changes

Fix location where backend_host is configured

Browse Source 
When the backend_host option is specified, configure it in each
cinder-volume backend driver's section and not in the DEFAULT section.
Backend drivers only look for the option in their backend section, so
setting in the DEFAULT section had no effect.

Clarify the fact that the DEFAULT host option is *not* deprecated, and
is only deprecated in backend sections (where the backed_host should be
used).

Partial-Bug: #1753596
Change-Id: I11a55571f4bed630967242c797e08e11c47eab11
This commit is contained in:
Alan Bishop
2018-03-06 08:46:19 -05:00
parent 0cec98ddab
commit bec1827866
6 changed files with 75 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
manifests/backend/rbd.pp
View File

@@ -120,13 +120,16 @@ define cinder::backend::rbd (
}
}
if $backend_host {
cinder_config {
"${name}/backend_host": value => $backend_host;
}
} else {
cinder_config {
"${name}/backend_host": value => "rbd:${rbd_pool}";
# Avoid colliding with code in backends.pp
unless defined(Cinder_config["${name}/backend_host"]) {
if $backend_host {
cinder_config {
"${name}/backend_host": value => $backend_host;
}
} else {
cinder_config {
"${name}/backend_host": value => "rbd:${rbd_pool}";
}
}
}

15
manifests/backends.pp
View File

@@ -10,9 +10,14 @@
# Example: ['volume1', 'volume2', 'sata3']
# Defaults to undef
#
# [*backend_host*]
# (optional) Backend override of host value.
# Defaults to hiera('cinder::backend_host', undef)
#
# Author: Andrew Woodward <awoodward@mirantis.com>
class cinder::backends (
$enabled_backends = undef,
$backend_host = hiera('cinder::backend_host', undef)
) {
include ::cinder::deps
@@ -26,5 +31,15 @@ set up backends. No volume service(s) started successfully otherwise.")
cinder_config {
'DEFAULT/enabled_backends': value => join($enabled_backends, ',');
}
if $backend_host {
$enabled_backends.each |$backend| {
# Avoid colliding with code in backend/rbd.pp
unless defined(Cinder_config["${backend}/backend_host"]) {
cinder_config {
"${backend}/backend_host": value => $backend_host;
}
}
}
}
}
}

21
manifests/init.pp
View File

@@ -243,6 +243,11 @@
# 'cinder::backend::rdb::volume_tmp_dir' parameter.
# Defaults to $::os_service_default
#
# [*host*]
# (optional) Name of this node. This can be an opaque identifier. It is
# not necessarily a host name, FQDN, or IP address.
# Defaults to $::os_service_default.
#
# [*purge_config*]
# (optional) Whether to set only the specified config options
# in the cinder config.
@@ -298,11 +303,6 @@
# (Optional) DEPRECATED. Virtual_host to use.
# Defaults to $::os_service_default
#
# [*host*]
# (optional) DEPRECATED. Name of this node. This can be an opaque identifier. It is
# not necessarily a host name, FQDN, or IP address.
# Defaults to $::os_service_default.
#
# [*rpc_backend*]
# (Optional) DEPRECATED. Use these options to configure the RabbitMQ message system.
# Defaults to undef
@@ -361,6 +361,7 @@ class cinder (
$enable_v3_api = true,
$lock_path = $::cinder::params::lock_path,
$image_conversion_dir = $::os_service_default,
$host = $::os_service_default,
$purge_config = false,
$backend_host = $::os_service_default,
# DEPRECATED PARAMETERS
@@ -374,7 +375,6 @@ class cinder (
$rabbit_port = $::os_service_default,
$rabbit_userid = $::os_service_default,
$rabbit_virtual_host = $::os_service_default,
$host = $::os_service_default,
$rpc_backend = undef,
) inherits cinder::params {
@@ -382,10 +382,6 @@ class cinder (
include ::cinder::db
include ::cinder::logging
if !is_service_default($host) {
warning('host is deprecated, has no effect and will be removed in a future release, use backend_host instead')
}
if !is_service_default($rabbit_host) or
!is_service_default($rabbit_hosts) or
!is_service_default($rabbit_password) or
@@ -470,7 +466,10 @@ instead.")
'DEFAULT/allow_availability_zone_fallback': value => $allow_availability_zone_fallback;
'DEFAULT/image_conversion_dir': value => $image_conversion_dir;
'DEFAULT/host': value => $host;
'DEFAULT/backend_host': value => $backend_host;
# NOTE(abishop): $backend_host is not written here because it is not a valid
# DEFAULT option. It is only recognized in the backend sections. Instead,
# for backward compatibility, backends.pp references this parameter.
}
# V3 APIs

14
releasenotes/notes/fix-backend_host-usage-316f4fb0f2ef6dae.yaml Normal file
View File

@@ -0,0 +1,14 @@
---
fixes:
- |
Fixes `bug 1753596
<https://bugs.launchpad.net/tripleo/+bug/1753596>`__ so the backend_host
option is set in each cinder-volume backend driver's section, and not in
the DEFAULT section where the option isn't relevant.
other:
- |
A previous deprecation notice stated the host option was deprecated in
favor of the backend_host option. This was misleading because the note
failed to clarify this only applied to options in the cinder-volume
backend driver sections. In the DEFAULT section, the host option is not
deprecated, and the backend_host option isn't relevant.

23
spec/classes/cinder_backends_spec.rb
View File

@@ -45,6 +45,29 @@ describe 'cinder::backends' do
it 'configures cinder.conf with default params' do
is_expected.to contain_cinder_config('DEFAULT/enabled_backends').with_value(p[:enabled_backends].join(','))
is_expected.to_not contain_cinder_config('lowcost/backend_host')
is_expected.to_not contain_cinder_config('regular/backend_host')
is_expected.to_not contain_cinder_config('premium/backend_host')
end
end
context 'configure cinder with backend_host' do
before :each do
params.merge!(
:enabled_backends => ['lowcost', 'regular', 'premium'],
:backend_host => 'somehost',
)
end
let(:pre_condition) do
# Verify there are no collisions with any previously defined value.
"cinder_config { 'regular/backend_host': value => 'anotherhost' }"
end
it 'configures backend_host in each backend' do
is_expected.to contain_cinder_config('lowcost/backend_host').with_value('somehost')
is_expected.to contain_cinder_config('regular/backend_host').with_value('anotherhost')
is_expected.to contain_cinder_config('premium/backend_host').with_value('somehost')
end
end
end

14
spec/classes/cinder_init_spec.rb
View File

@@ -53,8 +53,10 @@ describe 'cinder' do
is_expected.to contain_cinder_config('DEFAULT/default_availability_zone').with(:value => 'nova')
is_expected.to contain_cinder_config('DEFAULT/allow_availability_zone_fallback').with(:value => '<SERVICE DEFAULT>')
is_expected.to contain_cinder_config('DEFAULT/api_paste_config').with(:value => '/etc/cinder/api-paste.ini')
is_expected.to contain_cinder_config('DEFAULT/backend_host').with(:value => '<SERVICE DEFAULT>')
is_expected.to contain_cinder_config('oslo_concurrency/lock_path').with(:value => '/var/lock/cinder')
# backend_host should not be written to DEFAULT section
is_expected.to_not contain_cinder_config('DEFAULT/backend_host')
end
end
@@ -288,14 +290,4 @@ describe 'cinder' do
it { is_expected.to contain_cinder_config('DEFAULT/transport_url').with_value('rabbit://rabbit_user:password@localhost:5673') }
end
describe 'with backend_host' do
let :params do
req_params.merge({
:backend_host => 'cinder_backend',
})
end
it { is_expected.to contain_cinder_config('DEFAULT/backend_host').with_value('cinder_backend') }
end
end