Skip to content

Add affinity settings in viz and jaeger chart#11464

Merged
alpeb merged 4 commits intolinkerd:mainfrom
naing2victor:viz-chart-affinity
Nov 17, 2023
Merged

Add affinity settings in viz and jaeger chart#11464
alpeb merged 4 commits intolinkerd:mainfrom
naing2victor:viz-chart-affinity

Conversation

@naing2victor
Copy link
Contributor

@naing2victor naing2victor commented Oct 6, 2023

Add nodeAffinity for deployment template to linkerd-viz and linkerd-jaeger charts

Fixes #10680

@naing2victor naing2victor requested a review from a team as a code owner October 6, 2023 20:07
@naing2victor naing2victor changed the title Add nodeAffinity in viz and jaeger chart (#10680) Add nodeAffinity in viz and jaeger chart Oct 6, 2023
adleong
adleong reviewed Oct 9, 2023
View reviewed changes
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

Thanks, @naing2victor!

I noticed that you're setting the component and label values which are used by podAntiAffinity, not nodeAffinity. What do you think of adding

# -- Enables Pod Anti Affinity logic to balance the placement of replicas
# across hosts and zones for High Availability.
# Enable this only when you have multiple replicas of components.
enablePodAntiAffinity: false

to the values.yaml since we're setting those component and label values anyway. That makes it consistent with the other extensions as well.

@naing2victor
Copy link
Contributor Author

naing2victor commented Oct 10, 2023

@adleong Thank you for your feedback. I can see that linkerd-viz chart is already implemented with podAntiAffinity. We'll only need to apply for linkerd-jaeger chart. Adding enablePodAntiAffinity may also need to variabilize the replicas for each component. May I know how would you suggest for the value usage.

Would it be better if we separate the variable by the component. For example,

replicas: {{.Values.jaegerInjector.replicas}}
replicas: {{.Values.collector.replicas}}
replicas: {{.Values.jaeger.replicas}}

Or just using 1 value for all the components.

replicas: {{.Values.replicas}}

@adleong
Copy link
Member

adleong commented Oct 20, 2023

I'd recommend having a separate replicas value for each component (which is what we do in linkerd-viz). However, the exception to that is that increasing the number of replicas of the jaeger component could be problematic since this is a stateful trace backend; so we may want to leave that one hardcoded at 1 replica.

@naing2victor
Add enablePodAntiAffinity to jaeger chart
e17f2a3 
- variablize replica value for each component except `jager` component
- add update strategy
- update docs
Signed-off-by: Naing Naing Htun <[email protected]>
@naing2victor
Copy link
Contributor Author

naing2victor commented Oct 24, 2023

I've added enablePodAntiAffinity and make replicas values to get from values file.

alpeb
alpeb reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

If you try rendering the templates with enablePodAntiAffinity=true you'll see there's a problem. I believe the issue is not passing $tree into the linkerd.affinity include.

@naing2victor
Fix previous commit
4cdcc8e 
- issue is not passing `$tree` into the `linkerd.affinity` include
- move `jaegerInjector` under `webhook`
- Capitalize comments

Signed-off-by: Naing Naing Htun <[email protected]>
@naing2victor naing2victor requested a review from alpeb November 8, 2023 17:31
@adleong
Fix helm readmes
227f43d 
Signed-off-by: Alex Leong <[email protected]>
@alpeb alpeb changed the title Add nodeAffinity in viz and jaeger chart Add affinity settings in viz and jaeger chart Nov 17, 2023
@alpeb alpeb merged commit fd54697 into linkerd:main Nov 17, 2023
@hawkw hawkw mentioned this pull request Nov 22, 2023
Merged
hawkw added a commit that referenced this pull request Nov 22, 2023
@hawkw
edge-23.11.4 (#11642)
6a260fa 
## edge-23.11.4

This edge release introduces support for the native sidecar containers
entering beta support in Kubernetes 1.29. This improves the startup and
shutdown ordering for the proxy relative to other containers, fixing the
long-standing shutdown issue with injected `Job`s. Furthermore, traffic
from other `initContainer`s can now be proxied by Linkerd.

In addition, this edge release includes Helm chart improvements, and
improvements to the multicluster extension.

* Added a new `config.alpha.linkerd.io/proxy-enable-native-sidecar`
  annotation and `Proxy.NativeSidecar` Helm option that causes the proxy
  container to run as an init-container (thanks @teejaded!) (#11465;
  fixes #11461)
* Fixed broken affinity rules for the multicluster `service-mirror` when
  running in HA mode (#11609; fixes #11603)
* Added a new check to `linkerd check` that ensures all extension
  namespaces are configured properly (#11629; fixes #11509)
* Updated the Prometheus Docker image used by the `linkerd-viz`
  extension to v2.48.0, resolving a number of CVEs in older Prometheus
  versions (#11633)
* Added `nodeAffinity` to `deployment` templates in the `linkerd-viz`
  and `linkerd-jaeger` Helm charts (thanks @naing2victor!) (#11464;
  fixes #10680)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alpeb alpeb alpeb approved these changes

@adleong adleong adleong approved these changes

Assignees

@adleong adleong

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

linkerd-viz and linkerd-jaeger charts do not support nodeAffinity

3 participants

@naing2victor @adleong @alpeb