Revert "Update wait_for_status_success() call to look at both type and status for status.conditions" #2372
Conversation
…d status…" This reverts commit 44eb90f.
WalkthroughThis change updates the implementation of the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ocp_resources/node_network_configuration_policy.py (1)
407-433: Consider adding a docstring to explain the method's behaviorWhile the implementation is clear, adding a docstring would improve maintainability by explaining the method's purpose, expected behavior, return values, and possible exceptions.
def wait_for_status_success(self): + """ + Wait for the Node Network Configuration Policy to reach a successful state. + + This method first waits for the configuration conditions to be ready (unknown or progressing), + then samples the status until it's successfully configured or fails. + + Returns: + str: The status reason (SUCCESSFULLY_CONFIGURED) when successful + + Raises: + NNCPConfigurationFailed: If the configuration fails or no matching node is found + TimeoutExpiredError: If the configuration doesn't complete within the timeout + """ failed_condition_reason = self.Conditions.Reason.FAILED_TO_CONFIGURE no_match_node_condition_reason = self.Conditions.Reason.NO_MATCHING_NODE
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/node_network_configuration_policy.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
ocp_resources/node_network_configuration_policy.py (4)
ocp_resources/exceptions.py (1)
NNCPConfigurationFailed(54-55)ocp_resources/node_network_state.py (1)
NodeNetworkState(12-112)ocp_resources/resource.py (6)
Resource(302-1295)ResourceEditor(1411-1624)patches(1450-1452)update(911-925)update(1454-1501)status(899-909)ocp_resources/utils/resource_constants.py (1)
Reason(43-47)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: python-module-install
- GitHub Check: tox
- GitHub Check: conventional-title
🔇 Additional comments (4)
ocp_resources/node_network_configuration_policy.py (4)
5-5: Updated imports to include retry from timeout_samplerThe inclusion of
retryfromtimeout_samplermodule is consistent with the requirements for the refactored code. This import is needed to support the updated implementation in this file.
14-14: Added TIMEOUT_5SEC constant importThe addition of the
TIMEOUT_5SECconstant is appropriate. This constant is used in the_wait_for_nncp_status_updatemethod (line 349), providing a defined interval for retries.
328-329: Commented out status transition time tracking codeThe code for tracking and comparing the last successful transition time has been commented out. This is consistent with the reversion of changes from PR #2289, removing the mechanism that waited for NNCP status updates after interface changes.
Also applies to: 334-336
411-426: Reverted wait_for_status_success implementationThis change reverts to a cleaner implementation of the status checking logic:
- First waits for configuration conditions to be in a known state (unknown or progressing) to avoid race conditions
- Uses a 1-second sampling interval instead of 5 seconds for more responsive status checking
- Directly compares the status value to predefined constants rather than iterating through condition objects
The implementation is more straightforward while maintaining the core functionality of waiting until the NNCP is either successfully configured or fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ocp_resources/node_network_configuration_policy.py (2)
14-14: Import includes unused constantThe import statement has been modified to include
TIMEOUT_5SEC, but this constant appears to be used only in the_wait_for_nncp_status_updatemethod, which is not directly called in the active code (it's commented out at line 336).Consider removing unused imports to keep the code clean:
-from ocp_resources.utils.constants import TIMEOUT_1MINUTE, TIMEOUT_4MINUTES, TIMEOUT_5SEC +from ocp_resources.utils.constants import TIMEOUT_1MINUTE, TIMEOUT_4MINUTES
394-396: Inappropriate log level for non-error messageSimilar to the previous comment, the code is using
logger.errorfor what appears to be informational output.-self.logger.error(f"absent_ifaces: Before update resource: {initial_success_status_time}") +self.logger.info(f"absent_ifaces: Before update resource: {initial_success_status_time}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/node_network_configuration_policy.py(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
ocp_resources/node_network_configuration_policy.py (3)
ocp_resources/exceptions.py (1)
NNCPConfigurationFailed(54-55)ocp_resources/node_network_state.py (1)
NodeNetworkState(12-112)ocp_resources/resource.py (5)
Resource(302-1295)patches(1450-1452)update(911-925)update(1454-1501)status(899-909)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: python-module-install
- GitHub Check: tox
- GitHub Check: conventional-title
🔇 Additional comments (2)
ocp_resources/node_network_configuration_policy.py (2)
5-5:✅ Verification successful
Verify if all imported modules are necessary
The imports seem to have been modified to include
TimeoutWatchandretry, but it's unclear if these are still required after the reversion of PR #2289.TimeoutWatchis used inwait_for_configuration_conditions_unknown_or_progressing, butretrydoesn't appear to be used directly in this file.
🏁 Script executed:
#!/bin/bash # Check for any usages of the retry function in this file grep -n "retry(" ocp_resources/node_network_configuration_policy.pyLength of output: 85
Clarification on Imported Modules Necessity
After verifying the code, it's confirmed that the
retryfunction imported fromtimeout_sampleris used in the file. Specifically, the@retry(decorator appears on line 348, making its inclusion necessary along withTimeoutWatchwhich is used inwait_for_configuration_conditions_unknown_or_progressing.
- Usage of
retryfound at line 348.TimeoutWatchremains essential in its respective function.
414-429:❓ Verification inconclusive
Significantly altered status checking logic
The implementation of
wait_for_status_successhas been completely rewritten. The key changes are:
- Added an initial call to
wait_for_configuration_conditions_unknown_or_progressing- Changed the sleep interval from the previous 5 seconds (likely) to 1 second
- Now directly checks the status value instead of iterating through condition objects
This change affects the core behavior of how the system waits for status updates. Testing should verify that this reversion addresses the issues encountered with PR #2289.
Since this is a reversion PR, ensure that thorough testing is performed to verify that the issues with PR #2289 are resolved and that this implementation works as expected with different timing conditions.
🏁 Script executed:
#!/bin/bash # Description: Check for any references to this method in test files # Check for test calls to wait_for_status_success method echo "Looking for test references to wait_for_status_success method..." rg -n "wait_for_status_success" --type py tests/Length of output: 187
Critical: Manual Verification Required for Revised Wait Logic
The revised logic in
wait_for_status_successshows significant changes:
- An initial call to
wait_for_configuration_conditions_unknown_or_progressinghas been added.- The sleep interval is now 1 second instead of an assumed 5 seconds.
- The status check now directly compares status values instead of iterating through condition objects.
Action Items:
- Ensure that the new timing behavior has been thoroughly verified in real-world conditions.
- Although searching the test files did not reveal direct references to
wait_for_status_success, please confirm manually (or enhance tests) that the altered method correctly resolves the issues observed in PR Update wait_for_status_success() call to look at both type and status for status.conditions #2289.- Verify that the early call to
wait_for_configuration_conditions_unknown_or_progressingalong with the changed sleep interval provides sufficient stability across various timing scenarios.
| self.logger.error(f"absent_ifaces: Before update resource: {initial_success_status_time}") | ||
| ResourceEditor( | ||
| patches={self: {"spec": {"desiredState": {"interfaces": self.desired_state["interfaces"]}}}} | ||
| ).update() | ||
|
|
||
| # If the NNCP failed on setup, then its tear-down AVAIALBLE status will necessarily be the first. | ||
| if initial_success_status_time: | ||
| self._wait_for_nncp_status_update(initial_transition_time=initial_success_status_time) | ||
| # if initial_success_status_time: | ||
| # self._wait_for_nncp_status_update(initial_transition_time=initial_success_status_time) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error logging and commented code
There are two issues here:
- The log message is using
logger.errorfor what appears to be informational output - The code for waiting for NNCP status updates has been commented out rather than removed
If this is a revert PR, consider either:
- Properly removing the commented-out code rather than keeping it commented
- Using the appropriate log level (
infoinstead oferror) for non-error messages
-self.logger.error(f"absent_ifaces: Before update resource: {initial_success_status_time}")
+self.logger.info(f"absent_ifaces: Before update resource: {initial_success_status_time}")
-# if initial_success_status_time:
-# self._wait_for_nncp_status_update(initial_transition_time=initial_success_status_time)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.error(f"absent_ifaces: Before update resource: {initial_success_status_time}") | |
| ResourceEditor( | |
| patches={self: {"spec": {"desiredState": {"interfaces": self.desired_state["interfaces"]}}}} | |
| ).update() | |
| # If the NNCP failed on setup, then its tear-down AVAIALBLE status will necessarily be the first. | |
| if initial_success_status_time: | |
| self._wait_for_nncp_status_update(initial_transition_time=initial_success_status_time) | |
| # if initial_success_status_time: | |
| # self._wait_for_nncp_status_update(initial_transition_time=initial_success_status_time) | |
| self.logger.info(f"absent_ifaces: Before update resource: {initial_success_status_time}") | |
| ResourceEditor( | |
| patches={self: {"spec": {"desiredState": {"interfaces": self.desired_state["interfaces"]}}}} | |
| ).update() | |
| # If the NNCP failed on setup, then its tear-down AVAIALBLE status will necessarily be the first. |
|
Does not look like a clean revert as it is removing other people's changes as well. Specially after almost 3 months of the merge, may be fixing what does not work would be more simple? |
Thanks, this is only for testing. |
|
/hold |
Reverts #2289
Summary by CodeRabbit