Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a parameter 'apiTimeout' to allow customization #270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+44 −0
Open

Added a parameter 'apiTimeout' to allow customization #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/bases/placement.openstack.org_placementapis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ spec:
spec:
description: PlacementAPISpec defines the desired state of PlacementAPI
properties:
apiTimeout:
default: 60
description: APITimeout for HAProxy, Apache
minimum: 10
type: integer
containerImage:
description: PlacementAPI Container Image URL (will be set to environmental
default if empty)
Expand Down
6 changes: 6 additions & 0 deletions api/v1beta1/placementapi_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ type PlacementAPISpec struct {

// PlacementAPISpecCore -
type PlacementAPISpecCore struct {
// +kubebuilder:validation:Optional
// +kubebuilder:default=60
// +kubebuilder:validation:Minimum=10
// APITimeout for HAProxy, Apache
APITimeout int `json:"apiTimeout"`

// +kubebuilder:validation:Optional
// +kubebuilder:default=placement
// ServiceUser - optional username used for this service to register in keystone
Expand Down
23 changes: 23 additions & 0 deletions api/v1beta1/placementapi_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,3 +175,26 @@ func ValidateDefaultConfigOverwrite(
}
return errors
}

// SetDefaultRouteAnnotations sets HAProxy timeout values of the route
func (spec *PlacementAPISpecCore) SetDefaultRouteAnnotations(annotations map[string]string) {
const haProxyAnno = "haproxy.router.openshift.io/timeout"
// Use a custom annotation to flag when the operator has set the default HAProxy timeout
// With the annotation func determines when to overwrite existing HAProxy timeout with the APITimeout
const placementAnno = "api.placement.openstack.org/timeout"
valPlacementAPI, okPlacementAPI := annotations[placementAnno]
valHAProxy, okHAProxy := annotations[haProxyAnno]
// Human operator set the HAProxy timeout manually
if !okPlacementAPI && okHAProxy {
return
}
// Human operator modified the HAProxy timeout manually without removing the Placemen flag
if okPlacementAPI && okHAProxy && valPlacementAPI != valHAProxy {
delete(annotations, placementAnno)
mrkisaolamb marked this conversation as resolved.
Show resolved Hide resolved
placementapilog.Info("Human operator modified the HAProxy timeout manually without removing the Placement flag. Deleting the Placement flag to ensure proper configuration.")
return
}
timeout := fmt.Sprintf("%ds", spec.APITimeout)
annotations[placementAnno] = timeout
annotations[haProxyAnno] = timeout
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stuggi stuggi Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up works for me

5 changes: 5 additions & 0 deletions config/crd/bases/placement.openstack.org_placementapis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ spec:
spec:
description: PlacementAPISpec defines the desired state of PlacementAPI
properties:
apiTimeout:
default: 60
description: APITimeout for HAProxy, Apache
minimum: 10
type: integer
containerImage:
description: PlacementAPI Container Image URL (will be set to environmental
default if empty)
Expand Down
1 change: 1 addition & 0 deletions controllers/placementapi_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,7 @@ func (r *PlacementAPIReconciler) generateServiceConfigMaps(
httpdVhostConfig[endpt.String()] = endptConfig
}
templateParameters["VHosts"] = httpdVhostConfig
templateParameters["TimeOut"] = instance.Spec.APITimeout
bogdando marked this conversation as resolved.
Show resolved Hide resolved

extraTemplates := map[string]string{
"placement.conf": "placementapi/config/placement.conf",
Expand Down
1 change: 1 addition & 0 deletions templates/placementapi/config/httpd.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ CustomLog /dev/stdout proxy env=forwarded
ErrorLogFormat "%M"
</IfVersion>
ServerName {{ $vhost.ServerName }}
TimeOut {{ $.TimeOut }}

## Vhost docroot
ErrorLog /dev/stdout
Expand Down
3 changes: 3 additions & 0 deletions tests/functional/placementapi_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


It("creates service account, role and rolebindig", func() {
Expand Down
Loading