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

Add parameter to customize the number of cinderAPI processes #474

Closed
wants to merge 1 commit into from
Closed

Add parameter to customize the number of cinderAPI processes #474

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
8 changes: 8 additions & 0 deletions api/bases/cinder.openstack.org_cinderapis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,14 @@ spec:
- extraVol
type: object
type: array
httpdCustomization:
properties:
processNumber:
default: 4
format: int32
minimum: 1
type: integer
type: object
networkAttachments:
items:
type: string
Expand Down
8 changes: 8 additions & 0 deletions api/bases/cinder.openstack.org_cinders.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ spec:
items:
type: string
type: array
httpdCustomization:
properties:
processNumber:
default: 4
format: int32
minimum: 1
type: integer
type: object
networkAttachments:
items:
type: string
Expand Down
13 changes: 13 additions & 0 deletions api/v1beta1/cinderapi_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,19 @@ type CinderAPITemplate struct {
ContainerImage string `json:"containerImage"`

CinderAPITemplateCore `json:",inline"`

// +kubebuilder:validation:Optional
// HttpdCustomization - customize the httpd service
HttpdCustomization HttpdCustomization `json:"httpdCustomization,omitempty"`
}

// HttpdCustomization -
type HttpdCustomization struct {
// +kubebuilder:validation:Optional
// +kubebuilder:default=4
// +kubebuilder:validation:Minimum=1
// ProcessNumber - Number of processes running in Cinder API
ProcessNumber *int32 `json:"processNumber"`
Copy link
Contributor

Choose a reason for hiding this comment

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

At least in nova-operator we try to keep the replica as the unit of scaling of a service with a hardcoded process number. What is the case where a replica based scaling is not a good solution for cinder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were discussions on architectural aspects where you can't scale replicas (because of scheduling constraints), but you still want to tweak this parameter on the same API.
My initial understanding was pretty much the same as yours: use replicas as it represents a building block to scale the API, but to reach the same number of process with a smaller amount of replicas you might need to tune this value [1].

[1] https://issues.redhat.com/browse/OSPRH-10363

Copy link
Contributor

Choose a reason for hiding this comment

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

There were discussions on architectural aspects where you can't scale replicas (because of scheduling constraints), but you still want to tweak this parameter on the same API.

What are the scheduling constraints that prevents us having many replicas? I believe our anti affinity is soft, so it only tries to spread the pods across worker nodes but does not fail if more pods needs to land on the same worker. I looked at [1] but I don't see the actual scheduling problem described there.

Copy link
Contributor

Choose a reason for hiding this comment

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

moreover if we open up the process config for the user then it is easy to make a nonsensical config with a high process count but a low cpu resource request. I still think that having a hardcoded process count helps defining a meaningful resource request for the our pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. We don't have a way to keep all these parameters consistent and still scale in a way where we keep track of the resource consumption. I'm reconsidering these set of patches because they're not probably solving a problem that is already addressed at k8s level. I see we have a default number of processes: 4. Do you know if there are better defaults or did you do anything in Nova that can lead us to choose better numbers (if so, which is the metric used in this context?)

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good point. We don't have a way to keep all these parameters consistent and still scale in a way where we keep track of the resource consumption. I'm reconsidering these set of patches because they're not probably solving a problem that is already addressed at k8s level. I see we have a default number of processes: 4. Do you know if there are better defaults or did you do anything in Nova that can lead us to choose better numbers (if so, which is the metric used in this context?)

In nova-operator we ended up having processes=2 [1] based on the tempest results and troubleshooting of some probe timeouts. I think 4 is equally valid if you see that as a good fit for the steps of scaling. Probably we will gain more insight from the field especially for the cpu and memory resource request. Like I'm just fixing and performance measuring a placement-api timeout / memory consumption spike upstream so I have data that an idle placement-api process needs 100MB but during an allocation candidate query it spikes up to 650MB easily while pegging 1 CPU core. Probably similar data is needed for each service to set up a nice defaults.

[1]https://github.com/openstack-k8s-operators/nova-operator/blob/main/templates/novaapi/config/httpd.conf#L76

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for sharing. We didn't conduct any of this exercise and this is a prerequisite to learn more about Resource consumption and tuning the default parameters. I don't see a particular problem that comes from tempest with processes=4 as a default, but worth investigating for the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think 4 is fine. I would advise against 1 or bigger 4 though. Or maybe even bigger than 4 is OK if the resource request also set accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Deydra71 since you added this to keystone, was there a reason for keystone to be able to customize the processes, instead if bumping the replicas?

Copy link

Choose a reason for hiding this comment

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

@stuggi
Basically just the Jira conversation -->
https://issues.redhat.com/browse/OSPRH-10363

}

// APIOverrideSpec to override the generated manifest of several child resources.
Expand Down
21 changes: 21 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions config/crd/bases/cinder.openstack.org_cinderapis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,14 @@ spec:
- extraVol
type: object
type: array
httpdCustomization:
properties:
processNumber:
default: 4
format: int32
minimum: 1
type: integer
type: object
networkAttachments:
items:
type: string
Expand Down
8 changes: 8 additions & 0 deletions config/crd/bases/cinder.openstack.org_cinders.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ spec:
items:
type: string
type: array
httpdCustomization:
properties:
processNumber:
default: 4
format: int32
minimum: 1
type: integer
type: object
networkAttachments:
items:
type: string
Expand Down
1 change: 1 addition & 0 deletions controllers/cinder_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,7 @@ func (r *CinderReconciler) generateServiceConfigs(
cinder.DatabaseName)
templateParameters["MemcachedServersWithInet"] = memcached.GetMemcachedServerListWithInetString()
templateParameters["TimeOut"] = instance.Spec.APITimeout
templateParameters["Workers"] = instance.Spec.CinderAPI.HttpdCustomization.ProcessNumber

// create httpd vhost template parameters
httpdVhostConfig := map[string]interface{}{}
Expand Down
2 changes: 1 addition & 1 deletion templates/cinder/config/10-cinder_wsgi.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

## WSGI configuration
WSGIApplicationGroup %{GLOBAL}
WSGIDaemonProcess {{ $endpt }} display-name={{ $endpt }} group=cinder processes=4 threads=1 user=cinder
WSGIDaemonProcess {{ $endpt }} display-name={{ $endpt }} group=cinder processes={{ $.Workers }} threads=1 user=cinder
WSGIProcessGroup {{ $endpt }}
WSGIScriptAlias / "/var/www/cgi-bin/cinder/cinder-wsgi"
WSGIPassAuthorization On
Expand Down
Loading