feat: enhance wait_for_ready() on policies. #847
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
crstrn13
force-pushed
the
envoy_wait_mixin
branch
2 times, most recently
from
80700f6 to
24acf6d
Compare
crstrn13
force-pushed
the
envoy_wait_mixin
branch
from
24acf6d to
ddc603e
Compare
crstrn13
force-pushed
the
envoy_wait_mixin
branch
from
c3df06a to
4f11377
Compare
azgabur
reviewed
Jan 20, 2026
| if not kuadrant: | ||
| return None | ||
|
|
||
| route = OpenshiftRoute.create_instance( |
Contributor
There was a problem hiding this comment.
Using OpenshiftRoute for every gateway will make the testsuite incompatible with Kind/Kubernetes.
Contributor
Author
There was a problem hiding this comment.
you are right. I should use the exposer to expose that.
crstrn13
force-pushed
the
envoy_wait_mixin
branch
2 times, most recently
from
e304df5 to
5c2b364
Compare
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
crstrn13
force-pushed
the
envoy_wait_mixin
branch
from
9304fb3 to
2325864
Compare
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Contributor
Author
|
/make smoke |
|
Test run completed ( Short Test SummaryFull Output |
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Contributor
|
/make smoke |
|
Test run completed ( Short Test SummaryFull Output |
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
azgabur
reviewed
Jan 27, 2026
Contributor
azgabur
left a comment
azgabur
left a comment
There was a problem hiding this comment.
This PR has a lot of strange refactors I do not understand why are needed to achieve the PR's goal.
The suggested way to extend Policy class to add the wait for kuadrant_config update seems bit hack-y to me. The metrics are exposed in Gateway, so I recommend moving the code which handles the update check there. The Policies which are being updated/applied could have reference to their gateway stored as a field (similar how they store "cluster") and use it inside commit (to get current kuadrant_config generation number) and wait_for_* methods (to check the number was incremented).
I can see the problem in which we do commit fixture. In some situations (for example testsuite/tests/multicluster/coredns/two_clusters/conftest.py) there are multiple policies commited before their wait_for_ready methods are called. Meaning you cant expect that .commit() and .wait_for_ready() methods will be called right after each other so the generation field could be incremented by something else then the current policy waiting.
I do not know how the kuadrant_config structure looks like, but if it contains field about generation of specific policy being applied, you could use generation field of the policy similar how the current wait_for_ready works in Policy to wait until the gateway has the field in the metric.
In any case, this check seems like something that could be done on the Operator side and used to set the Ready/Enforced status more correctly, I would suggest that as a issue and have the implementation of such check in testsuite only if they reject implementing it.
Contributor
There was a problem hiding this comment.
Why rename name -> _name ? And create duplicate name property?
| """Returns TLS cert bound to this Gateway, if the Gateway does not use TLS, returns None""" | ||
|
|
||
| @abstractmethod | ||
| def name(self) -> str: |
Contributor
There was a problem hiding this comment.
There is already service_name
| class PlanPolicy(Policy): | ||
| """PlanPolicy object, used for applying plan-based policies to a Gateway/HTTPRoute""" | ||
|
|
||
| def __init__(self, *args, **kwargs): |
Contributor
There was a problem hiding this comment.
Unrelated refactor as well as in testsuite/kuadrant/extensions/telemetry_policy.py
| def gateway(request, authorino, cluster, blame, module_label, testconfig, keycloak): | ||
| """Deploys Envoy with additional JWT plain identity test setup""" | ||
| envoy = JwtEnvoy( | ||
| envoy = JwtEnvoy( # pylint: disable=abstract-class-instantiated |
Contributor
There was a problem hiding this comment.
JwtEnvoy is not abstract class, why is this disable here?
| self._hostname = hostname | ||
| self.verify = verify | ||
| self.ip_getter = ip_getter | ||
| self.verify = verify |
Contributor
Author
There was a problem hiding this comment.
it wasn't matching the parameters order.
|
|
||
| @pytest.fixture(scope="module", autouse=True) | ||
| def commit(request, commit, authorization2): # pylint: disable=unused-argument | ||
| def commit(request, authorization2): # pylint: disable=unused-argument |
Contributor
There was a problem hiding this comment.
Why remove parent commit fixture here?
| def gateway(request, authorino, cluster, blame, label, testconfig) -> Envoy: | ||
| """Deploys Envoy that wires up the Backend behind the reverse-proxy and Authorino instance""" | ||
| gw = Envoy( | ||
| gw = Envoy( # pylint: disable=abstract-class-instantiated |
Contributor
There was a problem hiding this comment.
Envoy is not abstract class, why is this disable here?
|
|
||
| @pytest.fixture(scope="module", autouse=True) | ||
| def commit(request, commit, wristband_authorization): # pylint: disable=unused-argument | ||
| def commit(request, wristband_authorization): # pylint: disable=unused-argument |
Contributor
There was a problem hiding this comment.
Why remove parent commit fixture here?
| metrics_service.commit() | ||
| metrics_service.wait_for_ready() | ||
|
|
||
| return exposer.expose_hostname(blame("metrics"), metrics_service) |
Contributor
There was a problem hiding this comment.
You dont need to create whole class MetricsServiceGateway just to run exposer on it. In the worst case this will create loadbalanced service targeting loadbalanced service doubling infrastructure strain which now mostly causes the sporadic failures.
I suggest using the Gateway's loadbalanced service which already exists to expose the port you need (15020) for the metrics. See https://istio.io/latest/docs/tasks/observability/metrics/secure-metrics/#secure-metrics-for-gateways
This will avoid whole exposing hassle.
| hostname, | ||
| backend, | ||
| module_label, | ||
| gateway_metrics_service, # pylint: disable=unused-argument |
Contributor
There was a problem hiding this comment.
Why add it, if its not used
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR introduces EnvoyWaitMixin which leverages the kuadrant_configs metric exposed by the WASM shim. This metric increments each time the WASM
configuration changes, providing a reliable indicator of actual data plane readiness.
Two-phase wait mechanism:
Changes
Closes: #846