Fixes #37081 - Add a setting to configure PAGE_SIZE

[wbclark]

No description provided.

[evgeni]

ACK pending template and tests agree ;)

[ekohl]

A small note on why this is useful would be nice. Though I also wonder if Pulp itself should increase the page size where needed. https://www.django-rest-framework.org/api-guide/pagination/ describes you can also do this for specific views. It also describes a mechanism similar to what Foreman has that allows you to request bigger pages. Has any effort been taken to use that route? It would give a better out of the experience for all users, not just those who tune the installer.

[wbclark]

A small note on why this is useful would be nice. Though I also wonder if Pulp itself should increase the page size where needed. https://www.django-rest-framework.org/api-guide/pagination/ describes you can also do this for specific views. It also describes a mechanism similar to what Foreman has that allows you to request bigger pages. Has any effort been taken to use that route? It would give a better out of the experience for all users, not just those who tune the installer.

Most users probably don't need to tune this. In some cases, it was helpful as a workaround for other issues, such as one issue that occurred when using Katello to provide content for provisioning OpenStack. In examples like that, the end user couldn't simply request bigger pages because they are using an install script from another project. Having a setting here allows it to be configured persistently so that they aren't blocked when these types of issues occur.

[ehelms]

I am curious why this is what I would call a system configuration change. The ability to change this value due to performance issues feels like it should be at the application level to prevent a service restart.

[ekohl]

Most users probably don't need to tune this. In some cases, it was helpful as a workaround for other issues, such as one issue that occurred when using Katello to provide content for provisioning OpenStack. In examples like that, the end user couldn't simply request bigger pages because they are using an install script from another project. Having a setting here allows it to be configured persistently so that they aren't blocked when these types of issues occur.

This is exactly the kind of information that should be presented in a commit message. Then reviewers don't need to guess why it's useful. And anyone reading the source can also retrieve it with git blame.

But still, even if the script today may be limited, it can be worthwhile to still start the conversation. Perhaps it can be improved in the future. And as you may have seen, you can tune individual views so Pulp can still increase the default page size for some views if that's beneficial. If the script has implemented paging correctly, they will automatically benefit from larger pages without being corrected. And if it doesn't page correctly, this change would also break it.

[wbclark]

I am curious why this is what I would call a system configuration change. The ability to change this value due to performance issues feels like it should be at the application level to prevent a service restart.

It turns out that Django settings are designed to be immutable.

https://docs.djangoproject.com/en/5.0/topics/settings/#altering-settings-at-runtime

[wbclark]

Sorry, didn't mean to close this -- wrong keyboard shortcut.

[wbclark]

But still, even if the script today may be limited, it can be worthwhile to still start the conversation. Perhaps it can be improved in the future. And as you may have seen, you can tune individual views so Pulp can still increase the default page size for some views if that's beneficial. If the script has implemented paging correctly, they will automatically benefit from larger pages without being corrected. And if it doesn't page correctly, this change would also break it.

Indeed, the OpenStack bug in this case was https://bugs.launchpad.net/tripleo/+bug/2028260

[wbclark]

This is exactly the kind of information that should be presented in a commit message. Then reviewers don't need to guess why it's useful. And anyone reading the source can also retrieve it with git blame.

I added the following explanation to the commit message so that interested readers can understand what this is about without chasing the rabbit through Redmine and various Bugzillas:

    This provides an option for users to configure the PAGE_SIZE
    setting in Django REST Framework as used by Pulpcore. Pulp has
    a default value for PAGE_SIZE but it could be overridden in the
    user's settings.py file. Pulp paginates views when listing
    repository contents, package metadata, container tags, etc. It
    doesn't affect delivery of content blobs, and most deployments
    in normal circumstances probably don't need to tune it.
    
    This setting was helpful as a workaround for an external issue that
    occurred when provisioning TripleO (OpenStack on OpenStack) with
    container content provided by Katello. In that case, the TripleO
    installation scripts didn't yet support pagination.

[wbclark]

@ekohl if you have a moment, mind taking a fresh look?

[ekohl]

Indeed, the OpenStack bug in this case was https://bugs.launchpad.net/tripleo/+bug/2028260

If it doesn't do any pagination then adding a page size is never going to work reliably. When you increase the page size to 200 and there are 201 tags, you will miss one.

Since it isn't performance and this isn't solving it I'm leaning to rejecting this PR.

[ekohl]

I prefer keeping conditions simple

Suggested change

	<% unless scope['pulpcore::page_size'].nil? -%>
	<% if scope['pulpcore::page_size'] -%>

[wbclark]

Well, I made the choice to write it this way so that readers know that it's the presence/absence of the value that we care about, not whether the value is truthy/falsy.

[ianballou]

It's certainly hacky that a workaround for the OpenStack bug is to bump up the Pulpcore page size. However, it's slow for OpenStack to fix their issue, but quick and easy for us to release this configurable option.

OpenStack bug aside, I think other folks could benefit from this setting since it is a Pulp configurable. If we even take this Puppet module out of the scope of Katello, wouldn't it be helpful to have more configurable options? More configurable options would mean more usefulness for people who just want to set up Pulp via Puppet.

[ekohl]

Pulp doesn't document the setting (https://docs.pulpproject.org/pulpcore/configuration/settings.html) so it's unclear if they want to support this at all. Just because Django Rest Framework can do it, doesn't mean the Pulp project supports it.

Increasing page sizes can have certain consequences we can't oversee. For example, certain pages may become slow and lead to time outs.

[ehelms]

@dralley Can you weigh in here to help us out?

[dralley]

I don't consider this setting part of Pulp, it's just there because we use Django REST Framework, and all the settings get merged together.

We aren't very likely to switch away from Django REST Framework soon so it's stable enough to use as a workaround if you want but I would not rely on this long-term. And if you do override it I'd do it in such a way that you'd know if it ever stops working, such as by patching Pulp's own override of this setting

[ekohl]

I don't consider this setting part of Pulp

Based on this and my other concerns I'm closing this.