-
Notifications
You must be signed in to change notification settings - Fork 21
make features and flavors configurable #188
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
Conversation
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: we also have Pulp plugins and not all are enabled by default (https://github.com/theforeman/puppet-foreman_proxy_content/blob/11cadd50b6cf19baa2edaea44bd6f85e87282fce/manifests/init.pp#L82-L88). Will this plugin naming start to bite us? Or are we going to build different pulp images for that?
Slightly related to theforeman/pulp-oci-images#2 (comment).
|
That's a great question! So far, I've not thought about that, and honestly would prefer to build as few containers as possible. That said, maybe we should stop thinking in plugins and start thinking in features? So users can say "I want content-deb" or "compute-azure" and the rest is handled by the code. |
|
I like the idea of features. Especially if we can extend it to the proxy too. Fewer implementation details is better for users. For Pulp it's mostly the content types. Downsides are larger images (because of code & deps), more code to load (longer startup times, more memory), more DB migrations. Perhaps Pulp understands or can be taught a similar trick to Foreman where we ship the code but don't load it. Certainly simplifies the building process. Image selection is a painful and crude way. |
|
For pulp it could also be storage backends (if we end up having more than "file") |
|
Oh yes, good point |
|
pulp natively supports selecting plugins via |
18a2985 to
ecbf19c
Compare
| - "../../vars/{{ certificate_source }}_certificates.yml" | ||
| vars: | ||
| hammer_ca_certificate: "{{ server_ca_certificate }}" | ||
| hammer_plugins: "{{ plugins | map('replace', 'foreman-tasks', 'foreman_tasks') }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is lower case, except foreman-tasks. Perhaps we should from a foremanctl perspective just consider it lower case and then special case it where needed?
Perhaps even already fix https://github.com/theforeman/foreman-oci-images/blob/220306c20253c7bbb3ae223fbdf5656ab7887c65/images/foreman/Containerfile#L17-L19 before we merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean snake case, I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: I've not forgot this, but plan to address it in the metadata we intend to introduce, making the feature tasks, and hiding the names of the gems somewhere.
1dfe2f8 to
2f3eead
Compare
09ad3d8 to
a05aea3
Compare
| features: | ||
| parameter: --feature | ||
| help: Features to enable in this deployment. | ||
| action: append |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do define a default feature set below:
features:
- foreman-tasks
- foreman_remote_execution
- katello
- content/container
- content/rpm
This has the problem that once you pass --feature X that default set is overridden completely. So if you want to change the default, you need to know the default to alter it correctly.
We could introduce --additional-feature or something, but that also feels weird, and still doesn't allow to remove things from the default set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I see the list of features as a user? And will this verify that whatever I pass in is a valid feature to enable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now not at all, and it's not validated (but also doesn't hurt in the "additional wrong plugin" case as if a plugin is not installed it also won't be enabled -- obviously hurts if you're missing a plugin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our current installer we had a similar problem and never solved it well.
I wonder if --feature +foreman-tasks is a reasonable syntax to add a value to the current list. It may end up weird if you repeat it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about having --additional-feature <X>, which would be IMHO better readable, but behave the same as your suggestion, but then was stuck with "but what's the baseline for additional?"
Like, if the baseline is "Foreman", then --feature already does that, done. If the baseline is Katello, then we also need --remove-feature for people who don't want Katello. And boom we're back to having scenarios again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ability to remove a feature seems like it should be dependent upon whether said feature supports that or not. I would be in favor of maintaining that list more explicitly:
- features that can be enabled
- features that can be disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It touches on how we ship Foreman. If we default to Katello then the UX to install plain Foreman becomes foremanctl deploy --remove-feature katello. But you can only do that on the very first run before you installed anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems easy -- do not default to Katello :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind that, but if we then extend it to our documentation: do we completely drop the distinction between running Foreman on EL and with Katello? I wouldn't mind that (quite the opposite), but it has a lot of implications. It's basically finally a huge step towards making Katello more like a regular plugin.
|
@archanaserver @ehelms @ekohl -- I think this is now in a state that works as a PoC and we can start talking about how we want to expose it to the users. Do we have any user stories defined around feature management already? |
|
Not yet, but I thought about it this way in the past. We deliver 3 containers, matching the Foreman repos:
I don't know if this is feasible. |
|
On my phone so linking us very hard, but in https://github.com/theforeman/foreman/blob/develop/config/application.rb we have |
|
building multiple plugins (and layering them) should be absolutely feasible, but seems more like an implementation detail? like, you can totally run a core-only install with the katello container (and all plugins disabled) if you configure it properly, but the way how you expose this configuration is what's interesting here? |
|
It certainly is. My idea was that every container has defaults, but can be modified. Previously we talked about writing our own code, but perhaps we can reuse My first aim was to figure out those defaults. One thing I don't know how to solve properly is disabling previously enabled plugins. Question is: what should be done if the user supplied something like First, we would need to detect it at all. I wonder if DB migrations should write a list of enabled plugins. I think puma has a phase you can hook into during loading which might be usable. |
|
I am not sure And yes, removing plugins is hard. If we can detect "bad" removals (those that leave db entries dangling), I'd be in favor of refusing to start without the plugin. |
|
We can probably move to using plain bundler in prod as well. These days bundler_ext doesn't really serve a purpose anymore: we actually want to enforce the versions specified and enforce it at the RPM level. For removal I was thinking about a common remover plugin that you can enhance for each plugin. Not strictly needed at first, but long term it can be useful. The compute resource part is easy to write in a common way. |
|
That'd still require plugins to change their bundler.d file to set a group, right? Poking PluginsRegistry has the benefit that it's a single place to change |
|
Does using a source-container help with any of this design? I know we had previously tabled that but I am leaning more and more towards the value of the source container for rapid feedback, and my own view of the barriers to that being removed. |
| - A feature is an abstract representation of "the deployed system can now do X", usually implemented by enabling a Foreman/Pulp/Hammer plugin (or a collection of these). | ||
| - A flavor is a set of features that are enabled by default and can not be disabled. This is to allow common deployment types like "vanilla foreman", "katello", "satellite" and similar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A flavor is something you deploy first and then add features on top of it? If so, I suggest you switch the order in which you list them (here and elsewhere, like in the PR's description or on line 28).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yepp.
I mainly listed them here in that order as flavor refers to feature, but you (well, my brain, really) can't talk about features without defining them first.
|
If we go with I think I always envisioned as there are clearly two distinct entities: a foreman server or a host for services (think Capsule). And therefore we didn't need flavors, we just just opt for two commands focused on these two baseline entities with features that you can add to each. I think the current approach is fine based on how things work today (e.g. flavors / features) |
| hosts: | ||
| - quadlet | ||
| become: true | ||
| vars_files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./foremanctl deploy --help
[--flavor FLAVOR]Is there a way to list available flavours?[--reset-flavor]How is this going to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| help: Additional features to enable in this deployment. | ||
| action: append_unique | ||
| remove_features: | ||
| parameter: --remove-feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this going to work? For example I'll run:
./foremanctl deploy --add-feature foreman-proxy
./foremanctl deploy --remove-feature foreman-proxy
How does the remove- task know what it has to remove, disable, and update in the DB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today it will just make that feature "unmanaged", like in the old installer.
We can add "cleanup" as a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add "cleanup" as a follow up.
Can you please create an issue or Jira ticket so we can keep track of it? thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| parameter: --add-feature | ||
| help: Additional features to enable in this deployment. | ||
| action: append_unique | ||
| remove_features: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a task for listing available features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also pointed that out in #188 (comment). @evgeni have talked offline and IIRC he wanted to tackle that in a follow up which I'm good with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, LGTM, @evgeni, Can we create an issue for it so we can track it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@evgeni there's a merge conflict. Also wondering if these are the final commits that you intend to get merged. I'd love to see this merged sooner rather than later because other work builds on it. |
|
rebased and squashed. |
|
@evgeni mind rebasing again to resolve merge conflicts? I'd like to merge this so we can build on it. |
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there are any more objections I'd like to merge this tomorrow. We can always refactor, but this is the foundation on which we're going to build various features.
Gauravtalreja1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, with a few non-blocking comments
| - 'foreman-client-key,type=mount,target=/etc/foreman/client_key.pem' | ||
| env: | ||
| FOREMAN_PUMA_WORKERS: "{{ foreman_puma_workers }}" | ||
| FOREMAN_ENABLED_PLUGINS: "{{ foreman_plugins | join(' ') }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, how this env var for foreman_plugins will be used on the containers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugins inside the container have the following snippet in their respective bundler files:
gem '$PLUGIN' if ENV.fetch('FOREMAN_ENABLED_PLUGINS', '').split.include?('$PLUGIN') || ENV.fetch('FOREMAN_ENABLED_PLUGINS', nil).nil?Which essentially means:
Execute gem '$PLUGIN' (thus loading the plugin) if one of the following is true:
FOREMAN_ENABLED_PLUGINSenvironment variable contains the plugin nameFOREMAN_ENABLED_PLUGINSis unset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks
| env: | ||
| DYNFLOW_REDIS_URL: "redis://localhost:6379/6" | ||
| REDIS_PROVIDER: "DYNFLOW_REDIS_URL" | ||
| FOREMAN_ENABLED_PLUGINS: "{{ foreman_plugins | join(' ') }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, is this required on Dynflow containers as well? do we enable any plugins on those containers too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the same code loaded in the dynflow containers, as otherwise plugin-specific tasks wouldn't work correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks
src/vars/base.yaml
Outdated
| foreman_client_certificate: "{{ client_certificate }}" | ||
| foreman_oauth_consumer_key: abcdefghijklmnopqrstuvwxyz123456 | ||
| foreman_oauth_consumer_secret: abcdefghijklmnopqrstuvwxyz123456 | ||
| foreman_plugins: "{{ enabled_features | reject('contains', 'content/') | reject('eq', 'hammer') | reject('eq', 'foreman-proxy') | reject('eq', 'foreman') }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use difference filter here instead of 3 reject filters?
| features: | ||
| parameter: --add-feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also consider adding a --list-feature option to display visible features, but that can be done outside this PR, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#337 is tracking this, yes
This PR introduces two new concepts: `flavor`s and `feature`s. A feature is an abstract representation of "the deployed system can now do X", usually implemented by enabling a Foreman/Pulp/Hammer plugin (or a collection of these). A flavor is a set of features that are enabled by default and can not be disabled. This is to allow our common deployment types like "vanilla foreman", "katello", "satellite" and similar. The idea is to allow people to select a baseline using `--flavor` and then further adjust it to their liking using `--feature` and the code then figures out what foreman/hammer/pulp plugin (or combination of these) needs to be enabled for that.
|
Thanks everyone! |
This PR introduces two new concepts:
flavors andfeatures.feature
A feature is an abstract representation of "the deployed system can now do X", usually implemented by enabling a Foreman/Pulp/Hammer plugin (or a collection of these).
flavor
A flavor is a set of features that are enabled by default and can not be disabled. This is to allow our common deployment types like "vanilla foreman", "katello", "satellite" and similar.
workflow
The idea is to allow people to select a baseline using
--flavorand then further adjust it to their liking using--featureand the code then figures out what foreman/hammer/pulp plugin (or combination of these) needs to be enabled for that.