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: Include runtime-config in compiling the SHA1 checksum #9448

Merged
merged 12 commits into from
May 26, 2023

Conversation

btaani
Copy link
Contributor

@btaani btaani commented May 11, 2023

What this PR does / why we need it:
Any changes made to the runtime-config can now trigger the reconciliation of the LokiStack.

Which issue(s) this PR fixes:
Fixes:
LOG-3953 - Updating the RulerConfig CR can now trigger the restart of the ruler pod (and thus reflecting the changes to the user-workload AM)
LOG-3950 - Enabling user-workload AM

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

@btaani btaani marked this pull request as ready for review May 11, 2023 12:49
@btaani btaani requested a review from a team as a code owner May 11, 2023 12:49
@btaani btaani changed the title operator: restart Ruler pod after RulerConfig instance is modified operator: Restart Ruler pod after RulerConfig instance is modified May 11, 2023
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.

The proposed change set works against our first principles that the operator should delegate anything possible to the Kubernetes APIServer instead of inventing the wheel anew.

By saying this, I suggest you take a look on the code path starting from this snippet to discover how we instruct k8s to restart pods by passing annotations to the pod template spec:

certRotationRequiredAt := ""
if stack.Annotations != nil {
certRotationRequiredAt = stack.Annotations[manifests.AnnotationCertRotationRequiredAt]
}

I.e. The ruler config annotation is something that is updated on the LokiStack CR on every RulerConfig change. Affected components, e.g. ruler needs this annotation to be passed to the pod template spec.

@btaani
Copy link
Contributor Author

btaani commented May 16, 2023

@periklis updating the annotations is already implemented here for RulerConfig changes. However, updating anything below the overrides section of the RulerConfig CR updates the annotationRulerConfigDiscoveredAt value for the lokistack, but doesn't seem to trigger restarting the ruler pod, which is why I explicitly restart it like this.
I do understand your argument though. I'll redo it to match the rest of our operator code

@btaani btaani force-pushed the am-relabel branch 2 times, most recently from 042f7fc to 7f3a09b Compare May 25, 2023 11:36
@pull-request-size pull-request-size bot added size/S and removed size/M labels May 25, 2023
@btaani btaani changed the title operator: Restart Ruler pod after RulerConfig instance is modified operator: Include runtime-config in compiling the SHA1 checksum May 25, 2023
operator/CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 10 to 11
lokiv1 "github.com/grafana/loki/operator/apis/loki/v1"
"github.com/grafana/loki/operator/internal/external/k8s"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this change?

@btaani btaani force-pushed the am-relabel branch 2 times, most recently from 7bad4f7 to a5d8064 Compare May 26, 2023 09:40
operator/CHANGELOG.md Outdated Show resolved Hide resolved
operator/CHANGELOG.md Outdated Show resolved Hide resolved
@periklis periklis merged commit bb96326 into grafana:main May 26, 2023
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