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 affinity config in fluentd deployment #436

Merged
merged 8 commits into from
Feb 21, 2020

Conversation

vsinghal13
Copy link
Contributor

@vsinghal13 vsinghal13 commented Feb 21, 2020

Description

This PR exposes the use of podAntiAffinity to better schedule the fluentd replicas and prometheus pods across nodes.

By default, we are using a preferredDuringSchedulingIgnoredDuringExecution configuration alias soft which will check the conditions and schedule accordingly even if the conditions are not met.

The requiredDuringSchedulingIgnoredDuringExecution configuration alias hard will keep the pods in Pending state if any of the conditions are not met.

Conditions:

  • Don't schedule two fleuntd replicas on the same node
  • Don't schedule prometheus pod along with fluentd
Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

Comment on lines +42 to +45
- key: app
operator: In
values:
- prometheus-operator-prometheus
Copy link
Contributor Author

@vsinghal13 vsinghal13 Feb 21, 2020

Choose a reason for hiding this comment

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

This section checks for the prometheus app label.

Comment on lines +38 to +41
- key: app
operator: In
values:
- {{ template "sumologic.labels.app" . }}
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 section checks for the fleuntd app label.

operator: In
values:
- prometheus-operator-prometheus
topologyKey: "kubernetes.io/hostname"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The topologyKey is the label for a particular node.

Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

LGTM. Adding @perk-sumo as a reviewer as he has some experience with this as well.

@@ -6,7 +6,7 @@
sumologicCollector:: {
remoteWriteConfigs+: [
{
url: $._config.sumologicCollectorSvc + "prometheus.metrics.state",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are those changes OK? Why they were expanded now with this PR?

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 has been fixed, by merging master into the branch.

Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

LGTM except the libsonnet changes that I'm not sure about.

Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

LGTM, one minor comment

@@ -8,6 +8,8 @@ nameOverride: ""
deployment:
nodeSelector: {}
tolerations: {}
affinity: {}
podAntiAffinity: "soft"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also leave a comment here for the customers that the accepted values are "soft" and "hard", and what those mean?

@vsinghal13 vsinghal13 merged commit 8b5353d into master Feb 21, 2020
@vsinghal13 vsinghal13 deleted the vsinghal-pod-anti-affinity branch February 21, 2020 19:22
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.

None yet

4 participants