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

Add option to configure machine config pools #50

Merged
merged 16 commits into from
Oct 6, 2022
Merged

Conversation

glrf
Copy link
Contributor

@glrf glrf commented Oct 4, 2022

Checklist

  • PR contains a single logical change (to build a better changelog).
  • Update the documentation.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog.

@glrf glrf added the enhancement New feature or request label Oct 4, 2022
@glrf glrf self-assigned this Oct 4, 2022
OpenShift doesn't really look at the labelselector of the pool and just
assumes that it matches labels with the name
@glrf glrf force-pushed the feat/machine-config branch 2 times, most recently from 96469a6 to 152b4ca Compare October 4, 2022 14:37
@glrf
Copy link
Contributor Author

glrf commented Oct 4, 2022

I will add a how to explaining some of the ways to use machine pool configs and some pitfalls in a separate PR. But I'd like to have a general review of the component first.

Also this change will most likely cause all worker nodes to reboot one after another as OpenShift somehow doesn't understand that the machine config did not actually change. Should we mark this as breaking, as this should be released during a maintenance window?

@glrf glrf marked this pull request as ready for review October 4, 2022 14:58
@glrf glrf requested a review from a team October 4, 2022 14:58
Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

LGTM, with the std.prune addressed as discussed.

Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM overall, take or leave the suggestions on how we could reduce code duplication, it's not that problematic here.

Generally speaking, I feel like the implementation for containerruntimeconfigs and kubeletconfigs is just cryptic enough that it would greatly benefit from short comments explaining what's happening. As it is, it took me a while to figure out what the code is doing (I actually read the parameter docs before going back to reviewing the code)

class/defaults.yml Outdated Show resolved Hide resolved
component/container-runtime.jsonnet Outdated Show resolved Hide resolved
component/container-runtime.jsonnet Outdated Show resolved Hide resolved
component/container-runtime.jsonnet Outdated Show resolved Hide resolved
@glrf
Copy link
Contributor Author

glrf commented Oct 6, 2022

Did a bunch of refactoring and removed some magic. I hope this is no a bit more readable.

@glrf glrf requested a review from simu October 6, 2022 08:57
local machineConfig = std.foldl(
function(configs, name)
local pool = params.machineConfigPools[name];
local machineConfigSpecs = std.get(pool, 'machineConfigs', default={});
Copy link
Member

Choose a reason for hiding this comment

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

This requires Jsonnet 0.18.0, which is probably fine at this point (at least my local Commodore install has Jsonnet 0.18.0)

docs/modules/ROOT/pages/references/parameters.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/references/parameters.adoc Outdated Show resolved Hide resolved
component/container-runtime.jsonnet Outdated Show resolved Hide resolved
Co-authored-by: Simon Gerber <simon.gerber@vshn.ch>
@simu
Copy link
Member

simu commented Oct 6, 2022

Also this change will most likely cause all worker nodes to reboot one after another as OpenShift somehow doesn't understand that the machine config did not actually change. Should we mark this as breaking, as this should be released during a maintenance window?

I think I'm in favor of making this a new major version, to clearly signal that it needs special care (and to ensure Renovate doesn't auto-update the VSHN defaults)

@glrf glrf added the breaking label Oct 6, 2022
@glrf glrf merged commit 324c650 into master Oct 6, 2022
@glrf glrf deleted the feat/machine-config branch October 6, 2022 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants