From 3f345c4a37369a677e8f61ea31a536b0e12a552e Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Thu, 13 Jul 2017 10:37:27 -0400 Subject: [PATCH] Update comments related to ipmi & old BMCs The help string for [ipmi]/command_retry_timeout is incorrect. From Sam Betts [1]: so looking at when this option was introduced ... it was originally introduced by the ipminative driver which used it as the timeout for waiting until the power state changed to the requested state. Reading between the lines in the commits that added this option and documentation, the "setting this too low might break things" comment originates from bugs in IPMInative. Later on the IPMItool driver then overloaded this configuration option and changed its meaning by introducing the min_command_interval config option. It continued to be used for the timeout for waiting for requested state, but it also now contributed to the the number of times a IPMItool command would retry retryable errors, which was/is worked out by dividing retry_timeout by min_command_interval. Now IPMInative no longer in tree I think we should clean up some of this information. [1] https://review.openstack.org/#/c/482631/ Change-Id: I8cd8e25a2fb224d477799a2e561573406f9427a9 --- doc/source/install/configure-ipmi.rst | 4 ++-- etc/ironic/ironic.conf.sample | 17 ++++++----------- ironic/conf/ipmi.py | 10 +++------- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/doc/source/install/configure-ipmi.rst b/doc/source/install/configure-ipmi.rst index ea7c3e946a..5a458d968e 100644 --- a/doc/source/install/configure-ipmi.rst +++ b/doc/source/install/configure-ipmi.rst @@ -48,8 +48,8 @@ IPMI configuration ~~~~~~~~~~~~~~~~~~ If there are slow or unresponsive BMCs in the environment, the -``command_retry_timeout`` configuration option in the ``[ipmi]`` section may -need to be lowered. The default is fairly conservative, as setting this timeout +``min_command_interval`` configuration option in the ``[ipmi]`` section may +need to be raised. The default is fairly conservative, as setting this timeout too low can cause older BMCs to crash and require a hard-reset. Collecting sensor data diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 5676c04d51..e82e15fd5e 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -1903,20 +1903,15 @@ # Maximum time in seconds to retry retryable IPMI operations. # (An operation is retryable, for example, if the requested -# operation fails because the BMC is busy.) There is a -# tradeoff when setting this value. Setting this too low may -# cause older BMCs to crash and require a hard reset. However, -# setting too high can cause the sync power state periodic -# task to hang when there are slow or unresponsive BMCs. -# (integer value) +# operation fails because the BMC is busy.) Setting this too +# high can cause the sync power state periodic task to hang +# when there are slow or unresponsive BMCs. (integer value) #command_retry_timeout = 60 # DEPRECATED: Maximum time in seconds to retry IPMI -# operations. There is a tradeoff when setting this value. -# Setting this too low may cause older BMCs to crash and -# require a hard reset. However, setting too high can cause -# the sync power state periodic task to hang when there are -# slow or unresponsive BMCs. (integer value) +# operations. Setting this too high can cause the sync power +# state periodic task to hang when there are slow or +# unresponsive BMCs. (integer value) # This option is deprecated for removal. # Its value may be silently ignored in the future. # Reason: Use option [ipmi]/command_retry_timeout to specify diff --git a/ironic/conf/ipmi.py b/ironic/conf/ipmi.py index c62b71dbba..e957d8c79c 100644 --- a/ironic/conf/ipmi.py +++ b/ironic/conf/ipmi.py @@ -25,17 +25,13 @@ opts = [ help=_('Maximum time in seconds to retry retryable IPMI ' 'operations. (An operation is retryable, for ' 'example, if the requested operation fails ' - 'because the BMC is busy.) There is a tradeoff when ' - 'setting this value. Setting this too low may cause ' - 'older BMCs to crash and require a hard reset. However, ' - 'setting too high can cause the sync power state ' + 'because the BMC is busy.) Setting this too high ' + 'can cause the sync power state ' 'periodic task to hang when there are slow or ' 'unresponsive BMCs.')), cfg.IntOpt('retry_timeout', help=_('Maximum time in seconds to retry IPMI operations. ' - 'There is a tradeoff when setting this value. Setting ' - 'this too low may cause older BMCs to crash and require ' - 'a hard reset. However, setting too high can cause the ' + 'Setting this too high can cause the ' 'sync power state periodic task to hang when there are ' 'slow or unresponsive BMCs.'), deprecated_for_removal=True,