fix: ocp_resources, resource: Extend 'wait_for_condition' criteria #2544
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
1225-1242: Clarify docstring for optional parameters.The docstring should explicitly state:
- That
reasonandmessageare optional filters- That
messageperforms a substring match (not exact match)- The behavior when these parameters are empty strings (i.e., they are ignored)
Apply this diff to improve the docstring:
def wait_for_condition( self, condition: str, status: str, timeout: int = 300, sleep_time: int = 1, reason: str = "", message: str = "" ) -> None: """ Wait for Resource condition to be in desire status. Args: condition (str): Condition to query. status (str): Expected condition status. - reason (str): Expected condition reason. - message (str): Expected condition text inclusion. + reason (str, optional): Expected condition reason. If empty (default), reason is not checked. + message (str, optional): Substring to find in condition message. If empty (default), message is not checked. timeout (int): Time to wait for the resource. sleep_time(int): Interval between each retry when checking the resource's condition. Raises: TimeoutExpiredError: If Resource condition in not in desire status. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/resource.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ocp_resources/resource.py (3)
ocp_resources/node_network_configuration_policy.py (1)
status(364-367)ocp_resources/datavolume.py (1)
wait(250-258)ocp_resources/virtual_machine_import.py (1)
wait(194-231)
🔇 Additional comments (1)
ocp_resources/resource.py (1)
1244-1250: Verify timeout allocation between existence check and condition verification.The current implementation calls
self.wait(timeout=timeout)with the full timeout value, then usestimeout_watcher.remaining_time()for condition checking. If the resource takes most of the timeout to exist, there may be insufficient time remaining to verify the condition, leading to premature timeout.Consider whether the timeout should be split between the existence check and condition verification, or if this behavior is intentional. For better timeout management, you might want:
self.wait(timeout=timeout_watcher.remaining_time(), sleep=sleep_time)However, this would require careful handling since
remaining_time()is called beforeself.wait(), which is fine. The current code should work correctly - my apologies for the confusion. Theself.wait()is called with the user-provided timeout, and then the condition checking uses whatever time remains from the original timeout window.
EdDev
force-pushed
the
lookup-condition
branch
from
d638ead to
8264500
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ocp_resources/resource.py (2)
1238-1244: Clarify param semantics and fix minor grammar in the docstring.Document that reason/message are optional and that message is a substring check. Also fix “desire(d)” wording.
- Wait for Resource condition to be in desire status. + Wait for Resource condition to be in desired status. @@ - reason (None): Expected condition reason. - message (str): Expected condition text inclusion. + reason (str | None): Optional exact condition reason to match. If None, ignored. + message (str): Optional substring required to be included in the condition message. If empty, ignored. @@ - TimeoutExpiredError: If Resource condition in not in desire status. + TimeoutExpiredError: If Resource condition is not in the desired status.
1248-1251: Include reason/message in the log context.Helps with triage when timeouts occur.
- self.logger.info(f"Wait for {self.kind}/{self.name}'s '{condition}' condition to be '{status}'") + details = [] + if reason is not None: + details.append(f"reason={reason}") + if message: + details.append(f"message~={message!r}") + suffix = f" ({', '.join(details)})" if details else "" + self.logger.info(f"Wait for {self.kind}/{self.name} condition '{condition}' == '{status}'{suffix}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/resource.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-06T08:02:12.098Z
Learnt from: sbahar619
PR: RedHatQE/openshift-python-wrapper#2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-06T08:02:12.098Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.
Applied to files:
ocp_resources/resource.py
🧬 Code graph analysis (1)
ocp_resources/resource.py (2)
ocp_resources/node_network_configuration_policy.py (1)
status(364-367)ocp_resources/virtual_machine_import.py (1)
wait(194-231)
myakove
approved these changes
Oct 28, 2025
rnetser
reviewed
Oct 28, 2025
EdDev
changed the title
Collaborator
|
/approve |
Collaborator
|
@EdDev can you please rebase and verity? I want to merge this. |
In some scenarios, tests are looking for a specific reason or message to be included in the condition. Although not a formal strict API, the message text includes extended reasoning which is useful for a user operator. Therefore, it is also included but with an inclusion check. See usage at: https://github.com/RedHatQE/openshift-virtualization-tests/blob/0116f056176dbb25da5e8846537264dccb8ef202/tests/network/general/test_bridge_marker.py#L130 Signed-off-by: Edward Haas <[email protected]>
EdDev
force-pushed
the
lookup-condition
branch
from
8264500 to
1d71613
Compare
Contributor
Author
|
change: Rebase |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
1240-1241: Consider clarifying optional parameter behavior in docstring.While the current docstring is accurate, it could be more explicit about how the optional parameters work:
reason (None)→ currently doesn't clarify thatNonemeans "match any reason"message (str)→ doesn't clarify that empty string means "ignore message content"Consider this enhancement:
Args: condition (str): Condition to query. status (str): Expected condition status. - reason (None): Expected condition reason. - message (str): Expected condition text inclusion. + reason (str | None): Expected condition reason. If None (default), any reason matches. + message (str): Expected substring in condition message. If empty (default), message is not checked. timeout (int): Time to wait for the resource. sleep_time(int): Interval between each retry when checking the resource's condition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/resource.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sbahar619
Repo: RedHatQE/openshift-python-wrapper PR: 2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-06T08:02:12.098Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.
Learnt from: sbahar619
Repo: RedHatQE/openshift-python-wrapper PR: 2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-08T23:43:22.342Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.
📚 Learning: 2024-10-06T08:02:12.098Z
Learnt from: sbahar619
Repo: RedHatQE/openshift-python-wrapper PR: 2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-06T08:02:12.098Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.
Applied to files:
ocp_resources/resource.py
📚 Learning: 2025-10-26T09:13:35.131Z
Learnt from: EdDev
Repo: RedHatQE/openshift-python-wrapper PR: 2544
File: ocp_resources/resource.py:1258-1266
Timestamp: 2025-10-26T09:13:35.131Z
Learning: In ocp_resources/resource.py, EdDev prefers to avoid `continue` statements in loops, favoring single exit points and simple "if-condition-action" structures over early loop skip patterns for better readability and code tracking.
Applied to files:
ocp_resources/resource.py
🧬 Code graph analysis (1)
ocp_resources/resource.py (2)
ocp_resources/node_network_configuration_policy.py (1)
status(364-367)ocp_resources/virtual_machine_import.py (1)
wait(194-231)
🔇 Additional comments (3)
ocp_resources/resource.py (3)
1225-1233: LGTM! Signature changes are well-designed.The new optional parameters are correctly typed and defaulted:
reason: str | None = Noneproperly distinguishes "not specified" from an empty string valuemessage: str = ""makes the message substring check optional (empty string matches any string)The signature maintains backward compatibility while enabling the extended condition matching criteria.
1251-1251: Good improvement: waiting for resource before checking conditions.The addition of
self.wait()before condition polling is a sensible enhancement:
- Ensures the resource exists before attempting to evaluate its conditions
- Uses
TimeoutWatchto track remaining time for the subsequent condition checks (line 1253)- Provides clearer separation of concerns (existence vs. condition state)
This is a behavior change from the previous implementation but represents better design.
1258-1266: LGTM! Condition matching logic is correct and clean.The implementation properly handles optional reason and message parameters:
Reason handling (lines 1261-1263):
- When
reason is None(default): reason field is not added to either dict, so the comparison ignores reason entirely ✓- When
reasonis provided: both dicts include the reason field, requiring an exact match ✓cond.get("reason", "")safely handles conditions without a reason fieldMessage handling (line 1265):
- When
message = ""(default): the substring check always passes (empty string is in any string), making it optional ✓- When
messageis provided: requires the message to contain the substring ✓cond.get("message", "")safely handles the optional message fieldThe dictionary comparison approach is elegant and respects your preference for avoiding
continuestatements while maintaining clear, readable logic.Based on learnings
Contributor
|
/verified with RedHatQE/openshift-virtualization-tests#2535 |
Collaborator
|
/approve |
rnetser
pushed a commit
to RedHatQE/openshift-virtualization-tests
that referenced
this pull request
Nov 13, 2025
…2535) ##### What this PR does / why we need it: Use the existing 'wait_for_condition' to query for a specific condition. Drop local implementation of condition parsing. * Depends on enhanced 'wait_for_condition' method from the wrapper, in which the 'reason' and 'message' arguments are available. ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: Depends on: RedHatQE/openshift-python-wrapper#2544 ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated bridge marker tests to replace a dedicated helper with direct condition checks and an explicit short timeout, improving reliability and clarity when asserting scheduling failures due to insufficient bridge resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Edward Haas <[email protected]>
jdao-rh
pushed a commit
to jdao-rh/openshift-virtualization-tests
that referenced
this pull request
Nov 17, 2025
…edHatQE#2535) ##### What this PR does / why we need it: Use the existing 'wait_for_condition' to query for a specific condition. Drop local implementation of condition parsing. * Depends on enhanced 'wait_for_condition' method from the wrapper, in which the 'reason' and 'message' arguments are available. ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: Depends on: RedHatQE/openshift-python-wrapper#2544 ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated bridge marker tests to replace a dedicated helper with direct condition checks and an explicit short timeout, improving reliability and clarity when asserting scheduling failures due to insufficient bridge resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Edward Haas <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
In some scenarios, tests are looking for a specific reason or message to be included in the condition.
Although not a formal strict API, the message text includes extended reasoning which is useful for a user operator.
Therefore, it is also included but with an inclusion check.
See usage at:
https://github.com/RedHatQE/openshift-virtualization-tests/blob/0116f056176dbb25da5e8846537264dccb8ef202/tests/network/general/test_bridge_marker.py#L130
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit