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

operator: Add zone awareness spec to LokiStack #9315

Merged
merged 15 commits into from
May 2, 2023
Merged

operator: Add zone awareness spec to LokiStack #9315

merged 15 commits into from
May 2, 2023

Conversation

aminesnow
Copy link
Contributor

@aminesnow aminesnow commented Apr 27, 2023 •
edited
Loading

What this PR does / why we need it:
This PR adds ReplicationSpec and deprecates ReplicationFactor (more details in the enhancement proposal).
This new spec will allow adding TopologySpreadConstraints to read/write paths' components.

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

@aminesnow aminesnow marked this pull request as ready for review April 27, 2023 10:23
@aminesnow aminesnow requested a review from a team as a code owner April 27, 2023 10:23
JoaoBraveCoding
JoaoBraveCoding reviewed Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

Some comments

operator/apis/loki/v1/lokistack_types.go
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Minimum:=1
// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors="urn:alm:descriptor:com.tectonic.ui:number",displayName="Replication Factor"
Factor int32 `json:"factor,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I only discovered this recently but according to [1] if a field is optional and its type doesn't build in nil then they should be pointers. Now I also know that to not change a project conventions so I'm simply raising the question if this should not be a pointer

[1] https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that for int32, 0 is the nil value, so if the value is 0, it will be omitted, which is the desired result.

operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
operator/internal/manifests/config.go Show resolved Hide resolved
operator/internal/manifests/distributor_test.go Outdated Show resolved Hide resolved
operator/internal/manifests/ingester_test.go Outdated Show resolved Hide resolved
operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
periklis
periklis reviewed May 2, 2023
View reviewed changes
Copy link
Collaborator

@periklis periklis left a comment

Choose a reason for hiding this comment

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

Did you leave the index-gateway and ruler components intentionally out?

@aminesnow
Copy link
Contributor Author

Did you leave the index-gateway and ruler components intentionally out?

Yes. As per the Enhancement Proposal, this only affects the read and write components.

@periklis
Copy link
Collaborator

periklis commented May 2, 2023

Did you leave the index-gateway and ruler components intentionally out?

Yes. As per the Enhancement Proposal, this only affects the read and write components.

But index-gateway is related to the read path

Mohamed-Amine Bouqsimi added 2 commits May 2, 2023 16:30
@periklis periklis merged commit a41176a into grafana:main May 2, 2023
This was referenced May 3, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoaoBraveCoding JoaoBraveCoding JoaoBraveCoding left review comments

@periklis periklis periklis approved these changes

Assignees
No one assigned
Labels
sig/operator size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aminesnow @periklis @JoaoBraveCoding