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 node affinity to linkerd-viz #10947

Conversation

reyes256
Copy link

@reyes256 reyes256 commented May 26, 2023

Add to Helm linkerd-viz and tracing deployment resources nodeAffinity labels

NodeAffinity was not enabled for Viz Deployments resources

Add to helm values and each of Viz Deployment the labels to add the affinity labels from the shared partials and
allow to be more granular on the manifest setup

cd viz/charts/linkerd-viz/
helm template .

Fixes #10680

Signed-off-by: Fernando Reyes [email protected]

@reyes256 reyes256 requested a review from a team as a code owner May 26, 2023 04:10
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.

Thanks @reyes256 👍

The PR description mentions Jaeger, but the changes are about linkerd-viz. Can you amend that?

Viz' values.yaml already contains a global nodeAffinity entry (commented out). Can you remove it?
For completeness sake, can you add nodeAffinity to the tap, tapInjector and dashboard components as well?

@@ -71,9 +71,10 @@ spec:
{{- include "linkerd.tolerations" (dict "Values" .Values.metricsAPI) | nindent 6 }}
{{- end -}}
{{- include "linkerd.node-selector" (dict "Values" .Values.metricsAPI) | nindent 6 }}
{{- include "linkerd.affinity" (dict "Values" .Values.metricsAPI) | nindent 6 }}
Copy link
Member

Choose a reason for hiding this comment

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

Why was this line moved?

Copy link
Author

Choose a reason for hiding this comment

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

Somehow the changes didn't work accordingly, and adding a line break at the end solved the problem

Copy link
Member

@alpeb alpeb Jun 16, 2023

Choose a reason for hiding this comment

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

I wasn't able to reproduce the issue. Can you provide more details? (I don't understand the comment about empty component label).

@alpeb
Copy link
Member

alpeb commented Jun 9, 2023

Hi @reyes256 , do you still want to take this one across the finish line? Is there anything I can help you with?

@reyes256
Copy link
Author

reyes256 commented Jun 9, 2023

@alpeb yes i would like to continue, i'll be working on it

@reyes256
Copy link
Author

reyes256 commented Jun 9, 2023

@alpeb Seems like 'nodeAffinity' has already been added in both tap and tap-injector components, however i'm not sure how to modify the dashboard component, its not in a separate file and i dont know if its directly on the values.yml

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.

@alpeb Seems like 'nodeAffinity' has already been added in both tap and tap-injector components

They're calling the linkerd.affinity partial, but there's no tap.nodeAffinity nor tapInjector.nodeAffinity entries in values.yaml.

however i'm not sure how to modify the dashboard component, its not in a separate file and i dont know if its directly on the values.yml

It's the web.yaml template, that uses the dashboard entries from values.yaml

@@ -71,9 +71,10 @@ spec:
{{- include "linkerd.tolerations" (dict "Values" .Values.metricsAPI) | nindent 6 }}
{{- end -}}
{{- include "linkerd.node-selector" (dict "Values" .Values.metricsAPI) | nindent 6 }}
{{- include "linkerd.affinity" (dict "Values" .Values.metricsAPI) | nindent 6 }}
Copy link
Member

@alpeb alpeb Jun 16, 2023

Choose a reason for hiding this comment

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

I wasn't able to reproduce the issue. Can you provide more details? (I don't understand the comment about empty component label).

@alpeb
Copy link
Member

alpeb commented Aug 2, 2023

This PR appears to be stale, so closing for now. Please reopen a new one addressing the feedback above if you're still interested on implementing this.

@alpeb alpeb closed this Aug 2, 2023
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.

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