openstack
/
os-brick
Code Issues Proposed changes

Support independent file lock path 

OS-Brick uses file locks to ensure single access to critital sections,
and uses the oslo concurrency "lock_path" configuration option to
determine where to create these locks.

This is fine when each host has a single service using os-brick, which
is the case of Compute nodes and Controller nodes where Glance is not
using Cinder as its backend.

The problem happens when we have an HCI deployment where Cinder and Nova
are collocated on the same host or when Glance uses Cinder as its
backend and is running on the same host.  In those scenarios os-brick
will create file locks in different paths for each service, which is not
the intended behavior.

A possible solutions is to set the "lock_path" of all the services to
the same location, which is not great, not only because we'll have all
nova, cinder, glance, and os-brick locks mixed in a single location
(service prefixes help a bit here), but also because then Cinder will
have permissions on the Nova and Glance locks, which doesn't seem right.

This patch introduces a new mechanism in os-brick to have its own
"lock_path" configuration option in the "os_brick" group.  It defaults
to the current behavior if not explicitly defined, so it will use olso
concurrency's "lock_path".

To do this the patch introduces the oslo.config dependency and adds a
new "setup" method that sets the default of the os_brick "lock_path".

This new "setup" method is required because the order in which
configuration options for the different namespaces are imported cannot
be guaranteed, so the setup must be called *after* the service has
already imported all (or at least the ones os-brick cares about)
configuration option namespaces.

In other words, when os-brick files are loaded the value for oslo
concurrency's "lock_path" is not yet known, so it cannot be used as a
default, and the oslo_config substitution feature does not support
values from other namespaces, so we cannot default the "lock_path" from
the os_brick namespace to the value in "oslo_concurrency".

Since the value that is going to be used as the "lock_path" is not known
until the "setup" method is called, we cannot use the standard
"synchronized" method from "oslo_concurrency.lock_utils" and we need to
use our own.

In the current os-brick code, each connector that requires a lock is
importing and creating its own "synchronized" instance, but this patch
changes this behavior and creates a single instance, just like Cinder
does.

This feature requires changes in multiple projects -os-brick, cinder,
nova, glance, glance_store, devstack- to be fully supported, but this is
the base of all this effort.

Change-Id: Ic52338278eb5bb3d90ce582fe6b23f37eb5568c4
changes/24/849324/6
Gorka Eguileor 2022-07-11 11:37:38 +02:00 committed by Rajat Dhasmana
parent c5076c37cb
commit b72c034885
14 changed files with 221 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
doc/source/user/tutorial.rst
View File

@ -15,6 +15,42 @@ run without raising an exception:
>>> import os_brick
Configuration
-------------
There are some os-brick connectors that use file locks to prevent concurrent
access to critical sections of the code.
These file locks use the ``oslo.concurrency`` ``lock_utils`` module and require
the ``lock_path`` to be configured with the path where locks should be created.
os-brick can use a specific directory just for its locks or use the same
directory as the service using os-brick.
The os-brick specific configuration option is ``[os_brick]/lock_path``, and if
left undefined it will use the value from ``[oslo_concurrency]/lock_path``.
Setup
-----
Once os_brick has been loaded it needs to be initialized, which is done by
calling the ``os_brick.setup`` method with the ``oslo.conf`` configuration.
It is important that the call to ``setup`` method happens **after** oslo.config
has been properly initialized.
.. code-block:: python
from oslo_config import cfg
from cinder import version
CONF = cfg.CONF
def main():
CONF(sys.argv[1:], project='cinder',
version=version.version_string())
os_brick.setup(CONF)
Fetch all of the initiator information from the host
----------------------------------------------------
@ -25,6 +61,9 @@ a volume to this host.
from os_brick.initiator import connector
os_brick.setup(CONF)
# what helper do you want to use to get root access?
root_helper = "sudo"
# The ip address of the host you are running on

33
os_brick/__init__.py
View File

@ -0,0 +1,33 @@
# Copyright (c) 2022, Red Hat, Inc.
# All Rights Reserved.
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
from oslo_log import log as logging
from os_brick import opts
LOG = logging.getLogger(__name__)
def setup(conf, **kwargs):
"""Setup the os-brick library.
Service configuration options must have been initialized before this call
because oslo's lock_path doesn't have a value before that.
Having kwargs allows us to receive parameters in the future.
"""
if kwargs:
LOG.warning('Ignoring arguments %s', kwargs.keys())
opts.set_defaults(conf)

