Add configurable API Timeouts #463
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fao89 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
fao89
requested review from
rabi and
bshephar
and removed request for
olliewalsh and
viroel
| annotations[heatAnno] = timeout | ||
| annotations[haProxyAnno] = timeout |
There was a problem hiding this comment.
How are these being used? This function isn't called from anywhere that I can see, and it doesn't return anything. Should we be taking in a pointer to an annotations map instead?
There was a problem hiding this comment.
I'm following the pattern of calling it from openstack-operator webhook:
- https://github.com/openstack-k8s-operators/manila-operator/blob/main/api/v1beta1/manila_webhook.go#L241
- https://github.com/openstack-k8s-operators/openstack-operator/blob/main/apis/core/v1beta1/openstackcontrolplane_webhook.go#L783
- https://github.com/search?q=org%3Aopenstack-k8s-operators%20SetDefaultRouteAnnotations&type=code
after this PR gets merged, I'll open a PR against openstack-operator
There was a problem hiding this comment.
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
| @@ -7,6 +7,8 @@ num_engine_workers=4 | |||
| auth_encryption_key={{ .AuthEncryptionKey }} | |||
| use_stderr=true | |||
| transport_url={{ .TransportURL }} | |||
| # Keep the RPC call timeout in sync with HAProxy and Apache timeouts | |||
There was a problem hiding this comment.
I don't think rpc_response_timeout default of 60 would work for heat. It's 600 in 17.1 along with apache/httpd timeout. Btw where is haproxy in the picture?
There was a problem hiding this comment.
There was a problem hiding this comment.
right, should we mention the route timeout instead of haproxy though it translates to that?
There was a problem hiding this comment.
HAProxy as in the one used for OpenShift ingress. So we pass an annotation to the Route to configure HAProxy timeout on the OpenShift Route.
Does it make sense to group RPC with API? Feels like a loose relationship at best.
There was a problem hiding this comment.
from OSPRH-10954, I tried to follow what manila, cinder, glance did. And I saw that both manila and cinder grouped it:
- https://github.com/openstack-k8s-operators/manila-operator/blob/57ac502c086d0bb2092e124d3152ab9e31a4ad79/templates/manila/config/00-config.conf#L13
- https://github.com/openstack-k8s-operators/cinder-operator/blob/980b1ab26c714bbc0b7f4925517c68f8d3e53169/templates/cinder/config/00-global-defaults.conf#L32
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
controllers/heat_controller.go
Outdated
| @@ -1353,6 +1353,7 @@ func initTemplateParameters( | |||
| "MemcachedServersWithInet": mc.GetMemcachedServerListWithInetString(), | |||
| "MemcachedTLS": mc.GetMemcachedTLSSupport(), | |||
| "DatabaseConnection": mysqlConnectionString, | |||
| "TimeOut": instance.Spec.APITimeout, | |||
This patch adds an apiTimeout field to the HeatSpecBase to allow human operators to simultaneously configure the timeouts for HAProxy, Apache, and the rpc_response_timeout. The apiTimeout defaults to 60 seconds, to mimic the behavior present in OSP17. Having different timeouts for HAProxy, Apache, and rpc_response_timeout is possible setting the HAProxy timeout in the apiOverride, the Apache timeout with apiTimeout, and setting rpc_response_timeout in the top level Heat customServiceConfig. To be able to change the HAProxy value based on the apiTimeout with any update (and not just the first time) the code adds a custom annotation "api.heat.openstack.org/timeout" with the value that was initially set, this way flags it as being set by the heat-operator. closes OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
openshift-merge-bot
bot
merged commit bd9afac
into
openstack-k8s-operators:main
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
Depends-On: openstack-k8s-operators/heat-operator#463 ref OSPRH-10965 Signed-off-by: Fabricio Aguiar <[email protected]>
This patch adds an apiTimeout field to the HeatSpecBase to allow human operators to simultaneously configure the timeouts for HAProxy, Apache, and the rpc_response_timeout.
The apiTimeout defaults to 60 seconds, to mimic the behavior present in OSP17.
Having different timeouts for HAProxy, Apache, and rpc_response_timeout is possible setting the HAProxy timeout in the apiOverride, the Apache timeout with apiTimeout, and setting rpc_response_timeout in the top level Heat customServiceConfig.
To be able to change the HAProxy value based on the apiTimeout with any update (and not just the first time) the code adds a custom annotation "api.heat.openstack.org/timeout" with the value that was initially set, this way flags it as being set by the heat-operator.
closes OSPRH-10965