feat: Retry exceptions in create() by adding PROTOCOL_ERROR_EXCEPTION_DICT #2581

Ahmad-Hafe · 2025-11-17T09:27:41Z

Short description:

Update the create() method to handle ProtocolError exceptions during resource creation by including PROTOCOL_ERROR_EXCEPTION_DICT in the default exceptions_dict. This ensures retries for connection aborts and remote disconnects without changing the global defaults.

More details:

failed on setup with "urllib3.exceptions.ProtocolError: ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response'))"

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewer:

Bug:

Summary by CodeRabbit

Bug Fixes
- Resource creation now retries a broader set of transient and protocol-related errors by default, improving reliability and reducing intermittent failures during provisioning.
- Fewer manual retries are needed for operations that previously failed due to temporary protocol or network issues.
- Default retry behavior expanded to handle additional protocol error cases, making provisioning more resilient without configuration changes.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-17T09:27:51Z

Walkthrough

The default value of exceptions_dict in Resource.create was extended from DEFAULT_CLUSTER_RETRY_EXCEPTIONS to the union DEFAULT_CLUSTER_RETRY_EXCEPTIONS | PROTOCOL_ERROR_EXCEPTION_DICT; signature changed accordingly, behavior otherwise unchanged.

Changes

Cohort / File(s)	Summary
Resource creation exception handling `ocp_resources/resource.py`	Updated `Resource.create` method signature to set `exceptions_dict` default to `DEFAULT_CLUSTER_RETRY_EXCEPTIONS

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Check that the union of the two dicts yields the intended combined mapping and that key collisions are handled as expected.
Verify callers that pass their own exceptions_dict are unaffected.
Confirm retry logic uses the merged mapping correctly.

Possibly related PRs

feat: Add retry on resource creation #2572 — Also modifies Resource.create retry/exceptions handling; directly related.

Suggested labels

verified, can-be-merged, approved-myakove

Suggested reviewers

myakove
dbasunag

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding PROTOCOL_ERROR_EXCEPTION_DICT to the create() method's default exceptions_dict parameter for retry handling.
Description check	✅ Passed	The PR description covers the key sections with Short description and More details filled in, though What this PR does/why we need it, Which issue(s) this PR fixes, Special notes for reviewer, and Bug sections are empty.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

redhat-qe-bot · 2025-11-17T09:28:40Z

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

Reviewer Assignment: Reviewers are automatically assigned based on the "
"OWNERS file in the repository root
"
"* Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are "
"automatically applied based on changes
"
f"* Issue Creation: A tracking issue is created for this PR and will be closed when the PR is merged or closed
"
"* Pre-commit Checks: pre-commit runs "
"automatically if .pre-commit-config.yaml exists
"
Branch Labeling: Branch-specific labels are applied to track the target branch
Auto-verification: Auto-verified users have their PRs automatically marked as verified

📋 Available Commands

PR Status Management

/wip - Mark PR as work in progress (adds WIP: prefix to title)
/wip cancel - Remove work in progress status
/hold - Block PR merging (approvers only)
/hold cancel - Unblock PR merging
/verified - Mark PR as verified
/verified cancel - Remove verification status
/reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)

Review & Approval

/lgtm - Approve changes (looks good to me)
/approve - Approve PR (approvers only)
/automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
/assign-reviewers - Assign reviewers based on OWNERS file
/assign-reviewer @username - Assign specific reviewer
/check-can-merge - Check if PR meets merge requirements

Testing & Validation

/retest tox - Run Python test suite with tox
/retest python-module-install - Test Python package installation
/retest conventional-title - Validate commit message format
/retest all - Run all available tests

Container Operations

/build-and-push-container - Build and push container image (tagged with PR number)
- Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

/cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
- Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

/<label-name> - Add a label to the PR
/<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

Approval: /approve from at least one approver
LGTM Count: Minimum 0 /lgtm from reviewers
Status Checks: All required status checks must pass
No Blockers: No WIP, hold, or conflict labels
Verified: PR must be marked as verified (if verification is enabled)

📊 Review Process

Approvers and Reviewers

Approvers:

myakove
rnetser

Reviewers:

dbasunag
myakove
rnetser

Available Labels

hold
verified
wip
lgtm
approve
automerge

💡 Tips

WIP Status: Use /wip when your PR is not ready for review
Verification: The verified label is automatically removed on each new commit
Cherry-picking: Cherry-pick labels are processed when the PR is merged
Container Builds: Container images are automatically tagged with the PR number
Permission Levels: Some commands require approver permissions
Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

ocp_resources/resource.py (1)
1007-1009: Update the docstring to reflect the optional parameter and default behavior.

The method signature correctly makes exceptions_dict optional, but the docstring at line 1016 doesn't document that it's optional or what happens when it's not provided. Consider updating the docstring to clarify the default behavior.

Apply this diff to improve the docstring:
         Args:
             wait (bool) : True to wait for resource status.
-            exceptions_dict (dict[type[Exception], list[str]]): Dictionary of exceptions to retry on.
+            exceptions_dict (dict[type[Exception], list[str]] | None): Dictionary of exceptions to retry on.
+                If None (default), merges DEFAULT_CLUSTER_RETRY_EXCEPTIONS with PROTOCOL_ERROR_EXCEPTION_DICT.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5206503 and 7019d73.

📒 Files selected for processing (1)

ocp_resources/resource.py (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

ocp_resources/resource.py (1)

ocp_resources/virtual_machine_import.py (1)

wait (192-230)

🔇 Additional comments (1)

ocp_resources/resource.py (1)

1021-1028: LGTM! Clean implementation of the default exceptions merge.

The logic correctly merges the default cluster retry exceptions with protocol error exceptions when no custom exceptions_dict is provided. This achieves the PR objective of handling ProtocolError exceptions during resource creation without modifying global defaults. The approach is consistent with similar patterns used in the wait() and wait_for_status() methods.

ocp_resources/resource.py

rnetser · 2025-11-17T12:32:14Z

/lgtm

myakove · 2025-11-17T18:38:37Z

/approve

Ahmad-Hafe · 2025-11-17T20:31:27Z

/build-and-push-container

redhat-qe-bot1 · 2025-11-17T20:32:00Z

No build-and-push-container configured for this repository

Ahmad-Hafe · 2025-11-18T19:06:20Z

/build-and-push-container --build-arg OPENSHIFT_PYTHON_WRAPPER_COMMIT=da58386833be3aa53575ce23ea331580c29736b1

redhat-qe-bot · 2025-11-18T19:06:54Z

No build-and-push-container configured for this repository

Ahmad-Hafe · 2025-11-18T19:40:09Z

build image

/build-and-push-container --build-arg OPENSHIFT_PYTHON_WRAPPER_COMMIT=da58386833be3aa53575ce23ea331580c29736b1

RedHatQE/openshift-virtualization-tests#2730

myakove · 2025-11-18T19:59:14Z

@Ahmad-Hafe if you comment to build and push and you get an answer that it's not configured for this repository it's not going to work if you comment a few more times

Ahmad-Hafe · 2025-11-18T21:07:57Z

hi @myakove thanks for your comment
I manage to buid image with this wrapper code
and waiting the run to finish, so I could see results

myakove · 2025-11-18T21:13:05Z

@Ahmad-Hafe do not post internet links here

Ahmad-Hafe · 2025-11-19T07:27:16Z

/verified
run #3626 in openshift-virtualization-tests-runner

Ahmad-Hafe · 2025-11-19T07:34:39Z

/cherry-pick v4.20

redhat-qe-bot2 · 2025-11-19T07:35:13Z

Cherry-pick requested for PR: feat: Retry exceptions in create() by adding PROTOCOL_ERROR_EXCEPTION_DICT by user Ahmad-Hafe
Adding label/s cherry-pick-v4.20 for automatic cheery-pick once the PR is merged

Ahmad-Hafe · 2025-11-19T11:55:01Z

/verified
run #3626 in openshift-virtualization-tests-runner

myakove · 2025-11-19T18:12:55Z

#3626

What is this? I search the internet for #3626 , I also ask the AI and not one know what is this.

Ahmad-Hafe · 2025-11-19T20:04:34Z

Hi @myakove ,
#3626 is the internal run number I used to verify the PR.
Understood about not adding these references here — I won’t include them going forward.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

ocp_resources/resource.py (1)

1007-1012: create() now correctly retries on protocol errors; optional thought on consistency

The new default exceptions_dict (union of DEFAULT_CLUSTER_RETRY_EXCEPTIONS and PROTOCOL_ERROR_EXCEPTION_DICT) cleanly achieves the PR goal of making create() resilient to protocol-level disconnects without mutating the global constants. Behavior of existing callers that pass their own exceptions_dict remains unchanged.

If, in future, you want protocol errors handled uniformly for other operations (e.g., instance lookups or get() classmethods), a small follow‑up could be to either:

extend retry_cluster_exceptions’s default exceptions_dict to also include PROTOCOL_ERROR_EXCEPTION_DICT, or

have those other call sites pass in a similarly extended dict.

Not required for this PR, just a possible consistency improvement later.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c8381f and d4b0b69.

📒 Files selected for processing (1)

ocp_resources/resource.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

ocp_resources/resource.py (1)

ocp_resources/virtual_machine_import.py (1)

wait (192-230)

Ahmad-Hafe · 2025-11-24T21:04:33Z

/verified

…reate()

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

ocp_resources/resource.py (1)
1012-1015: Create retries now cover ProtocolError cases – looks good; consider harmonizing dict-union order.

The new default exceptions_dict including PROTOCOL_ERROR_EXCEPTION_DICT matches the PR goal and aligns create() with how other operations use protocol-related retries. No functional issues spotted.

Minor consistency nit: other methods use a different merge order (PROTOCOL_ERROR_EXCEPTION_DICT | DEFAULT_CLUSTER_RETRY_EXCEPTIONS in wait_for_status, and **PROTOCOL_ERROR_EXCEPTION_DICT, **…, **DEFAULT_CLUSTER_RETRY_EXCEPTIONS in wait, where the last wins on key clashes). Here, DEFAULT_CLUSTER_RETRY_EXCEPTIONS | PROTOCOL_ERROR_EXCEPTION_DICT would make protocol-specific entries override any overlapping default entries, whereas in wait_for_status the defaults would win.

If you want uniform behavior and simpler reasoning later, consider aligning the merge order with wait_for_status, e.g.:
-        exceptions_dict: dict[type[Exception], list[str]] = DEFAULT_CLUSTER_RETRY_EXCEPTIONS
-        | PROTOCOL_ERROR_EXCEPTION_DICT,
+        exceptions_dict: dict[type[Exception], list[str]] = PROTOCOL_ERROR_EXCEPTION_DICT
+        | DEFAULT_CLUSTER_RETRY_EXCEPTIONS,

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d4b0b69 and d1bf216.

📒 Files selected for processing (1)

ocp_resources/resource.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

ocp_resources/resource.py (1)

ocp_resources/virtual_machine_import.py (1)

wait (192-230)

redhat-qe-bot requested a review from myakove November 17, 2025 09:28

redhat-qe-bot assigned Ahmad-Hafe Nov 17, 2025

redhat-qe-bot requested a review from dbasunag November 17, 2025 09:28

redhat-qe-bot added the branch-main label Nov 17, 2025

redhat-qe-bot requested a review from rnetser November 17, 2025 09:28

redhat-qe-bot added the size/XS label Nov 17, 2025

coderabbitai bot reviewed Nov 17, 2025

View reviewed changes

redhat-qe-bot1 added the commented-coderabbitai[bot] label Nov 17, 2025

rnetser requested changes Nov 17, 2025

View reviewed changes

ocp_resources/resource.py Outdated Show resolved Hide resolved

redhat-qe-bot added the changes-requested-rnetser label Nov 17, 2025

Ahmad-Hafe force-pushed the add_protocol_error_exception_dict_retry_create branch from 7019d73 to da58386 Compare November 17, 2025 11:34

Ahmad-Hafe requested a review from rnetser November 17, 2025 11:34

rh-bot-1 removed changes-requested-rnetser commented-coderabbitai[bot] labels Nov 17, 2025

redhat-qe-bot added the lgtm-rnetser label Nov 17, 2025

redhat-qe-bot1 added the approved-myakove label Nov 17, 2025

redhat-qe-bot added the needs-rebase label Nov 18, 2025

redhat-qe-bot added the verified label Nov 19, 2025

redhat-qe-bot2 added the can-be-merged label Nov 19, 2025

redhat-qe-bot2 added the cherry-pick-v4.20 label Nov 19, 2025

Ahmad-Hafe force-pushed the add_protocol_error_exception_dict_retry_create branch from da58386 to 7c8381f Compare November 19, 2025 11:52

redhat-qe-bot removed needs-rebase verified lgtm-rnetser approved-myakove labels Nov 19, 2025

rh-bot-1 removed the can-be-merged label Nov 19, 2025

redhat-qe-bot added the verified label Nov 19, 2025

Ahmad-Hafe mentioned this pull request Nov 20, 2025

[DO NOT REVIEW] [IMAGE BUILD]chore: trigger container build for OPENSHIFT_PYTHON_WRAPPER_COMMIT RedHatQE/openshift-virtualization-tests#2730

Open

redhat-qe-bot1 added the needs-rebase label Nov 23, 2025

Ahmad-Hafe force-pushed the add_protocol_error_exception_dict_retry_create branch from 7c8381f to d4b0b69 Compare November 23, 2025 11:33

redhat-qe-bot removed needs-rebase verified labels Nov 23, 2025

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

rh-bot-1 added the commented-coderabbitai[bot] label Nov 23, 2025

Add PROTOCOL_ERROR_EXCEPTION_DICT to the default exceptions_dict in c…

d1bf216

…reate()

Ahmad-Hafe force-pushed the add_protocol_error_exception_dict_retry_create branch from d4b0b69 to d1bf216 Compare November 24, 2025 21:04

redhat-qe-bot added the verified label Nov 24, 2025

redhat-qe-bot1 removed verified commented-coderabbitai[bot] labels Nov 24, 2025

coderabbitai bot reviewed Nov 24, 2025

View reviewed changes

redhat-qe-bot1 added the commented-coderabbitai[bot] label Nov 24, 2025

feat: Retry exceptions in create() by adding PROTOCOL_ERROR_EXCEPTION_DICT #2581

Are you sure you want to change the base?

feat: Retry exceptions in create() by adding PROTOCOL_ERROR_EXCEPTION_DICT #2581

Conversation

Ahmad-Hafe commented Nov 17, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Short description:

More details:

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewer:

Bug:

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

redhat-qe-bot commented Nov 17, 2025

Welcome! 🎉

🔄 Automatic Actions

📋 Available Commands

PR Status Management

Review & Approval

Testing & Validation

Container Operations

Cherry-pick Operations

Label Management

✅ Merge Requirements

📊 Review Process

💡 Tips

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rnetser commented Nov 17, 2025

Uh oh!

myakove commented Nov 17, 2025

Uh oh!

Ahmad-Hafe commented Nov 17, 2025

Uh oh!

redhat-qe-bot1 commented Nov 17, 2025

Uh oh!

Ahmad-Hafe commented Nov 18, 2025

Uh oh!

redhat-qe-bot commented Nov 18, 2025

Uh oh!

Ahmad-Hafe commented Nov 18, 2025 • edited by myakove Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myakove commented Nov 18, 2025

Uh oh!

Ahmad-Hafe commented Nov 18, 2025 • edited by myakove Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myakove commented Nov 18, 2025

Uh oh!

Ahmad-Hafe commented Nov 19, 2025

Uh oh!

Ahmad-Hafe commented Nov 19, 2025

Uh oh!

redhat-qe-bot2 commented Nov 19, 2025

Uh oh!

Ahmad-Hafe commented Nov 19, 2025

Uh oh!

myakove commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ahmad-Hafe commented Nov 19, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Ahmad-Hafe commented Nov 17, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 17, 2025 •

edited

Loading

Ahmad-Hafe commented Nov 18, 2025 •

edited by myakove

Loading

Ahmad-Hafe commented Nov 18, 2025 •

edited by myakove

Loading

myakove commented Nov 19, 2025 •

edited

Loading