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 podLabels podAnnotations to Helm Charts #8712

Merged
merged 10 commits into from Apr 17, 2024
Merged

Conversation

yashgorana
Copy link
Contributor

@yashgorana yashgorana commented Apr 17, 2024

Description

This is to support GKE and any cloud-specific configurations

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

  • Describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • List any relevant details for your test configuration.

Checklist

@yashgorana
Copy link
Contributor Author

@eelcovdw i have relaxed the resourcePresets in this PR.
backend=xlarge (4 cpu 8gb max)
seaweed=xlarge (4 cpu 8gb max)
mongo=large (2cpu 4gb max)
proxy=small (0.5cpu 1GB max)
frontend=medium (1cpu 2GB max)

in a separate pr i'll update worker pools to use 4xlarge presets (8cpu and 16gb min)

@@ -37,8 +46,6 @@ spec:
secretKeyRef:
name: {{ .Values.seaweedfs.secretKeyName | required "seaweedfs.secretKeyName is required" }}
key: s3RootPassword
- name: MOUNT_API_PORT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Why are we removing this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really required. Because (1) not exposed to the net and (2) the idea is to avoid dynamic port assignment for simplicity and follow standard port numbers for internal apis.

@@ -20,7 +20,16 @@ spec:
labels:
{{- include "common.labels" . | nindent 8 }}
app.kubernetes.io/component: backend
{{- if .Values.node.podLabels }}
{{- toYaml .Values.node.podLabels | nindent 8 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we input mutiple podsLabels , it takes the last one.

Example Values files

node:
    podLabels:
        test/label: 5
        test/label: 2

Output

Screenshot 2024-04-17 at 3 00 27 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if I reverse the order
Screenshot 2024-04-17 at 3 01 58 PM

It takes the last one

Screenshot 2024-04-17 at 3 02 19 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should take into account all the pod labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is valid & expected. metadata.labels & metadata.annotations are a map and not list. Both kubernetes and golang templating will hold only one value because underlying struct is a go map. You also see a squigly error line under your YAML because that's validation failure 😉

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh ok, is there a way such that a user could provide multiple pod annotations to pod spec by helm values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is supported and this change implements it (example), but k8s spec has explicit directives for the type of values the labels and annotations must have. Since keys-values must be strings, often times k8s providers will expect a JSON string as values (GKE example)

@yashgorana yashgorana self-assigned this Apr 17, 2024
@yashgorana yashgorana merged commit 6a34d91 into dev Apr 17, 2024
35 checks passed
@yashgorana yashgorana deleted the yash/helm-node-selector branch April 17, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants