diff --git a/ironic/conf/default.py b/ironic/conf/default.py index b43e3bbe05..9b2a3c407c 100644 --- a/ironic/conf/default.py +++ b/ironic/conf/default.py @@ -167,6 +167,9 @@ exc_log_opts = [ 'an exception message (a programming error). If True, ' 'raise an exception; if False, use the unformatted ' 'message.')), + cfg.IntOpt('log_in_db_max_size', default=4096, + help=_('Max number of characters of any node ' + 'last_error/maintenance_reason pushed to database.')) ] hash_opts = [ diff --git a/ironic/objects/node.py b/ironic/objects/node.py index dfd56e5896..9c4b49c2a4 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -12,11 +12,13 @@ # 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 +from oslo_log import log from oslo_utils import strutils from oslo_utils import uuidutils from oslo_utils import versionutils from oslo_versionedobjects import base as object_base +import six from ironic.common import exception from ironic.common.i18n import _ @@ -29,6 +31,9 @@ from ironic.objects import notification REQUIRED_INT_PROPERTIES = ['local_gb', 'cpus', 'memory_mb'] +CONF = cfg.CONF +LOG = log.getLogger(__name__) + @base.IronicObjectRegistry.register class Node(base.IronicObject, object_base.VersionedObjectDictCompat): @@ -416,6 +421,16 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): object, e.g.: Node(context) :raises: InvalidParameterValue if some property values are invalid. """ + + for attr_name in ('last_error', 'maintenance_reason'): + attr_value = getattr(self, attr_name, '') + if (attr_value and isinstance(attr_value, six.string_types) and + len(attr_value) > CONF.log_in_db_max_size): + LOG.info('Truncating too long %s to %s characters for node %s', + attr_name, CONF.log_in_db_max_size, self.uuid) + setattr(self, attr_name, + attr_value[0:CONF.log_in_db_max_size]) + updates = self.do_version_changes_for_db() self._validate_property_values(updates.get('properties')) if 'driver' in updates and 'driver_internal_info' not in updates: diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index f8334016f1..1f0988c2a2 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -23,6 +23,7 @@ from testtools import matchers from ironic.common import context from ironic.common import exception from ironic import objects +from ironic.objects import node as node_objects from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils from ironic.tests.unit.objects import utils as obj_utils @@ -171,6 +172,64 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): self.assertEqual(test_time, res_updated_at) self.assertEqual({}, n.driver_internal_info) + @mock.patch.object(node_objects, 'LOG', autospec=True) + def test_save_truncated(self, log_mock): + uuid = self.fake_node['uuid'] + test_time = datetime.datetime(2000, 1, 1, 0, 0) + with mock.patch.object(self.dbapi, 'get_node_by_uuid', + autospec=True) as mock_get_node: + mock_get_node.return_value = self.fake_node + with mock.patch.object(self.dbapi, 'update_node', + autospec=True) as mock_update_node: + mock_update_node.return_value = db_utils.get_test_node( + properties={'fake': 'property'}, driver='fake-driver', + driver_internal_info={}, updated_at=test_time) + n = objects.Node.get(self.context, uuid) + self.assertEqual({'private_state': 'secret value'}, + n.driver_internal_info) + n.properties = {'fake': 'property'} + n.driver = 'fake-driver' + last_error = 'BOOM' * 2000 + maintenance_reason = last_error + n.last_error = last_error + n.maintenance_reason = maintenance_reason + + n.save() + + self.assertEqual([ + mock.call.info( + 'Truncating too long %s to %s characters for node %s', + 'last_error', + node_objects.CONF.log_in_db_max_size, + uuid), + mock.call.info( + 'Truncating too long %s to %s characters for node %s', + 'maintenance_reason', + node_objects.CONF.log_in_db_max_size, + uuid)], + log_mock.mock_calls) + + mock_get_node.assert_called_once_with(uuid) + mock_update_node.assert_called_once_with( + uuid, + { + 'properties': {'fake': 'property'}, + 'driver': 'fake-driver', + 'driver_internal_info': {}, + 'version': objects.Node.VERSION, + 'maintenance_reason': + maintenance_reason[ + 0:node_objects.CONF.log_in_db_max_size], + 'last_error': + last_error[ + 0:node_objects.CONF.log_in_db_max_size] + } + ) + self.assertEqual(self.context, n._context) + res_updated_at = (n.updated_at).replace(tzinfo=None) + self.assertEqual(test_time, res_updated_at) + self.assertEqual({}, n.driver_internal_info) + def test_save_updated_at_field(self): uuid = self.fake_node['uuid'] extra = {"test": 123} diff --git a/releasenotes/notes/bug-2005377-5c63357681a465ec.yaml b/releasenotes/notes/bug-2005377-5c63357681a465ec.yaml new file mode 100644 index 0000000000..d610cfc379 --- /dev/null +++ b/releasenotes/notes/bug-2005377-5c63357681a465ec.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Fixes node last_error and maintenance_reason overflow with too long error + messages, preventing the object from being correctly committed to database. + The maximum message length can be customized through a new configuration + parameter, ``[DEFAULT]/log_in_db_max_size`` (default, 4096 characters).