feat: enable optional ServiceMonitor creation for Backstage CR - #1374
Merged
gazarenkov merged 46 commits intoAug 6, 2025
Merged
Conversation
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Contributor
Reviewer's GuideAdd optional ServiceMonitor support for Backstage CR by introducing a spec.monitoring.enabled flag, extending API types and CRD schemas, updating controller logic and RBAC, registering the Prometheus ServiceMonitor API, and adding corresponding tests and mock client support. Sequence diagram for ServiceMonitor reconciliation when monitoring is enabledsequenceDiagram
participant Operator
participant K8sAPI as Kubernetes API
participant Prometheus
Operator->>K8sAPI: Read Backstage CR (spec.monitoring.enabled)
alt monitoring enabled
Operator->>K8sAPI: Patch ServiceMonitor (create/update)
K8sAPI-->>Operator: ServiceMonitor applied
Prometheus->>K8sAPI: Discover ServiceMonitor
K8sAPI-->>Prometheus: ServiceMonitor details
else monitoring disabled
Operator->>K8sAPI: Delete ServiceMonitor if exists
K8sAPI-->>Operator: ServiceMonitor deleted
end
ER diagram for Backstage CRD with new Monitoring fielderDiagram
BACKSTAGE_SPEC {
string name
Database database
Monitoring monitoring
}
MONITORING {
bool enabled
}
BACKSTAGE_SPEC ||--o{ MONITORING : has
Class diagram for updated BackstageSpec and Monitoring typesclassDiagram
class BackstageSpec {
Database* database
Monitoring monitoring
+IsMonitoringEnabled() bool
}
class Monitoring {
bool enabled
}
BackstageSpec --> Monitoring : monitoring
BackstageSpec --> Database : database
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @Fortune-Ndlovu - I've reviewed your changes - here's some feedback:
- The operator’s RBAC needs to be updated to grant permissions for monitoring.coreos.com ServiceMonitor resources so it can create/patch them.
- Disabling spec.monitoring.enabled currently just skips creation and leaves existing ServiceMonitors behind—consider adding logic to delete the ServiceMonitor when monitoring is turned off.
- You may want to expose the metrics port name (and scrape interval) in the CR rather than hardcoding "metrics" to make the ServiceMonitor more configurable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The operator’s RBAC needs to be updated to grant permissions for monitoring.coreos.com ServiceMonitor resources so it can create/patch them.
- Disabling spec.monitoring.enabled currently just skips creation and leaves existing ServiceMonitors behind—consider adding logic to delete the ServiceMonitor when monitoring is turned off.
- You may want to expose the metrics port name (and scrape interval) in the CR rather than hardcoding "metrics" to make the ServiceMonitor more configurable.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: Fortune-Ndlovu <Fortune-Ndlovu@users.noreply.github.com>
Contributor
|
|
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Co-authored-by: Fortune-Ndlovu <Fortune-Ndlovu@users.noreply.github.com>
Contributor
|
|
Fortune-Ndlovu
changed the title
…a make manifests. Implemented applyServiceMonitor logic in monitor.go. Added RBAC for ServiceMonitor and regenerated manager-role. Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Co-authored-by: Fortune-Ndlovu <Fortune-Ndlovu@users.noreply.github.com>
Contributor
|
|
gazarenkov
reviewed
Aug 1, 2025
Fortune-Ndlovu
commented
Aug 3, 2025
Fortune-Ndlovu
commented
Aug 4, 2025
Fortune-Ndlovu
left a comment
Fortune-Ndlovu
left a comment
Member
Author
There was a problem hiding this comment.
make monitor.go consistent with the rest of the codebase while maintaining the same logging functionality
Fortune-Ndlovu
commented
Aug 4, 2025
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @Fortune-Ndlovu - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `internal/controller/mock_client.go:105` </location>
<code_context>
-func (m MockClient) Patch(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
- panic(implementMe)
+func (m MockClient) Patch(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
+ if obj.GetName() == "" {
+ return fmt.Errorf("patch: object Name should not be empty")
</code_context>
<issue_to_address>
Patch method does not simulate patch logic, only replaces the object.
Currently, the Patch method replaces the entire object instead of merging changes, which may cause tests to pass incorrectly. Please update the implementation to better simulate patch behavior or clearly document this limitation.
Suggested implementation:
```golang
func (m MockClient) Patch(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
// NOTE: This mock Patch implementation does NOT perform a true merge/patch operation.
// It only replaces the object in the store with the provided object.
// This may cause tests to pass incorrectly if they rely on patch semantics.
// If you need more accurate patch simulation, extend this method to merge fields from the patch into the existing object.
if obj.GetName() == "" {
return fmt.Errorf("patch: object Name should not be empty")
}
// Simulate CRD not found for ServiceMonitor when CRD is not registered
objKind := kind(obj)
if objKind == "ServiceMonitor" {
// Check if ServiceMonitor CRD exists in our mock store
crdKey := NameKind{Name: "servicemonitors.monitoring.coreos.com", Kind: "CustomResourceDefinition"}
if _, exists := m.objects[crdKey]; !exists {
return fmt.Errorf(`no matches for kind "ServiceMonitor" in version "monitoring.coreos.com/v1"`)
}
```
If you want to simulate a minimal merge for `client.MergeFrom`, you could:
- Retrieve the existing object from `m.objects`
- Use reflection or type assertion to copy only the fields that are set in the patch source to the target object
- Update the object in the store
However, this can be complex and is not shown here. The above change at least documents the limitation clearly, as requested.
</issue_to_address>
### Comment 2
<location> `api/v1alpha4/backstage_types.go:348` </location>
<code_context>
+// IsMonitoringEnabled checks if monitoring is explicitly enabled in the BackstageSpec.
+// Returns false if the Monitoring field is nil (not configured) or explicitly disabled.
+// Returns true only when spec.monitoring.enabled is set to true in the CR
+func (s *BackstageSpec) IsMonitoringEnabled() bool {
+ return s.Monitoring.Enabled
+}
</code_context>
<issue_to_address>
IsMonitoringEnabled does not distinguish between unset and explicitly disabled.
Since Monitoring is not a pointer, IsMonitoringEnabled cannot differentiate between Monitoring being unset and Enabled being false. To address this, consider making Monitoring a pointer.
Suggested implementation:
```golang
// IsMonitoringEnabled checks if monitoring is explicitly enabled in the BackstageSpec.
// Returns false if the Monitoring field is nil (not configured) or explicitly disabled.
// Returns true only when spec.monitoring.enabled is set to true in the CR
func (s *BackstageSpec) IsMonitoringEnabled() bool {
return s.Monitoring != nil && s.Monitoring.Enabled
}
```
```golang
type Monitoring struct {
// Enable ServiceMonitor for Prometheus scraping
// +optional
// +kubebuilder:default=false
Enabled bool `json:"enabled,omitempty"`
}
// In BackstageSpec struct definition (not shown in the provided code), change:
// Monitoring Monitoring `json:"monitoring,omitempty"`
// to:
// Monitoring *Monitoring `json:"monitoring,omitempty"`
```
You must update the `BackstageSpec` struct definition (not shown in the provided code) to change the type of the `Monitoring` field from `Monitoring` to `*Monitoring`:
From:
```go
Monitoring Monitoring `json:"monitoring,omitempty"`
```
To:
```go
Monitoring *Monitoring `json:"monitoring,omitempty"`
```
You should also check any code that instantiates or accesses `BackstageSpec` to ensure it correctly handles the pointer type for `Monitoring`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
gazarenkov
reviewed
Aug 5, 2025
…egenerate bundle manifests Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Co-authored-by: Fortune-Ndlovu <Fortune-Ndlovu@users.noreply.github.com>
Contributor
|
|
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
gazarenkov
approved these changes
Aug 6, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gazarenkov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 tasks
2 tasks
2 tasks
2 tasks
2 tasks
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds optional ServiceMonitor support for Backstage.
A new spec.monitoring.enabled field is introduced in the Backstage CR. When set to true, the operator will automatically create and manage a Prometheus ServiceMonitor for the Backstage instance. If disabled or omitted, no ServiceMonitor is created.
Which issue(s) does this PR fix or relate to
https://issues.redhat.com/browse/RHIDP-5780
PR acceptance criteria
How to test changes / Special notes to the reviewer
Summary by Sourcery
Add optional Prometheus ServiceMonitor support for Backstage by introducing a spec.monitoring.enabled flag, registering the monitoring API, and implementing controller logic to create and manage ServiceMonitor resources when enabled.
New Features:
Build:
Summary by Sourcery
Enable optional ServiceMonitor support for Backstage CR by introducing a new spec.monitoring.enabled flag and implementing controller logic to manage Prometheus ServiceMonitor resources accordingly
New Features:
Enhancements:
Build:
Tests: