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

operator: Add PodAntiAffinity overwrites per component #9408

Merged
merged 2 commits into from
May 9, 2023

Conversation

JoaoBraveCoding
Copy link
Collaborator

What this PR does / why we need it:
Issue: https://issues.redhat.com/browse/LOG-3854
Adds PodAffinity and PodAntiAffinity overwrites per component
Refactors Affinity/AntiAffinity tests to reduce code duplication

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

operator/CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 272 to 279

// PodAntiAffinity defines the pod anti affinity scheduling rules to schedule pods
// of a component.
//
// +optional
// +kubebuilder:validation:Optional
PodAntiAffinity *corev1.PodAntiAffinity `json:"podAntiAffinity,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK, anti-affinity is only a pod-level concept in k8s. So WDYT to skip the Pod prefix here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't 100% agree with this just because it assumes the user also knows about that "truth", while the full name makes it more explicit and clear. But I'm happy to oblige if it makes more sense to drop the Pod prefix

operator/apis/loki/v1/lokistack_types.go Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why var_test_lib.go and not simply var_test.go? Furthermore I believe all these tests belong in node_placement_test.go anyway.

Copy link
Collaborator Author

@JoaoBraveCoding JoaoBraveCoding May 8, 2023

Choose a reason for hiding this comment

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

If defined the function in node_placement_test.go then 2 things would happen:

  1. I could not export it because otherwise it would be run by the go test command and it would always fail
  2. Since I could not export it, some components would not be able to access it since it would not be exported, because their tests are not on the package manifest_test.

However, given #9408 (comment) we will stop using an auxiliary function for the tests and we will have a single function so this stops being a problem.

operator/internal/manifests/ruler_test.go Outdated Show resolved Hide resolved
operator/internal/manifests/var_test_lib.go Outdated Show resolved Hide resolved
@periklis periklis changed the title operator: Add PodAffinity and PodAntiAffinity overwrites per component operator: Add PodAntiAffinity overwrites per component May 8, 2023
@pull-request-size pull-request-size bot added size/XL and removed size/L labels May 8, 2023
Issue: https://issues.redhat.com/browse/LOG-3854

Refactors Affinity/AntiAffinity tests to reduce code duplication

Signed-off-by: Joao Marcal <jmarcal@redhat.com>
Signed-off-by: Joao Marcal <jmarcal@redhat.com>
@periklis periklis merged commit 3fa46c5 into grafana:main May 9, 2023
@JoaoBraveCoding JoaoBraveCoding deleted the pa-paa branch January 31, 2024 11:08
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.

2 participants