-
Notifications
You must be signed in to change notification settings - Fork 419
Add metrics for logical clusters count #3496
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
Conversation
Skipping CI for Draft Pull Request. |
afc32f4
to
bc40271
Compare
On-behalf-of: @SAP [email protected] Signed-off-by: Karol Szwaj <[email protected]>
bc40271
to
dfc8444
Compare
Signed-off-by: Karol Szwaj <[email protected]> On-behalf-of: @SAP [email protected]
return | ||
} | ||
|
||
if logicalCluster.Status.Phase == corev1alpha1.LogicalClusterPhaseReady { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add metrics for all states:
const (
LogicalClusterPhaseScheduling LogicalClusterPhaseType = "Scheduling"
LogicalClusterPhaseInitializing LogicalClusterPhaseType = "Initializing"
LogicalClusterPhaseReady LogicalClusterPhaseType = "Ready"
// LogicalClusterPhaseUnavailable phase is used to indicate that the logical cluster is unavailable to be used.
// It will not be served via front-proxy when in this state.
// Possible state transitions are from Ready to Unavailable and from Unavailable to Ready.
// This should be used when we really can't serve the logical cluster content and not some
// temporary flakes, like readiness probe failing.
LogicalClusterPhaseUnavailable LogicalClusterPhaseType = "Unavailable"
)
Basically if I have bug where my logicalcluster hangs in failed or initializing state - I want to know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid point, generic count can be helpful in this situation, let's keep it simpler as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look implemented @cnvergence or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped the Readiness check - we just fetch the count of all logical clusters.
Or do we want to add a logical cluster count metric for ready and other separately? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was the idea. So it would be a phase
label on the metric:
kcp_logicalcluster_count{phase="Ready"} 5
kcp_logicalcluster_count{phase="Scheduling"} 3
kcp_logicalcluster_count{phase="Unavailable"} 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will consider adding the phase label and processing it during updates. Possibly, we can also split the metrics.
This will align with how, for example, kube-state-metrics processes the count of the pods
/kind feature |
pkg/reconciler/tenancy/logicalcluster/logicalcluster_controller.go
Outdated
Show resolved
Hide resolved
return | ||
} | ||
|
||
if logicalCluster.Status.Phase == corev1alpha1.LogicalClusterPhaseReady { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look implemented @cnvergence or am I missing something?
Signed-off-by: Karol Szwaj <[email protected]> On-behalf-of: @SAP [email protected]
22c864b
to
9ccda86
Compare
/lgtm |
LGTM label has been added. Git tree hash: f8834b926c94a3030f3347a5a60926207023dfb8
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjudeikis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
Signed-off-by: Karol Szwaj <[email protected]> On-behalf-of: @SAP [email protected]
79293d9
to
37ba8e9
Compare
pkg/reconciler/tenancy/logicalcluster/logicalcluster_controller.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/tenancy/logicalcluster/logicalcluster_controller.go
Outdated
Show resolved
Hide resolved
…r.go Co-authored-by: Nelo-T. Wallus <[email protected]>
…r.go Co-authored-by: Nelo-T. Wallus <[email protected]>
/retest integration flake, it's been getting worse =/ |
LGTM label has been added. Git tree hash: a4fb857ee54430d476313708e0f3142af893c4d7
|
/retest |
Summary
Introduce
kcp_logicalcluster_count
metric, that will hold the value of currently running logical clusters in the shard.What Type of PR Is This?
/kind feature
Related Issue(s)
Fixes #3479
Release Notes