Skip to content

Conversation

@andrey-dubnik
Copy link
Contributor

@andrey-dubnik andrey-dubnik commented Oct 28, 2021

This fixes #3031

@andrey-dubnik andrey-dubnik requested a review from a team as a code owner October 28, 2021 13:56
@andrey-dubnik andrey-dubnik force-pushed the dev-affinity-cp-ingress branch from fbf1426 to 94c771a Compare October 28, 2021 13:56
@bartsmykla bartsmykla changed the title add affinity to CP and Ingress PODs feat(kuma-cp) add affinity to CP and Ingress PODs Nov 2, 2021
@jpeach
Copy link
Contributor

jpeach commented Nov 3, 2021

@andrey-dubnik a few questions on this approach

  • should kuma-cp include a "soft" anti-affinity configuration by default?
  • are there conventions around this in the helm chart community?

@andrey-dubnik
Copy link
Contributor Author

Yes, soft anti-affinity would be the preferred default but for the true HA affinity should be the hard rule which makes it only 1 pod per the node which might be too restrictive in the scenario where the control plane needs to scale beyond the number of nodes. Unfortunately for the control plane the app label is dynamic so setting the rule via values is problematic so it either changing the label to static and having a default or letting the helm user to write own affinity rule leaving the default empty.

@jpeach
Copy link
Contributor

jpeach commented Nov 4, 2021

Unfortunately for the control plane the app label is dynamic so setting the rule via values is problematic

Do you mean the kuma.selectorLabels? It's not that clear to me what that's for, but I think that just using the app label is probably good enough for affinity?

If the labels are unpredictably dynamic, that makes me wonder how people would reasonably know what to use to set the whole affinity stanza themselves.

@andrey-dubnik
Copy link
Contributor Author

andrey-dubnik commented Nov 4, 2021

I mean this one -

app: {{ include "kuma.name" . }}-control-plane

      app: {{ include "kuma.name" . }}-control-plane

Affinity rule has to have a label filter which is computed during the chart templating which make it possible to write the affinity rule in the values knowing the name of the deployment chart is going to have but does not make it straight forward to write a default rule defined in the helm values.

Default can be written directly in the template with the option to override if affinity value is provided. I can update the PR with default rules in the template files and added logic to accept affinity override if provided via values if this helps?

@jpeach
Copy link
Contributor

jpeach commented Nov 5, 2021

Default can be written directly in the template with the option to override if affinity value is provided. I can update the PR with default rules in the template files and added logic to accept affinity override if provided via values if this helps?

Yeh, it sounds to me like soft anti-affinity is the right default, and we should let people override this if they need to, as per your PR here.

Signed-off-by: cloudwiz <[email protected]>
@andrey-dubnik andrey-dubnik force-pushed the dev-affinity-cp-ingress branch from 90edbef to 86cbe76 Compare November 5, 2021 09:40
Signed-off-by: cloudwiz <[email protected]>
@andrey-dubnik andrey-dubnik force-pushed the dev-affinity-cp-ingress branch from 4edd093 to 67248e0 Compare November 5, 2021 11:01
@codecov-commenter
Copy link

Codecov Report

Merging #3036 (67248e0) into master (db4f9b0) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3036      +/-   ##
==========================================
+ Coverage   52.32%   52.34%   +0.01%     
==========================================
  Files         919      919              
  Lines       52989    52989              
==========================================
+ Hits        27727    27736       +9     
+ Misses      23053    23040      -13     
- Partials     2209     2213       +4     
Impacted Files Coverage Δ
pkg/plugins/runtime/gateway/route/sorter.go 61.53% <0.00%> (-5.13%) ⬇️
pkg/plugins/leader/postgres/leader_elector.go 93.61% <0.00%> (-4.26%) ⬇️
pkg/core/runtime/component/component.go 88.67% <0.00%> (+7.54%) ⬆️
pkg/core/bootstrap/autoconfig.go 54.46% <0.00%> (+8.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db4f9b0...67248e0. Read the comment docs.

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

This looks great to me @andrey-dubnik! I'll wait a couple of days to give other people a change to give feedback, then merge.

@lahabana
Copy link
Contributor

This looks good thanks @andrey-dubnik! Seems like you might want to update the PR though!

Signed-off-by: cloudwiz <[email protected]>
@andrey-dubnik andrey-dubnik force-pushed the dev-affinity-cp-ingress branch from 47478ae to ef0e20f Compare November 22, 2021 11:02
@jpeach jpeach merged commit cdff385 into kumahq:master Nov 24, 2021
@jpeach
Copy link
Contributor

jpeach commented Nov 24, 2021

Thanks @andrey-dubnik 👍

mergify bot pushed a commit that referenced this pull request Nov 24, 2021
* add affinity configuration to CP and Ingress pods
* add default affinity rules

Signed-off-by: cloudwiz <[email protected]>
(cherry picked from commit cdff385)
mergify bot added a commit that referenced this pull request Dec 6, 2021
* add affinity configuration to CP and Ingress pods
* add default affinity rules

Signed-off-by: cloudwiz <[email protected]>
(cherry picked from commit cdff385)

Co-authored-by: cloudwiz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm chart does not have an option to configure AntiAffinity

4 participants