3
os_brick/initiator/connector.py
View File

@ -24,7 +24,6 @@ import platform
import socket
import sys
from oslo_concurrency import lockutils
from oslo_log import log as logging
from oslo_utils import importutils
@ -35,8 +34,6 @@ from os_brick import utils
LOG = logging.getLogger(__name__)
synchronized = lockutils.synchronized_with_prefix('os-brick-')
# List of connectors to call when getting
# the connector properties for a host
windows_connector_list = [

63
os_brick/initiator/connectors/base.py
View File

@ -13,11 +13,16 @@
# under the License.
import functools
import glob
import os
from oslo_concurrency import lockutils
from oslo_concurrency import processutils as putils
from oslo_config import cfg
from oslo_log import log as logging
from oslo_utils import reflection
from oslo_utils import timeutils
from os_brick import exception
from os_brick import initiator
@ -26,6 +31,64 @@ from os_brick.initiator import initiator_connector
from os_brick.initiator import linuxscsi
LOG = logging.getLogger(__name__)
CONF = cfg.CONF
def synchronized(name, lock_file_prefix='os-brick-', external=False,
lock_path=None, semaphores=None, delay=0.01, fair=False,
blocking=True):
"""os-brick synchronization decorator
Like the one in lock_utils but defaulting the prefix to os-brick- and using
our own lock_path.
Cannot use lock_utils one because when using the default we don't know the
value until setup has been called, which can be after the code using the
decorator has been loaded.
"""
def wrap(f):
@functools.wraps(f)
def inner(*args, **kwargs):
t1 = timeutils.now()
t2 = None
gotten = True
lpath = lock_path or CONF.os_brick.lock_path
# TODO: (AA Release) Remove this failsafe
if not lpath and CONF.oslo_concurrency.lock_path:
LOG.warning("Service needs to call os_brick.setup() before "
"connecting volumes, if it doesn't it will break "
"on the next release")
lpath = CONF.oslo_concurrency.lock_path
f_name = reflection.get_callable_name(f)
try:
LOG.debug('Acquiring lock "%s" by "%s"', name, f_name)
with lockutils.lock(name, lock_file_prefix, external, lpath,
do_log=False, semaphores=semaphores,
delay=delay, fair=fair, blocking=blocking):
t2 = timeutils.now()
LOG.debug('Lock "%(name)s" acquired by "%(function)s" :: '
'waited %(wait_secs)0.3fs',
{'name': name,
'function': f_name,
'wait_secs': (t2 - t1)})
return f(*args, **kwargs)
except lockutils.AcquireLockFailedException:
gotten = False
finally:
t3 = timeutils.now()
if t2 is None:
held_secs = "N/A"
else:
held_secs = "%0.3fs" % (t3 - t2)
LOG.debug('Lock "%(name)s" "%(gotten)s" by "%(function)s" ::'
' held %(held_secs)s',
{'name': name,
'gotten': 'released' if gotten else 'unacquired',
'function': f_name,
'held_secs': held_secs})
return inner
return wrap
class BaseLinuxConnector(initiator_connector.InitiatorConnector):

9
os_brick/initiator/connectors/fibre_channel.py
View File

@ -14,7 +14,6 @@
import os
from oslo_concurrency import lockutils
from oslo_log import log as logging
from oslo_service import loopingcall
@ -25,8 +24,6 @@ from os_brick.initiator.connectors import base
from os_brick.initiator import linuxfc
from os_brick import utils
synchronized = lockutils.synchronized_with_prefix('os-brick-')
LOG = logging.getLogger(__name__)
@ -168,7 +165,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
return volume_paths
@utils.trace
@synchronized('extend_volume', external=True)
@base.synchronized('extend_volume', external=True)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Update the local kernel's size information.
@ -191,7 +188,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties):
"""Attach the volume to instance_name.
@ -316,7 +313,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
return raw_devices
@utils.trace
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):

6
os_brick/initiator/connectors/huawei.py
View File

@ -14,7 +14,6 @@
import os
from oslo_concurrency import lockutils
from oslo_log import log as logging
from os_brick import exception
@ -23,7 +22,6 @@ from os_brick.initiator.connectors import base
from os_brick import utils
LOG = logging.getLogger(__name__)
synchronized = lockutils.synchronized_with_prefix('os-brick-')
class HuaweiStorHyperConnector(base.BaseLinuxConnector):
@ -85,7 +83,7 @@ class HuaweiStorHyperConnector(base.BaseLinuxConnector):
return out['dev_addr']
@utils.trace
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties):
"""Connect to a volume.
@ -116,7 +114,7 @@ class HuaweiStorHyperConnector(base.BaseLinuxConnector):
return device_info
@utils.trace
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Disconnect a volume from the local host.

11
os_brick/initiator/connectors/iscsi.py
View File

@ -21,7 +21,6 @@ import re
import time
from typing import List, Tuple # noqa: H301
from oslo_concurrency import lockutils
from oslo_concurrency import processutils as putils
from oslo_log import log as logging
from oslo_utils import excutils
@ -36,8 +35,6 @@ from os_brick.initiator.connectors import base_iscsi
from os_brick.initiator import utils as initiator_utils
from os_brick import utils
synchronized = lockutils.synchronized_with_prefix('os-brick-')
LOG = logging.getLogger(__name__)
@ -474,7 +471,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
root_helper=self._root_helper)
@utils.trace
@synchronized('extend_volume', external=True)
@base.synchronized('extend_volume', external=True)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties: dict):
"""Update the local kernel's size information.
@ -499,7 +496,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties: dict):
"""Attach the volume to instance_name.
@ -831,7 +828,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
return device_map
@utils.trace
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
@ -1017,7 +1014,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
target_iqn = connection_properties['target_iqn']
lock_name = f'connect_to_iscsi_portal-{portal}-{target_iqn}'
method = synchronized(
method = base.synchronized(
lock_name, external=True)(self._connect_to_iscsi_portal_unsafe)
return method(connection_properties)

8
os_brick/initiator/connectors/lightos.py
View File

@ -22,7 +22,6 @@ import tempfile
import time
import traceback
from oslo_concurrency import lockutils
from oslo_concurrency import processutils as putils
from oslo_log import log as logging
@ -37,7 +36,6 @@ DEVICE_SCAN_ATTEMPTS_DEFAULT = 5
DISCOVERY_CLIENT_PORT = 6060
LOG = logging.getLogger(__name__)
synchronized = lockutils.synchronized_with_prefix('os-brick-')
nvmec_pattern = ".*nvme[0-9]+[cp][0-9]+.*"
nvmec_match = re.compile(nvmec_pattern)
@ -257,7 +255,7 @@ class LightOSConnector(base.BaseLinuxConnector):
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('volume_op')
@base.synchronized('volume_op')
def connect_volume(self, connection_properties):
"""Discover and attach the volume.
@ -292,7 +290,7 @@ class LightOSConnector(base.BaseLinuxConnector):
return device_info
@utils.trace
@synchronized('volume_op')
@base.synchronized('volume_op')
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
@ -335,7 +333,7 @@ class LightOSConnector(base.BaseLinuxConnector):
raise exc
@utils.trace
@synchronized('volume_op')
@base.synchronized('volume_op')
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
uuid = connection_properties['uuid']

9
os_brick/initiator/connectors/nvmeof.py
View File

@ -17,7 +17,6 @@ import os.path
import re
import time
from oslo_concurrency import lockutils
from oslo_concurrency import processutils as putils
from oslo_log import log as logging
@ -35,8 +34,6 @@ DEV_SEARCH_PATH = '/dev/'
DEVICE_SCAN_ATTEMPTS_DEFAULT = 5
synchronized = lockutils.synchronized_with_prefix('os-brick-')
LOG = logging.getLogger(__name__)
@ -330,7 +327,7 @@ class NVMeOFConnector(base.BaseLinuxConnector):
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties):
"""Discover and attach the volume.
@ -381,7 +378,7 @@ class NVMeOFConnector(base.BaseLinuxConnector):
return device_info
@utils.trace
@synchronized('connect_volume', external=True)
@base.synchronized('connect_volume', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
@ -426,7 +423,7 @@ class NVMeOFConnector(base.BaseLinuxConnector):
raise
@utils.trace
@synchronized('extend_volume', external=True)
@base.synchronized('extend_volume', external=True)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Update the local kernel's size information.

6
os_brick/initiator/connectors/scaleio.py
View File

@ -16,7 +16,6 @@ import json
import os
import urllib
from oslo_concurrency import lockutils
from oslo_log import log as logging
import requests
@ -30,7 +29,6 @@ from os_brick import utils
LOG = logging.getLogger(__name__)
DEVICE_SCAN_ATTEMPTS_DEFAULT = 3
CONNECTOR_CONF_PATH = '/opt/emc/scaleio/openstack/connector.conf'
synchronized = lockutils.synchronized_with_prefix('os-brick-')
def io(_type, nr):
@ -349,7 +347,7 @@ class ScaleIOConnector(base.BaseLinuxConnector):
@utils.trace
@utils.connect_volume_prepare_result
@lockutils.synchronized('scaleio', 'scaleio-', external=True)
@base.synchronized('scaleio', 'scaleio-', external=True)
def connect_volume(self, connection_properties):
"""Connect the volume.
@ -463,7 +461,7 @@ class ScaleIOConnector(base.BaseLinuxConnector):
return device_info
@utils.trace
@lockutils.synchronized('scaleio', 'scaleio-', external=True)
@base.synchronized('scaleio', 'scaleio-', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):

45
os_brick/opts.py Normal file
View File

@ -0,0 +1,45 @@
# Copyright (c) 2022, Red Hat, Inc.
# All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
from oslo_config import cfg
_opts = [
cfg.StrOpt('lock_path',
default=None, # Set by set_defaults method below on setup
help='Directory to use for os-brick lock files. Defaults to '
'oslo_concurrency.lock_path which is a sensible default '
'for compute nodes, but not for HCI deployments or '
'controllers where Glance uses Cinder as a backend, as '
'locks should use the same directory.'),
]
cfg.CONF.register_opts(_opts, group='os_brick')
def list_opts():
"""oslo.config.opts entrypoint for sample config generation."""
return [('os_brick', _opts)]
def set_defaults(conf=cfg.CONF):
"""Set default values that depend on other libraries.
Service configuration options must have been initialized before this call
because oslo's lock_path doesn't have a value before that.
Called from both os_brick setup and from the oslo.config.opts entrypoint
for sample config generation.
"""
conf.set_default('lock_path', conf.oslo_concurrency.lock_path, 'os_brick')

17
releasenotes/notes/lock_path-c1c58a253391b41c.yaml Normal file
View File

@ -0,0 +1,17 @@
---
features:
- |
Specific location for os-brick file locks using the ``lock_path``
configuration option in the ``os_brick`` configuration group. Previously,
os-brick used the consuming service's lock_path for its locks, but there
are some deployment configurations (for example, Nova and Cinder collocated
on the same host) where this would result in anomalous behavior. Default
is to use the consuming service's lock_path.
This change requires a consuming service to call the ``os_brick.setup``
method after service configuration options have been called.
upgrade:
- |
To use the os-brick specific file lock location introduced in this release,
an external service using the library must call the ``os_brick.setup``
method.

1
requirements.txt
View File

@ -5,6 +5,7 @@
pbr>=5.8.0 # Apache-2.0
eventlet>=0.30.1,!=0.32.0 # MIT
oslo.concurrency>=4.5.0 # Apache-2.0
oslo.config>=8.1.0 # Apache-2.0
oslo.context>=3.4.0 # Apache-2.0
oslo.log>=4.6.1 # Apache-2.0
oslo.i18n>=5.1.0 # Apache-2.0

6
setup.cfg
View File

@ -26,6 +26,12 @@ packages =
data_files =
etc/ = etc/*
[entry_points]
oslo.config.opts =
os_brick = os_brick.opts:list_opts
oslo.config.opts.defaults =
os_brick = os_brick.opts:set_defaults
[egg_info]
tag_build =
tag_date = 0