From 4acc0ed4cea444ce660cc49f917191d41e3a04f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Bouliane?= Date: Thu, 10 Nov 2016 15:41:18 -0500 Subject: [PATCH] Make SNMP UDP transport settings configurable SNMP timeout and retries are now configurable through ironic.conf. This allows to have custom configurations when certain PDUs are slower than others. The config defaults are the same defaults that cmdgen.UdpTransportTarget uses (1.0 second timeout and 5 retries). The config is now reflecting these defaults. Closes-Bug: #1640932 Change-Id: I2490902ad2e39e8e4dc34484799a0dae96bc57f8 --- etc/ironic/ironic.conf.sample | 11 ++++++++ ironic/conf/snmp.py | 13 ++++++++- ironic/drivers/modules/snmp.py | 5 +++- .../tests/unit/drivers/modules/test_snmp.py | 28 +++++++++++++++++-- ...p-transport-settings-67419be988fcff40.yaml | 7 +++++ 5 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/snmp-driver-udp-transport-settings-67419be988fcff40.yaml diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index ce5dc10200..753e394aa9 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -3609,6 +3609,17 @@ # Minimum value: 0 #reboot_delay = 0 +# Response timeout in seconds used for UDP transport. Timeout +# should be a multiple of 0.5 seconds and is applicable to +# each retry. (floating point value) +# Minimum value: 0 +#udp_transport_timeout = 1.0 + +# Maximum number of UDP request retries, 0 means just a single +# request. (integer value) +# Minimum value: 0 +#udp_transport_retries = 5 + [ssl] diff --git a/ironic/conf/snmp.py b/ironic/conf/snmp.py index 1dd84181b6..1a7026e2cc 100644 --- a/ironic/conf/snmp.py +++ b/ironic/conf/snmp.py @@ -29,7 +29,18 @@ opts = [ default=0, min=0, help=_('Time (in seconds) to sleep between when rebooting ' - '(powering off and on again)')) + '(powering off and on again)')), + cfg.FloatOpt('udp_transport_timeout', + default=1.0, + min=0.0, + help=_('Response timeout in seconds used for UDP transport. ' + 'Timeout should be a multiple of 0.5 seconds and ' + 'is applicable to each retry.')), + cfg.IntOpt('udp_transport_retries', + default=5, + min=0, + help=_('Maximum number of UDP request retries, ' + '0 means no retries.')), ] diff --git a/ironic/drivers/modules/snmp.py b/ironic/drivers/modules/snmp.py index 5b9ffb6514..1e2f2a73f8 100644 --- a/ironic/drivers/modules/snmp.py +++ b/ironic/drivers/modules/snmp.py @@ -122,7 +122,10 @@ class SNMPClient(object): # The transport target accepts timeout and retries parameters, which # default to 1 (second) and 5 respectively. These are deemed sensible # enough to allow for an unreliable network or slow device. - return cmdgen.UdpTransportTarget((self.address, self.port)) + return cmdgen.UdpTransportTarget( + (self.address, self.port), + timeout=CONF.snmp.udp_transport_timeout, + retries=CONF.snmp.udp_transport_retries) def get(self, oid): """Use PySNMP to perform an SNMP GET operation on a single object. diff --git a/ironic/tests/unit/drivers/modules/test_snmp.py b/ironic/tests/unit/drivers/modules/test_snmp.py index ca2da7ea50..43f80e7f4e 100644 --- a/ironic/tests/unit/drivers/modules/test_snmp.py +++ b/ironic/tests/unit/drivers/modules/test_snmp.py @@ -75,7 +75,10 @@ class SNMPClientTestCase(base.TestCase): client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V3) client._get_transport() mock_cmdgen.assert_called_once_with() - mock_transport.assert_called_once_with((client.address, client.port)) + mock_transport.assert_called_once_with( + (client.address, client.port), + retries=CONF.snmp.udp_transport_retries, + timeout=CONF.snmp.udp_transport_timeout) @mock.patch.object(cmdgen, 'UdpTransportTarget', autospec=True) def test__get_transport_err(self, mock_transport, mock_cmdgen): @@ -83,7 +86,28 @@ class SNMPClientTestCase(base.TestCase): client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V3) self.assertRaises(snmp_error.PySnmpError, client._get_transport) mock_cmdgen.assert_called_once_with() - mock_transport.assert_called_once_with((client.address, client.port)) + mock_transport.assert_called_once_with( + (client.address, client.port), + retries=CONF.snmp.udp_transport_retries, + timeout=CONF.snmp.udp_transport_timeout) + + @mock.patch.object(cmdgen, 'UdpTransportTarget', autospec=True) + def test__get_transport_custom_timeout(self, mock_transport, mock_cmdgen): + self.config(udp_transport_timeout=2.0, group='snmp') + client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V3) + client._get_transport() + mock_cmdgen.assert_called_once_with() + mock_transport.assert_called_once_with((client.address, client.port), + retries=5, timeout=2.0) + + @mock.patch.object(cmdgen, 'UdpTransportTarget', autospec=True) + def test__get_transport_custom_retries(self, mock_transport, mock_cmdgen): + self.config(udp_transport_retries=10, group='snmp') + client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V3) + client._get_transport() + mock_cmdgen.assert_called_once_with() + mock_transport.assert_called_once_with((client.address, client.port), + retries=10, timeout=1.0) @mock.patch.object(snmp.SNMPClient, '_get_transport', autospec=True) @mock.patch.object(snmp.SNMPClient, '_get_auth', autospec=True) diff --git a/releasenotes/notes/snmp-driver-udp-transport-settings-67419be988fcff40.yaml b/releasenotes/notes/snmp-driver-udp-transport-settings-67419be988fcff40.yaml new file mode 100644 index 0000000000..eae9c8271f --- /dev/null +++ b/releasenotes/notes/snmp-driver-udp-transport-settings-67419be988fcff40.yaml @@ -0,0 +1,7 @@ +--- +features: + - Adds SNMP request timeout and retries settings for the SNMP UDP transport. + Some SNMP devices take longer than others to respond. + The new Ironic configuration settings ``[snmp]/udp_transport_retries`` + and ``[snmp]/udp_transport_timeout`` allow to change the number of + retries and the timeout values respectively for the the SNMP driver.