Added a parameter 'apiTimeout' to allow customization #270

mrkisaolamb · 2024-11-26T08:30:53Z

to the Apache timeout.

The default is set to 60s which we do set for
HAProxy timeouts currently.

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.placement.openstack.org/timeout" with the value that was initially set, this way flags it as being set by the placement-operator.

Timeout is global for both api and metadata and they are set from placement lvl

There will be follow up patch in openstack-operator to utilize the method 'SetDefaultRouteAnnotations' to set these default route annotations in openstack-operator

resolve: https://issues.redhat.com/browse/OSPRH-10962

openshift-ci · 2024-11-26T08:31:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrkisaolamb

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:

~~OWNERS~~ [mrkisaolamb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Depends-On: openstack-k8s-operators/placement-operator#270 Depends-On: openstack-k8s-operators/nova-operator#891 ref OSPRH-10962 ref OSPRH-10955

controllers/placementapi_controller.go

auniyal61 · 2024-12-05T12:07:49Z

tests/functional/placementapi_controller_test.go

@@ -344,6 +344,9 @@ var _ = Describe("PlacementAPI controller", func() {
 			myCnf := cm.Data["my.cnf"]
 			Expect(myCnf).To(
 				ContainSubstring("[client]\nssl=0"))
+			configData := cm.Data["httpd.conf"]
+			Expect(configData).Should(
+				ContainSubstring("TimeOut 60"))


gibizer · 2024-12-06T12:05:29Z

api/v1beta1/placementapi_webhook.go

+	timeout := fmt.Sprintf("%ds", spec.APITimeout)
+	annotations[placementAnno] = timeout
+	annotations[haProxyAnno] = timeout
+}


This feels sensible. Thanks for taking care of the possible conflicts.

I'm wondering if the other operators using the same logic. At least they should. And then This function can be abstracted to lib-common as it only need the annotations param and a service specific annotation param but it does not depend on the spec at all.

iirc, neutron was the first one adding this https://github.com/openstack-k8s-operators/neutron-operator/blob/main/api/v1beta1/neutronapi_webhook.go. we have functional tests as part of the openstack-operaotr https://github.com/openstack-k8s-operators/openstack-operator/blob/main/tests/functional/ctlplane/openstackoperator_controller_test.go#L855

I'm wondering if the other operators using the same logic. At least they should. And then This function can be abstracted to lib-common as it only need the annotations param and a service specific annotation param but it does not depend on the spec at all.

yes all operators use same logic so it can be move to lib-common but maybe as a follow up?

follow up works for me

gibizer · 2024-12-06T12:07:13Z

tests/functional/placementapi_controller_test.go

@@ -344,6 +344,9 @@ var _ = Describe("PlacementAPI controller", func() {
 			myCnf := cm.Data["my.cnf"]
 			Expect(myCnf).To(
 				ContainSubstring("[client]\nssl=0"))
+			configData := cm.Data["httpd.conf"]
+			Expect(configData).Should(
+				ContainSubstring("TimeOut 60"))
 		})


Do we have a way to test the annotations on the Route? I guess not in functional env as the Route is only created by the openstack-operator but maybe in the kuttl env?

At least we need to extend https://github.com/openstack-k8s-operators/openstack-operator/blob/main/tests/functional/ctlplane/openstackoperator_controller_test.go#L855 when we integrate this patch into the openstack-operator.

sure, done https://github.com/openstack-k8s-operators/openstack-operator/pull/1220/files

api/v1beta1/placementapi_webhook.go

bogdando · 2024-12-06T12:41:33Z

api/v1beta1/placementapi_webhook.go

+	valPlacementAPI, okPlacemenAPI := annotations[placementAnno]
+	valHAProxy, okHAProxy := annotations[haProxyAnno]
+	// Human operator set the HAProxy timeout manually
+	if !okPlacemenAPI && okHAProxy {


please log why the annotation wasn't updated

not sure if this is good idea to log that nothing happend, this sounds like something that make only noise in logs

to the Apache timeout. The default is set to 60s which we do set for HAProxy timeouts currently. 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.placement.openstack.org/timeout" with the value that was initially set, this way flags it as being set by the placement-operator. There will be follow up patch in openstack-operator to utilize the method 'SetDefaultRouteAnnotations' to set these default route annotations in openstack-operator

gibizer

I have a testing question still open. Do you have an openstack-operator patch integrating this change into there?

gibizer · 2024-12-09T15:43:47Z

api/v1beta1/placementapi_webhook.go

+	timeout := fmt.Sprintf("%ds", spec.APITimeout)
+	annotations[placementAnno] = timeout
+	annotations[haProxyAnno] = timeout
+}


follow up works for me

controllers/placementapi_controller.go

gibizer · 2024-12-09T15:48:47Z

tests/functional/placementapi_controller_test.go

@@ -344,6 +344,9 @@ var _ = Describe("PlacementAPI controller", func() {
 			myCnf := cm.Data["my.cnf"]
 			Expect(myCnf).To(
 				ContainSubstring("[client]\nssl=0"))
+			configData := cm.Data["httpd.conf"]
+			Expect(configData).Should(
+				ContainSubstring("TimeOut 60"))
 		})


At least we need to extend https://github.com/openstack-k8s-operators/openstack-operator/blob/main/tests/functional/ctlplane/openstackoperator_controller_test.go#L855 when we integrate this patch into the openstack-operator.

Depends-On: openstack-k8s-operators/placement-operator#270 Depends-On: openstack-k8s-operators/nova-operator#891 ref OSPRH-10962 ref OSPRH-10955

openshift-ci bot requested review from kk7ds and olliewalsh November 26, 2024 08:31

openshift-ci bot added the approved label Nov 26, 2024

mrkisaolamb added a commit to mrkisaolamb/openstack-operator that referenced this pull request Dec 3, 2024

Set Nova and Placement timeout

7e63459

Depends-On: openstack-k8s-operators/placement-operator#270 Depends-On: openstack-k8s-operators/nova-operator#891 ref OSPRH-10962 ref OSPRH-10955

mrkisaolamb mentioned this pull request Dec 3, 2024

WIP: Set Nova and Placement timeout openstack-k8s-operators/openstack-operator#1220

Open

mrkisaolamb force-pushed the timeout branch from 492ef25 to d9520b3 Compare December 4, 2024 10:07

mrkisaolamb requested review from gibizer and bogdando December 4, 2024 10:07

auniyal61 reviewed Dec 5, 2024

View reviewed changes

controllers/placementapi_controller.go Show resolved Hide resolved

auniyal61 reviewed Dec 5, 2024

View reviewed changes

gibizer reviewed Dec 6, 2024

View reviewed changes

Deydra71 reviewed Dec 6, 2024

View reviewed changes

api/v1beta1/placementapi_webhook.go Outdated Show resolved Hide resolved

bogdando reviewed Dec 6, 2024

View reviewed changes

api/v1beta1/placementapi_webhook.go Show resolved Hide resolved

bogdando reviewed Dec 6, 2024

View reviewed changes

mrkisaolamb force-pushed the timeout branch from d9520b3 to 7c6d621 Compare December 9, 2024 10:37

gibizer reviewed Dec 9, 2024

View reviewed changes

mrkisaolamb added a commit to mrkisaolamb/openstack-operator that referenced this pull request Dec 11, 2024

Set Nova and Placement timeout

5efb530

Depends-On: openstack-k8s-operators/placement-operator#270 Depends-On: openstack-k8s-operators/nova-operator#891 ref OSPRH-10962 ref OSPRH-10955

mrkisaolamb requested review from gibizer and bogdando December 11, 2024 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a parameter 'apiTimeout' to allow customization #270

Added a parameter 'apiTimeout' to allow customization #270

mrkisaolamb commented Nov 26, 2024

openshift-ci bot commented Nov 26, 2024

auniyal61 Dec 5, 2024

gibizer Dec 6, 2024

gibizer Dec 6, 2024

stuggi Dec 6, 2024 •

edited

Loading

mrkisaolamb Dec 9, 2024

gibizer Dec 9, 2024

gibizer Dec 6, 2024

gibizer Dec 9, 2024

mrkisaolamb Dec 11, 2024

bogdando Dec 6, 2024

mrkisaolamb Dec 9, 2024

gibizer left a comment

gibizer Dec 9, 2024

gibizer Dec 9, 2024

Added a parameter 'apiTimeout' to allow customization #270

Are you sure you want to change the base?

Added a parameter 'apiTimeout' to allow customization #270

Conversation

mrkisaolamb commented Nov 26, 2024

openshift-ci bot commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuggi Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuggi Dec 6, 2024 •

edited

Loading