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.

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

Fixes #33312 - Set Puma tuning values #715

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Aug 19, 2021

This initially sets the Puma tuning values based on the same ideas
from theforeman/puppet-foreman#986 statically
calculated for each of the CPU and memory values that exist for the
various profiles.

This initially sets the Puma tuning values based on the same ideas
from theforeman/puppet-foreman#986 statically
calculated for each of the CPU and memory values that exist for the
various profiles.
Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

Is there is a reason not to omit foreman::foreman_service_puma_threads_min ? If it is omitted, then the module will configure it equal to the maximum by default.

Arguably, including it here does make it easier for the user to understand what they are getting in each case?

@ehelms
Copy link
Member Author

ehelms commented Aug 24, 2021

Is there is a reason not to omit foreman::foreman_service_puma_threads_min ? If it is omitted, then the module will configure it equal to the maximum by default.

Arguably, including it here does make it easier for the user to understand what they are getting in each case?

I looked at it as the tuning profiles are designed to be concrete bases of configuration. Users can vary values on top of that base but the bases are meant to be more tightly controlled. Thus being explicit with the settings seemed in line with that philosophy.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

This doesn't work. Answers are read before built ins (and tuning is considered a built in):

- name: "Kafo Answers"
path: "%{facts.kafo.scenario.answer_file}"
- name: "Built in"
paths:
- "scenario/%{facts.kafo.scenario.id}/family/%{facts.os.family}-%{facts.os.release.major}.yaml"
- "family/%{facts.os.family}-%{facts.os.release.major}.yaml"
- "family/%{facts.os.family}.yaml"
- "security.yaml"
- "tuning/sizes/%{facts.kafo.scenario.custom.tuning}.yaml"
- "tuning/common.yaml"
- "common.yaml"

Hiera is also not used to determine defaults so it will first read the defaults from Puppet itself, store that in the answers and ignore these.

@ehelms
Copy link
Member Author

ehelms commented Aug 25, 2021

This doesn't work. Answers are read before built ins (and tuning is considered a built in):

- name: "Kafo Answers"
path: "%{facts.kafo.scenario.answer_file}"
- name: "Built in"
paths:
- "scenario/%{facts.kafo.scenario.id}/family/%{facts.os.family}-%{facts.os.release.major}.yaml"
- "family/%{facts.os.family}-%{facts.os.release.major}.yaml"
- "family/%{facts.os.family}.yaml"
- "security.yaml"
- "tuning/sizes/%{facts.kafo.scenario.custom.tuning}.yaml"
- "tuning/common.yaml"
- "common.yaml"

Hiera is also not used to determine defaults so it will first read the defaults from Puppet itself, store that in the answers and ignore these.

How does it handle empty values? If the answers file has an empty value for the puma workers entry will that still take precedence over having a value set in the layer below such as this?

@ekohl
Copy link
Member

ekohl commented Aug 25, 2021

I think my information is outdated and I actually solved the limitation:
theforeman/kafo@3b154b5 was my start to making Puppet optional. I just never found the time to actually benefit from it.

It was originally my goal to separate out all scenario defaults so katello-answers.yaml contains nothing but a list of modules with true/false as value. All defaults should be in a separate layer. That way you can run --enable-puppet and benefit from those defaults. So roughly what #717 aims to achieve in the end.

@ekohl
Copy link
Member

ekohl commented Aug 25, 2021

So to clarify: right now I think this does work for fresh installations. However, for runtime switching (i.e., from existing medium to large) it won't change the answers. It'll be needed to write a hook that clears the answer, probably in pre_values but maybe earlier.

@ehelms
Copy link
Member Author

ehelms commented Aug 27, 2021

Could we skip writing empty values to the answer files? Would that solve this problem? Would it create other problems?

@ekohl
Copy link
Member

ekohl commented Aug 27, 2021

I think I found it. It's in this bit here:

if answers[FOREMAN_PROXY_CONTENT] == true || answers[FOREMAN_PROXY_CONTENT].is_a?(Hash)
upgrade
end

@ekohl
Copy link
Member

ekohl commented Aug 27, 2021

Oh, and I also thought about dropping a bunch of migrations instead but that's also a big task that I didn't want to start until I had time to finish it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants