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

feat: add Alertmanager's status subresource #5270

Merged

Conversation

simonpasquier
Copy link
Contributor

Description

Following #4580, this change implements the status subresource for the Alertmanager CRD.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Add Alertmanager's status subresource

@simonpasquier simonpasquier force-pushed the alertmanager-status branch 6 times, most recently from 2e1144f to 8c430bd Compare January 6, 2023 16:00
@simonpasquier simonpasquier marked this pull request as ready for review January 9, 2023 09:47
@simonpasquier simonpasquier requested a review from a team as a code owner January 9, 2023 09:47
Copy link
Contributor

@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.

just some minor comments, but apart from that looks awsome 💯

pkg/alertmanager/operator.go Outdated Show resolved Hide resolved
pkg/alertmanager/operator.go Outdated Show resolved Hide resolved
pkg/alertmanager/operator.go Outdated Show resolved Hide resolved
}

a := pobj.(*monitoringv1.Alertmanager)
a = a.DeepCopy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, why is this necessary?

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 is to ensure that the object won't be modified underneath or that the function will not modify it further down the line. I can't find the documentation stating that it's a good practice but it's the safest approach.

aStatus.AvailableReplicas = int32(len(stsReporter.ReadyPods()))
aStatus.UnavailableReplicas = int32(len(stsReporter.Pods) - len(stsReporter.ReadyPods()))

if len(stsReporter.ReadyPods()) < replicas {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wouldn't this also work? Since we already do the calculations in the block above

Suggested change
if len(stsReporter.ReadyPods()) < replicas {
if aStatus.UnavailableReplicas > 0 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

in fact replicas can be > 0 and number of "concrete" pods == 0 in situations where the statefulset definition doesn't generate pods (e.g. non-existing service account name).

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

pkg/alertmanager/operator.go Outdated Show resolved Hide resolved
test/e2e/prometheus_test.go Outdated Show resolved Hide resolved
test/framework/alertmanager.go Outdated Show resolved Hide resolved
test/e2e/alertmanager_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

just one final that slipped through the cracks but from my side after that I'm fine with giving it ✅

test/framework/alertmanager.go Outdated Show resolved Hide resolved
Following prometheus-operator#4580, this change implements the status subresource for the
Alertmanager CRD.

Co-authored-by: JoaoBraveCoding <jmarcal@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

@simonpasquier thanks for doing this.

@JoaoBraveCoding thanks for the great job on the prior reviews as I was a bit late to the party here

@simonpasquier simonpasquier merged commit 8cebe1b into prometheus-operator:main Jan 20, 2023
@simonpasquier simonpasquier deleted the alertmanager-status branch January 20, 2023 07:58
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

3 participants