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.

Sign up for GitHub

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

Jump to bottom

Seperate affinity value per pod #57

Closed
wants to merge 4 commits into from
Closed

Seperate affinity value per pod #57

wants to merge 4 commits into from

Conversation

younsl
Copy link

@younsl younsl commented Jul 1, 2024 •
edited
Loading

Summary

Tests

  • On my EKS v1.28 cluster, successfully deployed via argocd.
$ kubectl get app -n argocd
NAME     SYNC STATUS   HEALTH STATUS
...
mageai   Synced        Degraded

cc: @wangxiaoyou1993

younsl added 2 commits July 1, 2024 14:04
@younsl
chore: Add affinity to deployment templates
a1008e1
@younsl
refactor: Divide affinity value per pod spec
989751c 
* The affinity settings commonly used by webServer and scheduler are separated to improve flexibility.
wangxiaoyou1993
wangxiaoyou1993 reviewed Jul 10, 2024
View reviewed changes
charts/mageai/templates/deployment-scheduler.yaml Outdated
@@ -120,7 +120,7 @@ spec:
nodeSelector:
{{- toYaml . | nindent 8 }}
{{- end }}
{{- with .Values.affinity }}
{{- with .Values.scheduler.affinity }}
Copy link
Member

Choose a reason for hiding this comment

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

can you make it backward compatible?

Copy link
Author

@younsl younsl Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@wangxiaoyou1993 Could you please give me more details for backward compatible? Does backward compatible mean conditional processing of Values.affinity values, too?

Copy link
Member

Choose a reason for hiding this comment

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

the old values.yaml uses affinity config. can you make it as the fallback config here.
similar change in another PR: https://github.com/mage-ai/helm-charts/pull/45/files

Copy link
Author

Choose a reason for hiding this comment

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

@wangxiaoyou1993 I understand. I will add a commit for the if else conditional statement.

Copy link
Author

Choose a reason for hiding this comment

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

@wangxiaoyou1993 I added the backward compatibility for affinity value using "if else" condition.
Could you please review this changes?

@younsl younsl closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangxiaoyou1993 wangxiaoyou1993 wangxiaoyou1993 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@younsl @wangxiaoyou1993