OLS-2077 - Enabling cert rotation tests.#2875
OLS-2077 - Enabling cert rotation tests.#2875sriroopar wants to merge 1 commit intoopenshift:mainfrom
Conversation
sriroopar
force-pushed
the
enabling-cert-rotation-test
branch
from
5b90c28 to
1b1d48a
Compare
|
/retest |
|
/lgtm |
onmete
left a comment
onmete
left a comment
There was a problem hiding this comment.
Two findings from code review — both about failure-mode clarity, not correctness of the happy path.
| ), "Timed out waiting for pod rollout after CA certificate rotation" | ||
|
|
||
| cluster_utils.wait_for_running_pod() | ||
| wait_for_ols(pytest.ols_url, timeout=300, interval=10) |
There was a problem hiding this comment.
The return value of wait_for_ols() is discarded here. If OLS never becomes ready within 300s, the test continues silently and fails at the query step with a misleading error.
Other callers in the codebase (conftest.py, adapt_ols_config.py, data_collector_control.py) all check the return value.
Suggestion:
| wait_for_ols(pytest.ols_url, timeout=300, interval=10) | |
| assert wait_for_ols(pytest.ols_url, timeout=300, interval=10), \ | |
| "OLS did not become ready after CA certificate rotation" |
There was a problem hiding this comment.
Addressed in 8e5000f — return value is now asserted.
| 30, | ||
| _query_succeeds, | ||
| "Retrying query while route stabilizes after CA cert rotation", | ||
| ), f"OLS returned {response.status_code} after CA certificate rotation" |
There was a problem hiding this comment.
If all 6 attempts of _query_succeeds raise exceptions (e.g. ConnectionError), response stays None. The assertion failure message f"OLS returned {response.status_code}..." then raises AttributeError, masking the real failure.
Suggestion:
status = response.status_code if response is not None else "no response (all attempts raised)"
assert retry_until_timeout_or_success(
6,
30,
_query_succeeds,
"Retrying query while route stabilizes after CA cert rotation",
), f"OLS returned {status} after CA certificate rotation"There was a problem hiding this comment.
Addressed in 8e5000f — response is guarded against None in the f-string. Also nice additions: type annotation on response and return type on _query_succeeds.
sriroopar
force-pushed
the
enabling-cert-rotation-test
branch
from
1b1d48a to
8e5000f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: onmete The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
sriroopar
force-pushed
the
enabling-cert-rotation-test
branch
from
8e5000f to
dd50c91
Compare
|
/lgtm |
|
/retest |
|
/hold Revision dd50c91 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest |
|
/test e2e-ols-cluster |
|
@sriroopar: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description
Enabling certificate rotation e2e tests with modifications to adapt with recent changes on pod starts/restarts.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
After enabling and modifying, this test was run across operator HEAD.