[OVN] Fix rate and burst for stateless security groups
Right now, as per kernel limitation, the burst limit is not correctly
enforcing the rate and burst when using the ovn "log-related" option and
stateless security groups. We log exactly double the burst. Creating a
new meter that limits the rate and burst to half of the expected ones is
a workaround that solves the issue.
Closes-bug: #2032929
Conflicts:
- neutron/services/logapi/drivers/ovn/driver.py
- neutron/tests/functional/services/logapi/drivers/ovn/test_driver.py
Signed-off-by: Elvira García <egarciar@redhat.com>
Change-Id: Ib0047d38c58bcebb23c8887e7934987ff8c8a432
(cherry picked from commit a3a113aedb
)
This commit is contained in:
parent
0364f18848
commit
3152bc14f8
@ -2741,7 +2741,8 @@ class OVNClient(object):
|
|||||||
txn.add(self._nb_idl.dns_remove_record(
|
txn.add(self._nb_idl.dns_remove_record(
|
||||||
ls_dns_record.uuid, ptr_record, if_exists=True))
|
ls_dns_record.uuid, ptr_record, if_exists=True))
|
||||||
|
|
||||||
def create_ovn_fair_meter(self, meter_name, from_reload=False, txn=None):
|
def _create_ovn_fair_meter(self, meter_name, from_reload=False, txn=None,
|
||||||
|
stateless=False):
|
||||||
"""Create row in Meter table with fair attribute set to True.
|
"""Create row in Meter table with fair attribute set to True.
|
||||||
|
|
||||||
Create a row in OVN's NB Meter table based on well-known name. This
|
Create a row in OVN's NB Meter table based on well-known name. This
|
||||||
@ -2756,11 +2757,26 @@ class OVNClient(object):
|
|||||||
"""
|
"""
|
||||||
meter = self._nb_idl.db_find_rows(
|
meter = self._nb_idl.db_find_rows(
|
||||||
"Meter", ("name", "=", meter_name)).execute(check_error=True)
|
"Meter", ("name", "=", meter_name)).execute(check_error=True)
|
||||||
# The meter is created when a log object is created, not by default.
|
# The meters are created when a log object is created, not by default.
|
||||||
# This condition avoids creating the meter if it wasn't there already
|
# This condition avoids creating the meter if it wasn't there already.
|
||||||
commands = []
|
commands = []
|
||||||
if from_reload and not meter:
|
if from_reload and not meter:
|
||||||
return
|
return
|
||||||
|
|
||||||
|
burst_limit = cfg.CONF.network_log.burst_limit
|
||||||
|
rate_limit = cfg.CONF.network_log.rate_limit
|
||||||
|
if stateless:
|
||||||
|
meter_name = meter_name + "_stateless"
|
||||||
|
burst_limit = int(burst_limit / 2)
|
||||||
|
rate_limit = int(rate_limit / 2)
|
||||||
|
# The stateless meter is only created once the stateful meter was
|
||||||
|
# successfully created.
|
||||||
|
# The treatment of limits is not equal for stateful and stateless
|
||||||
|
# traffic at a kernel level according to:
|
||||||
|
# https://bugzilla.redhat.com/show_bug.cgi?id=2212952
|
||||||
|
# The stateless meter is created to adjust this issue.
|
||||||
|
meter = self._nb_idl.db_find_rows(
|
||||||
|
"Meter", ("name", "=", meter_name)).execute(check_error=True)
|
||||||
if meter:
|
if meter:
|
||||||
meter = meter[0]
|
meter = meter[0]
|
||||||
meter_band = self._nb_idl.lookup("Meter_Band",
|
meter_band = self._nb_idl.lookup("Meter_Band",
|
||||||
@ -2768,9 +2784,8 @@ class OVNClient(object):
|
|||||||
if meter_band:
|
if meter_band:
|
||||||
if all((meter.unit == "pktps",
|
if all((meter.unit == "pktps",
|
||||||
meter.fair[0],
|
meter.fair[0],
|
||||||
meter_band.rate == cfg.CONF.network_log.rate_limit,
|
meter_band.rate == rate_limit,
|
||||||
meter_band.burst_size ==
|
meter_band.burst_size == burst_limit)):
|
||||||
cfg.CONF.network_log.burst_limit)):
|
|
||||||
# Meter (and its meter-band) unchanged: noop.
|
# Meter (and its meter-band) unchanged: noop.
|
||||||
return
|
return
|
||||||
# Re-create meter (and its meter-band) with the new attributes.
|
# Re-create meter (and its meter-band) with the new attributes.
|
||||||
@ -2784,10 +2799,15 @@ class OVNClient(object):
|
|||||||
commands.append(self._nb_idl.meter_add(
|
commands.append(self._nb_idl.meter_add(
|
||||||
name=meter_name,
|
name=meter_name,
|
||||||
unit="pktps",
|
unit="pktps",
|
||||||
rate=cfg.CONF.network_log.rate_limit,
|
rate=rate_limit,
|
||||||
fair=True,
|
fair=True,
|
||||||
burst_size=cfg.CONF.network_log.burst_limit,
|
burst_size=burst_limit,
|
||||||
may_exist=False,
|
may_exist=False,
|
||||||
external_ids={ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
|
external_ids={ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
|
||||||
log_const.LOGGING_PLUGIN}))
|
log_const.LOGGING_PLUGIN}))
|
||||||
self._transaction(commands, txn=txn)
|
self._transaction(commands, txn=txn)
|
||||||
|
|
||||||
|
def create_ovn_fair_meter(self, meter_name, from_reload=False, txn=None):
|
||||||
|
self._create_ovn_fair_meter(meter_name, from_reload, txn)
|
||||||
|
self._create_ovn_fair_meter(meter_name, from_reload, txn,
|
||||||
|
stateless=True)
|
||||||
|
@ -28,6 +28,7 @@ from neutron._i18n import _
|
|||||||
from neutron.common.ovn import constants as ovn_const
|
from neutron.common.ovn import constants as ovn_const
|
||||||
from neutron.common.ovn import utils
|
from neutron.common.ovn import utils
|
||||||
from neutron.conf.services import logging as log_cfg
|
from neutron.conf.services import logging as log_cfg
|
||||||
|
from neutron.objects import securitygroup as sg_obj
|
||||||
from neutron.services.logapi.common import db_api
|
from neutron.services.logapi.common import db_api
|
||||||
from neutron.services.logapi.common import sg_callback
|
from neutron.services.logapi.common import sg_callback
|
||||||
from neutron.services.logapi.drivers import base
|
from neutron.services.logapi.drivers import base
|
||||||
@ -152,9 +153,17 @@ class OVNDriver(base.DriverBase):
|
|||||||
msg += " for network log {}".format(log_name)
|
msg += " for network log {}".format(log_name)
|
||||||
LOG.info(msg, acl_changes, acl_absents, acl_visits)
|
LOG.info(msg, acl_changes, acl_absents, acl_visits)
|
||||||
|
|
||||||
def _set_acls_log(self, pgs, ovn_txn, actions_enabled, log_name):
|
def _set_acls_log(self, pgs, context, ovn_txn, actions_enabled, log_name):
|
||||||
acl_changes, acl_visits = 0, 0
|
acl_changes, acl_visits = 0, 0
|
||||||
for pg in pgs:
|
for pg in pgs:
|
||||||
|
meter_name = self.meter_name
|
||||||
|
if ovn_const.OVN_DROP_PORT_GROUP_NAME not in pg["name"]:
|
||||||
|
stateful = (sg_obj.SecurityGroup
|
||||||
|
.get_sg_by_id(context, pg["name"]
|
||||||
|
.replace('pg_', '', 1)
|
||||||
|
.replace('_', '-')).stateful)
|
||||||
|
if not stateful:
|
||||||
|
meter_name = meter_name + ("_stateless")
|
||||||
for acl_uuid in pg["acls"]:
|
for acl_uuid in pg["acls"]:
|
||||||
acl_visits += 1
|
acl_visits += 1
|
||||||
acl = self.ovn_nb.lookup("ACL", acl_uuid)
|
acl = self.ovn_nb.lookup("ACL", acl_uuid)
|
||||||
@ -163,7 +172,7 @@ class OVNDriver(base.DriverBase):
|
|||||||
continue
|
continue
|
||||||
columns = {
|
columns = {
|
||||||
'log': acl.action in actions_enabled,
|
'log': acl.action in actions_enabled,
|
||||||
'meter': self.meter_name,
|
'meter': meter_name,
|
||||||
'name': log_name,
|
'name': log_name,
|
||||||
'severity': "info"
|
'severity': "info"
|
||||||
}
|
}
|
||||||
@ -183,7 +192,7 @@ class OVNDriver(base.DriverBase):
|
|||||||
for log_obj in log_objs:
|
for log_obj in log_objs:
|
||||||
pgs = self._pgs_from_log_obj(context, log_obj)
|
pgs = self._pgs_from_log_obj(context, log_obj)
|
||||||
actions_enabled = self._acl_actions_enabled(log_obj)
|
actions_enabled = self._acl_actions_enabled(log_obj)
|
||||||
self._set_acls_log(pgs, ovn_txn, actions_enabled,
|
self._set_acls_log(pgs, context, ovn_txn, actions_enabled,
|
||||||
utils.ovn_name(log_obj.id))
|
utils.ovn_name(log_obj.id))
|
||||||
|
|
||||||
def _pgs_all(self):
|
def _pgs_all(self):
|
||||||
@ -266,7 +275,7 @@ class OVNDriver(base.DriverBase):
|
|||||||
with self.ovn_nb.transaction(check_error=True) as ovn_txn:
|
with self.ovn_nb.transaction(check_error=True) as ovn_txn:
|
||||||
self._ovn_client.create_ovn_fair_meter(self.meter_name,
|
self._ovn_client.create_ovn_fair_meter(self.meter_name,
|
||||||
txn=ovn_txn)
|
txn=ovn_txn)
|
||||||
self._set_acls_log(pgs, ovn_txn, actions_enabled,
|
self._set_acls_log(pgs, context, ovn_txn, actions_enabled,
|
||||||
utils.ovn_name(log_obj.id))
|
utils.ovn_name(log_obj.id))
|
||||||
|
|
||||||
def create_log_precommit(self, context, log_obj):
|
def create_log_precommit(self, context, log_obj):
|
||||||
@ -334,7 +343,7 @@ class OVNDriver(base.DriverBase):
|
|||||||
if not self._unset_disabled_acls(context, log_obj, ovn_txn):
|
if not self._unset_disabled_acls(context, log_obj, ovn_txn):
|
||||||
pgs = self._pgs_from_log_obj(context, log_obj)
|
pgs = self._pgs_from_log_obj(context, log_obj)
|
||||||
actions_enabled = self._acl_actions_enabled(log_obj)
|
actions_enabled = self._acl_actions_enabled(log_obj)
|
||||||
self._set_acls_log(pgs, ovn_txn, actions_enabled,
|
self._set_acls_log(pgs, context, ovn_txn, actions_enabled,
|
||||||
utils.ovn_name(log_obj.id))
|
utils.ovn_name(log_obj.id))
|
||||||
|
|
||||||
def delete_log(self, context, log_obj):
|
def delete_log(self, context, log_obj):
|
||||||
@ -356,6 +365,8 @@ class OVNDriver(base.DriverBase):
|
|||||||
self._remove_acls_log(pgs, ovn_txn)
|
self._remove_acls_log(pgs, ovn_txn)
|
||||||
ovn_txn.add(self.ovn_nb.meter_del(self.meter_name,
|
ovn_txn.add(self.ovn_nb.meter_del(self.meter_name,
|
||||||
if_exists=True))
|
if_exists=True))
|
||||||
|
ovn_txn.add(self.ovn_nb.meter_del(
|
||||||
|
self.meter_name + "_stateless", if_exists=True))
|
||||||
LOG.info("All ACL logs cleared after deletion of log_obj %s",
|
LOG.info("All ACL logs cleared after deletion of log_obj %s",
|
||||||
log_obj.id)
|
log_obj.id)
|
||||||
return
|
return
|
||||||
|
@ -1026,10 +1026,9 @@ class TestLogMaintenance(_TestMaintenanceHelper,
|
|||||||
# Check a meter and fair meter exist
|
# Check a meter and fair meter exist
|
||||||
self.assertTrue(self.nb_api._tables['Meter'].rows)
|
self.assertTrue(self.nb_api._tables['Meter'].rows)
|
||||||
self.assertTrue(self.nb_api._tables['Meter_Band'].rows)
|
self.assertTrue(self.nb_api._tables['Meter_Band'].rows)
|
||||||
self.assertEqual(cfg.CONF.network_log.burst_limit,
|
self.assertEqual(len([*self.nb_api._tables['Meter'].rows.values()]),
|
||||||
[*self.nb_api._tables['Meter_Band'].rows.values()][0].burst_size)
|
len([*self.nb_api._tables['Meter_Band'].rows.values()]))
|
||||||
self.assertEqual(cfg.CONF.network_log.rate_limit,
|
self._check_meters_consistency()
|
||||||
[*self.nb_api._tables['Meter_Band'].rows.values()][0].rate)
|
|
||||||
# Update burst and rate limit values on the configuration
|
# Update burst and rate limit values on the configuration
|
||||||
ovn_config.cfg.CONF.set_override('burst_limit', CFG_NEW_BURST,
|
ovn_config.cfg.CONF.set_override('burst_limit', CFG_NEW_BURST,
|
||||||
group='network_log')
|
group='network_log')
|
||||||
@ -1039,7 +1038,16 @@ class TestLogMaintenance(_TestMaintenanceHelper,
|
|||||||
self.assertRaises(periodics.NeverAgain,
|
self.assertRaises(periodics.NeverAgain,
|
||||||
self.maint.check_fair_meter_consistency)
|
self.maint.check_fair_meter_consistency)
|
||||||
# Check meter band was effectively changed after the maintenance call
|
# Check meter band was effectively changed after the maintenance call
|
||||||
self.assertEqual(CFG_NEW_BURST,
|
self._check_meters_consistency(CFG_NEW_BURST, CFG_NEW_RATE)
|
||||||
[*self.nb_api._tables['Meter_Band'].rows.values()][0].burst_size)
|
|
||||||
self.assertEqual(CFG_NEW_RATE,
|
def _check_meters_consistency(self, new_burst=None, new_rate=None):
|
||||||
[*self.nb_api._tables['Meter_Band'].rows.values()][0].rate)
|
burst, rate = (new_burst, new_rate) if new_burst else (
|
||||||
|
cfg.CONF.network_log.burst_limit, cfg.CONF.network_log.rate_limit)
|
||||||
|
for meter in [*self.nb_api._tables['Meter'].rows.values()]:
|
||||||
|
meter_band = self.nb_api.lookup('Meter_Band', meter.bands[0].uuid)
|
||||||
|
if "_stateless" in meter.name:
|
||||||
|
self.assertEqual(int(burst / 2), meter_band.burst_size)
|
||||||
|
self.assertEqual(int(rate / 2), meter_band.rate)
|
||||||
|
else:
|
||||||
|
self.assertEqual(burst, meter_band.burst_size)
|
||||||
|
self.assertEqual(rate, meter_band.rate)
|
||||||
|
@ -30,6 +30,12 @@ class LogApiTestCaseBase(functional_base.TestOVNFunctionalBase):
|
|||||||
self._check_is_supported()
|
self._check_is_supported()
|
||||||
self.ctxt = context.Context('admin', 'fake_tenant')
|
self.ctxt = context.Context('admin', 'fake_tenant')
|
||||||
|
|
||||||
|
# Since these tests use the _create_network() from the unit test suite
|
||||||
|
# but _create_security_group() is from the functional tests, two
|
||||||
|
# different tenant_ids will be used unless we specify the following
|
||||||
|
# line in the code:
|
||||||
|
self._tenant_id = self.ctxt.project_id
|
||||||
|
|
||||||
def _check_is_supported(self):
|
def _check_is_supported(self):
|
||||||
if not self.log_driver.network_logging_supported(self.nb_api):
|
if not self.log_driver.network_logging_supported(self.nb_api):
|
||||||
self.skipTest("The current OVN version does not offer support "
|
self.skipTest("The current OVN version does not offer support "
|
||||||
|
@ -247,7 +247,16 @@ class TestOVNClientFairMeter(TestOVNClientBase,
|
|||||||
self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
|
self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
|
||||||
self.assertFalse(self.nb_idl.meter_del.called)
|
self.assertFalse(self.nb_idl.meter_del.called)
|
||||||
self.assertTrue(self.nb_idl.meter_add.called)
|
self.assertTrue(self.nb_idl.meter_add.called)
|
||||||
self.nb_idl.meter_add.assert_called_once_with(
|
self.nb_idl.meter_add.assert_any_call(
|
||||||
|
name=self._log_driver.meter_name + "_stateless",
|
||||||
|
unit="pktps",
|
||||||
|
rate=int(self.fake_cfg_network_log.rate_limit / 2),
|
||||||
|
fair=True,
|
||||||
|
burst_size=int(self.fake_cfg_network_log.burst_limit / 2),
|
||||||
|
may_exist=False,
|
||||||
|
external_ids={constants.OVN_DEVICE_OWNER_EXT_ID_KEY:
|
||||||
|
log_const.LOGGING_PLUGIN})
|
||||||
|
self.nb_idl.meter_add.assert_any_call(
|
||||||
name=self._log_driver.meter_name,
|
name=self._log_driver.meter_name,
|
||||||
unit="pktps",
|
unit="pktps",
|
||||||
rate=self.fake_cfg_network_log.rate_limit,
|
rate=self.fake_cfg_network_log.rate_limit,
|
||||||
@ -259,10 +268,17 @@ class TestOVNClientFairMeter(TestOVNClientBase,
|
|||||||
|
|
||||||
def test_create_ovn_fair_meter_unchanged(self):
|
def test_create_ovn_fair_meter_unchanged(self):
|
||||||
mock_find_rows = mock.Mock()
|
mock_find_rows = mock.Mock()
|
||||||
mock_find_rows.execute.return_value = [self._fake_meter()]
|
fake_meter1 = [self._fake_meter()]
|
||||||
|
fake_meter2 = [self._fake_meter(
|
||||||
|
name=self._log_driver.meter_name + "_stateless",
|
||||||
|
bands=[mock.Mock(uuid='tb_stateless')])]
|
||||||
|
mock_find_rows.execute.side_effect = [fake_meter1, fake_meter1,
|
||||||
|
fake_meter2, fake_meter2]
|
||||||
self.nb_idl.db_find_rows.return_value = mock_find_rows
|
self.nb_idl.db_find_rows.return_value = mock_find_rows
|
||||||
self.nb_idl.lookup.side_effect = lambda table, key, default: (
|
self.nb_idl.lookup.side_effect = lambda table, key, default: (
|
||||||
self._fake_meter_band() if key == "test_band" else default)
|
self._fake_meter_band() if key == "test_band" else
|
||||||
|
self._fake_meter_band_stateless() if key == "tb_stateless" else
|
||||||
|
default)
|
||||||
self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
|
self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
|
||||||
self.assertFalse(self.nb_idl.meter_del.called)
|
self.assertFalse(self.nb_idl.meter_del.called)
|
||||||
self.assertFalse(self.nb_idl.meter_add.called)
|
self.assertFalse(self.nb_idl.meter_add.called)
|
||||||
|
@ -21,6 +21,7 @@ from neutron.common import utils as neutron_utils
|
|||||||
|
|
||||||
from neutron.common.ovn import constants as ovn_const
|
from neutron.common.ovn import constants as ovn_const
|
||||||
from neutron.common.ovn import utils as ovn_utils
|
from neutron.common.ovn import utils as ovn_utils
|
||||||
|
from neutron.objects import securitygroup as sg_obj
|
||||||
from neutron.services.logapi.drivers.ovn import driver as ovn_driver
|
from neutron.services.logapi.drivers.ovn import driver as ovn_driver
|
||||||
from neutron.tests import base
|
from neutron.tests import base
|
||||||
from neutron.tests.unit import fake_resources
|
from neutron.tests.unit import fake_resources
|
||||||
@ -92,6 +93,15 @@ class TestOVNDriverBase(base.BaseTestCase):
|
|||||||
meter_band_obj_dict = {**meter_band_defaults_dict, **kwargs}
|
meter_band_obj_dict = {**meter_band_defaults_dict, **kwargs}
|
||||||
return mock.Mock(**meter_band_obj_dict)
|
return mock.Mock(**meter_band_obj_dict)
|
||||||
|
|
||||||
|
def _fake_meter_band_stateless(self, **kwargs):
|
||||||
|
meter_band_defaults_dict = {
|
||||||
|
'uuid': 'tb_stateless',
|
||||||
|
'rate': int(self.fake_cfg_network_log.rate_limit / 2),
|
||||||
|
'burst_size': int(self.fake_cfg_network_log.burst_limit / 2),
|
||||||
|
}
|
||||||
|
meter_band_obj_dict = {**meter_band_defaults_dict, **kwargs}
|
||||||
|
return mock.Mock(**meter_band_obj_dict)
|
||||||
|
|
||||||
|
|
||||||
class TestOVNDriver(TestOVNDriverBase):
|
class TestOVNDriver(TestOVNDriverBase):
|
||||||
def test_create(self):
|
def test_create(self):
|
||||||
@ -287,7 +297,8 @@ class TestOVNDriver(TestOVNDriverBase):
|
|||||||
self._nb_ovn.db_set.call_count)
|
self._nb_ovn.db_set.call_count)
|
||||||
|
|
||||||
@mock.patch.object(ovn_driver.LOG, 'info')
|
@mock.patch.object(ovn_driver.LOG, 'info')
|
||||||
def test__set_acls_log(self, m_info):
|
@mock.patch.object(sg_obj.SecurityGroup, 'get_sg_by_id')
|
||||||
|
def test__set_acls_log(self, get_sg, m_info):
|
||||||
pg_dict = self._fake_pg_dict(acls=['acl1', 'acl2', 'acl3', 'acl4'])
|
pg_dict = self._fake_pg_dict(acls=['acl1', 'acl2', 'acl3', 'acl4'])
|
||||||
log_name = 'test_obj_name'
|
log_name = 'test_obj_name'
|
||||||
used_name = 'test_used_name'
|
used_name = 'test_used_name'
|
||||||
@ -297,10 +308,14 @@ class TestOVNDriver(TestOVNDriverBase):
|
|||||||
return self._fake_acl()
|
return self._fake_acl()
|
||||||
return self._fake_acl(name=used_name)
|
return self._fake_acl(name=used_name)
|
||||||
|
|
||||||
|
sg = fake_resources.FakeSecurityGroup.create_one_security_group(
|
||||||
|
attrs={'stateful': True})
|
||||||
|
get_sg.return_value = sg
|
||||||
self._nb_ovn.lookup.side_effect = _mock_lookup
|
self._nb_ovn.lookup.side_effect = _mock_lookup
|
||||||
actions_enabled = self._log_driver._acl_actions_enabled(
|
actions_enabled = self._log_driver._acl_actions_enabled(
|
||||||
self._fake_log_obj(event=log_const.ALL_EVENT))
|
self._fake_log_obj(event=log_const.ALL_EVENT))
|
||||||
self._log_driver._set_acls_log([pg_dict], self._nb_ovn.transaction,
|
self._log_driver._set_acls_log([pg_dict], self.context,
|
||||||
|
self._nb_ovn.transaction,
|
||||||
actions_enabled, log_name)
|
actions_enabled, log_name)
|
||||||
info_args, _info_kwargs = m_info.call_args_list[0]
|
info_args, _info_kwargs = m_info.call_args_list[0]
|
||||||
self.assertIn('Set %d (out of %d visited) ACLs for network log %s',
|
self.assertIn('Set %d (out of %d visited) ACLs for network log %s',
|
||||||
|
Loading…
Reference in New Issue
Block a user