From 1ea726d628eab88a5a72d61e14f08aea14c7078e Mon Sep 17 00:00:00 2001 From: Jan Fajerski Date: Wed, 21 Dec 2022 11:11:06 +0100 Subject: [PATCH] feat: add resourceDiscovery status condition (#223) * feat: add resourceDiscovery status condition The ResourceSelector field in a MonitoringStack CR can be set to nil. This is passed on to the Prometheus CR and can be used to disable all service discovery and thus most scraping (we add prometheus and alertmanager scrape jobs via additionalScrapeConfigs). When this is configured we now set a new status condition to alert users to the fact that no service discovery is configured. Fixes: https://issues.redhat.com/browse/MON-2864 * refactor: updateConditions should update previously not existing conditions Problem: Before this commit only an empty Conditions list would initialize all conditions. This means that if the controller gets updated to report a new conditions and existing MonitoringStack CR would not report that new conditions until its recreated. Fix: Update all conditions independently of the previously reported conditions. Signed-off-by: Jan Fajerski --- .../monitoring.rhobs_monitoringstacks.yaml | 5 +- .../monitoring.rhobs_monitoringstacks.yaml | 5 +- docs/api.md | 4 +- pkg/apis/monitoring/v1alpha1/types.go | 10 +- .../monitoring/monitoring-stack/components.go | 3 - .../monitoring/monitoring-stack/conditions.go | 110 ++++-- .../monitoring-stack/conditions_test.go | 350 ++++++++++-------- .../monitoring/monitoring-stack/controller.go | 2 +- test/e2e/monitoring_stack_controller_test.go | 46 ++- 9 files changed, 333 insertions(+), 202 deletions(-) diff --git a/bundle/manifests/monitoring.rhobs_monitoringstacks.yaml b/bundle/manifests/monitoring.rhobs_monitoringstacks.yaml index 104c4070..052883eb 100644 --- a/bundle/manifests/monitoring.rhobs_monitoringstacks.yaml +++ b/bundle/manifests/monitoring.rhobs_monitoringstacks.yaml @@ -835,7 +835,10 @@ spec: type: integer type: object resourceSelector: - description: Label selector for Monitoring Stack Resources. + description: Label selector for Monitoring Stack Resources. Set to + the empty LabelSelector ({}) to monitoring everything. Set to null + to disable service discovery. + nullable: true properties: matchExpressions: description: matchExpressions is a list of label selector requirements. diff --git a/deploy/crds/common/monitoring.rhobs_monitoringstacks.yaml b/deploy/crds/common/monitoring.rhobs_monitoringstacks.yaml index 1b20ec24..c2fac9bf 100644 --- a/deploy/crds/common/monitoring.rhobs_monitoringstacks.yaml +++ b/deploy/crds/common/monitoring.rhobs_monitoringstacks.yaml @@ -836,7 +836,10 @@ spec: type: integer type: object resourceSelector: - description: Label selector for Monitoring Stack Resources. + description: Label selector for Monitoring Stack Resources. Set to + the empty LabelSelector ({}) to monitoring everything. Set to null + to disable service discovery. + nullable: true properties: matchExpressions: description: matchExpressions is a list of label selector requirements. diff --git a/docs/api.md b/docs/api.md index e2901a3d..ed197f78 100644 --- a/docs/api.md +++ b/docs/api.md @@ -124,7 +124,7 @@ MonitoringStackSpec is the specification for desired Monitoring Stack resourceSelector object - Label selector for Monitoring Stack Resources.
+ Label selector for Monitoring Stack Resources. Set to the empty LabelSelector ({}) to monitoring everything. Set to null to disable service discovery.
false @@ -1803,7 +1803,7 @@ RelabelConfig allows dynamic rewriting of the label set, being applied to sample -Label selector for Monitoring Stack Resources. +Label selector for Monitoring Stack Resources. Set to the empty LabelSelector ({}) to monitoring everything. Set to null to disable service discovery. diff --git a/pkg/apis/monitoring/v1alpha1/types.go b/pkg/apis/monitoring/v1alpha1/types.go index 42f8f987..7f307fda 100644 --- a/pkg/apis/monitoring/v1alpha1/types.go +++ b/pkg/apis/monitoring/v1alpha1/types.go @@ -58,8 +58,11 @@ type MonitoringStackSpec struct { LogLevel LogLevel `json:"logLevel,omitempty"` // Label selector for Monitoring Stack Resources. + // Set to the empty LabelSelector ({}) to monitoring everything. + // Set to null to disable service discovery. // +optional - ResourceSelector *metav1.LabelSelector `json:"resourceSelector,omitempty"` + // +nullable + ResourceSelector *metav1.LabelSelector `json:"resourceSelector"` // Namespace selector for Monitoring Stack Resources. // If left empty the Monitoring Stack will only match resources in the namespace it was created in. @@ -108,8 +111,9 @@ const ( ConditionFalse ConditionStatus = "False" ConditionUnknown ConditionStatus = "Unknown" - ReconciledCondition ConditionType = "Reconciled" - AvailableCondition ConditionType = "Available" + ReconciledCondition ConditionType = "Reconciled" + AvailableCondition ConditionType = "Available" + ResourceDiscoveryCondition ConditionType = "ResourceDiscovery" ) type Condition struct { diff --git a/pkg/controllers/monitoring/monitoring-stack/components.go b/pkg/controllers/monitoring/monitoring-stack/components.go index 50886313..1c5b6c6f 100644 --- a/pkg/controllers/monitoring/monitoring-stack/components.go +++ b/pkg/controllers/monitoring/monitoring-stack/components.go @@ -108,9 +108,6 @@ func newPrometheus( instanceSelectorValue string, ) *monv1.Prometheus { prometheusSelector := ms.Spec.ResourceSelector - if prometheusSelector == nil { - prometheusSelector = &metav1.LabelSelector{} - } config := ms.Spec.PrometheusConfig diff --git a/pkg/controllers/monitoring/monitoring-stack/conditions.go b/pkg/controllers/monitoring/monitoring-stack/conditions.go index 0b6e8f47..1cfa2121 100644 --- a/pkg/controllers/monitoring/monitoring-stack/conditions.go +++ b/pkg/controllers/monitoring/monitoring-stack/conditions.go @@ -15,60 +15,78 @@ const ( PrometheusNotAvailable = "PrometheusNotAvailable" PrometheusNotReconciled = "PrometheusNotReconciled" PrometheusDegraded = "PrometheusDegraded" + ResourceSelectorIsNil = "ResourceSelectorNil" CannotReadPrometheusConditions = "Cannot read Prometheus status conditions" AvailableMessage = "Monitoring Stack is available" SuccessfullyReconciledMessage = "Monitoring Stack is successfully reconciled" + ResourceSelectorIsNilMessage = "No resources will be discovered, ResourceSelector is nil" + ResourceDiscoveryOnMessage = "Resource discovery is operational" NoReason = "None" ) -func updateConditions(msConditions []v1alpha1.Condition, prom monv1.Prometheus, generation int64, recError error) []v1alpha1.Condition { - if len(msConditions) == 0 { - return []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionUnknown, - Reason: NoReason, - LastTransitionTime: metav1.Now(), - }, - { - Type: v1alpha1.ReconciledCondition, - Status: v1alpha1.ConditionUnknown, - Reason: NoReason, - LastTransitionTime: metav1.Now(), - }, +func updateConditions(ms *v1alpha1.MonitoringStack, prom monv1.Prometheus, recError error) []v1alpha1.Condition { + return []v1alpha1.Condition{ + updateResourceDiscovery(ms), + updateAvailable(ms.Status.Conditions, prom, ms.Generation), + updateReconciled(ms.Status.Conditions, prom, ms.Generation, recError), + } +} + +func getMSCondition(conditions []v1alpha1.Condition, t v1alpha1.ConditionType) (v1alpha1.Condition, error) { + for _, c := range conditions { + if c.Type == t { + return c, nil } } - var updatedConditions []v1alpha1.Condition - - for _, mc := range msConditions { - switch mc.Type { - case v1alpha1.AvailableCondition: - available := updateAvailable(mc, prom, generation) - if !available.Equal(mc) { - available.LastTransitionTime = metav1.Now() - } - updatedConditions = append(updatedConditions, available) - case v1alpha1.ReconciledCondition: - reconciled := updateReconciled(mc, prom, generation, recError) - if !reconciled.Equal(mc) { - reconciled.LastTransitionTime = metav1.Now() - } - updatedConditions = append(updatedConditions, reconciled) + return v1alpha1.Condition{}, fmt.Errorf("condition type %v not found", t) +} + +// updateResourceDiscovery updates the ResourceDiscoveryCondition based on the +// ResourceSelector in the MonitorinStack spec. A ResourceSelector of nil causes +// the condition to be false, any other value sets the condition to true +func updateResourceDiscovery(ms *v1alpha1.MonitoringStack) v1alpha1.Condition { + if ms.Spec.ResourceSelector == nil { + return v1alpha1.Condition{ + Type: v1alpha1.ResourceDiscoveryCondition, + Status: v1alpha1.ConditionFalse, + Reason: ResourceSelectorIsNil, + Message: ResourceSelectorIsNilMessage, + LastTransitionTime: metav1.Now(), + ObservedGeneration: ms.Generation, + } + } else { + return v1alpha1.Condition{ + Type: v1alpha1.ResourceDiscoveryCondition, + Status: v1alpha1.ConditionTrue, + Reason: NoReason, + Message: ResourceDiscoveryOnMessage, + LastTransitionTime: metav1.Now(), + ObservedGeneration: ms.Generation, } } - return updatedConditions } // updateAvailable gets existing "Available" condition and updates its parameters // based on the Prometheus "Available" condition -func updateAvailable(ac v1alpha1.Condition, prom monv1.Prometheus, generation int64) v1alpha1.Condition { +func updateAvailable(conditions []v1alpha1.Condition, prom monv1.Prometheus, generation int64) v1alpha1.Condition { + ac, err := getMSCondition(conditions, v1alpha1.AvailableCondition) + if err != nil { + ac = v1alpha1.Condition{ + Type: v1alpha1.AvailableCondition, + Status: v1alpha1.ConditionUnknown, + Reason: NoReason, + LastTransitionTime: metav1.Now(), + } + } + prometheusAvailable, err := getPrometheusCondition(prom.Status.Conditions, monv1.PrometheusAvailable) if err != nil { ac.Status = v1alpha1.ConditionUnknown ac.Reason = PrometheusNotAvailable ac.Message = CannotReadPrometheusConditions + ac.LastTransitionTime = metav1.Now() return ac } // MonitoringStack status will not be updated if there is a difference between the Prometheus generation @@ -85,31 +103,43 @@ func updateAvailable(ac v1alpha1.Condition, prom monv1.Prometheus, generation in ac.Reason = PrometheusNotAvailable } ac.Message = prometheusAvailable.Message + ac.LastTransitionTime = metav1.Now() return ac } ac.Status = v1alpha1.ConditionTrue ac.Reason = AvailableReason ac.Message = AvailableMessage ac.ObservedGeneration = generation + ac.LastTransitionTime = metav1.Now() return ac } // updateReconciled updates "Reconciled" conditions based on the provided error value and // Prometheus "Reconciled" condition -func updateReconciled(rc v1alpha1.Condition, prom monv1.Prometheus, generation int64, err error) v1alpha1.Condition { - - if err != nil { +func updateReconciled(conditions []v1alpha1.Condition, prom monv1.Prometheus, generation int64, reconcileErr error) v1alpha1.Condition { + rc, cErr := getMSCondition(conditions, v1alpha1.ReconciledCondition) + if cErr != nil { + rc = v1alpha1.Condition{ + Type: v1alpha1.ReconciledCondition, + Status: v1alpha1.ConditionUnknown, + Reason: NoReason, + LastTransitionTime: metav1.Now(), + } + } + if reconcileErr != nil { rc.Status = v1alpha1.ConditionFalse - rc.Message = err.Error() + rc.Message = reconcileErr.Error() rc.Reason = FailedToReconcileReason + rc.LastTransitionTime = metav1.Now() return rc } - prometheusReconciled, err := getPrometheusCondition(prom.Status.Conditions, monv1.PrometheusReconciled) + prometheusReconciled, reconcileErr := getPrometheusCondition(prom.Status.Conditions, monv1.PrometheusReconciled) - if err != nil { + if reconcileErr != nil { rc.Status = v1alpha1.ConditionUnknown rc.Reason = PrometheusNotReconciled rc.Message = CannotReadPrometheusConditions + rc.LastTransitionTime = metav1.Now() return rc } @@ -121,12 +151,14 @@ func updateReconciled(rc v1alpha1.Condition, prom monv1.Prometheus, generation i rc.Status = prometheusStatusToMSStatus(prometheusReconciled.Status) rc.Reason = PrometheusNotReconciled rc.Message = prometheusReconciled.Message + rc.LastTransitionTime = metav1.Now() return rc } rc.Status = v1alpha1.ConditionTrue rc.Reason = ReconciledReason rc.Message = SuccessfullyReconciledMessage rc.ObservedGeneration = generation + rc.LastTransitionTime = metav1.Now() return rc } diff --git a/pkg/controllers/monitoring/monitoring-stack/conditions_test.go b/pkg/controllers/monitoring/monitoring-stack/conditions_test.go index 130dca3e..0f4ae1bd 100644 --- a/pkg/controllers/monitoring/monitoring-stack/conditions_test.go +++ b/pkg/controllers/monitoring/monitoring-stack/conditions_test.go @@ -9,59 +9,79 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestUpdateConditions(t *testing.T) { - transitionTime := metav1.Now() - +func TestUpdateAvailable(t *testing.T) { tt := []struct { - name string - originalMSConditions []v1alpha1.Condition - prometheus monv1.Prometheus - generation int64 - recError error - expectedResults []v1alpha1.Condition - // flag to compare LastTransitionTime of each condition - sameTransitionTimes bool + name string + prometheus monv1.Prometheus + previousConditions []v1alpha1.Condition + generation int64 + expectedResult v1alpha1.Condition }{ { - name: "empty conditions", - originalMSConditions: []v1alpha1.Condition{}, - generation: 1, - recError: nil, - prometheus: monv1.Prometheus{}, - expectedResults: []v1alpha1.Condition{ + name: "conditions not changed when Prometheus Available", + previousConditions: []v1alpha1.Condition{ { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionUnknown, - Reason: "None", + Type: v1alpha1.AvailableCondition, + Status: v1alpha1.ConditionTrue, + ObservedGeneration: 1, + Reason: AvailableReason, + Message: AvailableMessage, }, - { - Type: v1alpha1.ReconciledCondition, - Status: v1alpha1.ConditionUnknown, - Reason: "None", - }}, + }, + prometheus: monv1.Prometheus{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + Status: monv1.PrometheusStatus{ + Conditions: []monv1.PrometheusCondition{ + { + Type: monv1.PrometheusAvailable, + Status: monv1.PrometheusConditionTrue, + ObservedGeneration: 1, + }, + }}}, + generation: 1, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.AvailableCondition, + Status: v1alpha1.ConditionTrue, + ObservedGeneration: 1, + Reason: AvailableReason, + Message: AvailableMessage, + }, }, { - name: "conditions not changed when Prometheus Available", - originalMSConditions: []v1alpha1.Condition{ + name: "cannot read Prometheus conditions", + previousConditions: []v1alpha1.Condition{ { Type: v1alpha1.AvailableCondition, Status: v1alpha1.ConditionTrue, ObservedGeneration: 1, Reason: AvailableReason, Message: AvailableMessage, - LastTransitionTime: transitionTime, }, + }, + generation: 1, + prometheus: monv1.Prometheus{}, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.AvailableCondition, + Status: v1alpha1.ConditionUnknown, + ObservedGeneration: 1, + Reason: PrometheusNotAvailable, + Message: CannotReadPrometheusConditions, + }, + }, + { + name: "degraded Prometheus conditions", + previousConditions: []v1alpha1.Condition{ { - Type: v1alpha1.ReconciledCondition, + Type: v1alpha1.AvailableCondition, Status: v1alpha1.ConditionTrue, ObservedGeneration: 1, - Reason: ReconciledReason, - Message: SuccessfullyReconciledMessage, - LastTransitionTime: transitionTime, + Reason: AvailableReason, + Message: AvailableMessage, }, }, generation: 1, - recError: nil, prometheus: monv1.Prometheus{ ObjectMeta: metav1.ObjectMeta{ Generation: 1, @@ -70,205 +90,235 @@ func TestUpdateConditions(t *testing.T) { Conditions: []monv1.PrometheusCondition{ { Type: monv1.PrometheusAvailable, - Status: monv1.PrometheusConditionTrue, - ObservedGeneration: 1, - }, - { - Type: monv1.PrometheusReconciled, - Status: monv1.PrometheusConditionTrue, + Status: monv1.PrometheusConditionDegraded, ObservedGeneration: 1, }, }}}, - expectedResults: []v1alpha1.Condition{ + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.AvailableCondition, + Status: v1alpha1.ConditionFalse, + ObservedGeneration: 1, + Reason: PrometheusDegraded, + }, + }, + { + name: "Prometheus observed generation is different from the Prometheus generation", + previousConditions: []v1alpha1.Condition{ { Type: v1alpha1.AvailableCondition, Status: v1alpha1.ConditionTrue, - ObservedGeneration: 1, + ObservedGeneration: 2, Reason: AvailableReason, Message: AvailableMessage, - LastTransitionTime: transitionTime, }, + }, + generation: 1, + prometheus: monv1.Prometheus{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 3, + }, + Status: monv1.PrometheusStatus{ + Conditions: []monv1.PrometheusCondition{ + { + Type: monv1.PrometheusAvailable, + Status: monv1.PrometheusConditionFalse, + ObservedGeneration: 2, + }, + }}}, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.AvailableCondition, + Status: v1alpha1.ConditionTrue, + ObservedGeneration: 2, + Reason: AvailableReason, + Message: AvailableMessage, + }, + }, + } + + for _, test := range tt { + res := updateAvailable(test.previousConditions, test.prometheus, test.generation) + assert.Check(t, test.expectedResult.Equal(res), "%s - expected:\n %v\n and got:\n %v\n", test.name, test.expectedResult, res) + } +} + +func TestUpdateReconciled(t *testing.T) { + tt := []struct { + name string + prometheus monv1.Prometheus + previousConditions []v1alpha1.Condition + generation int64 + recError error + expectedResult v1alpha1.Condition + }{ + { + name: "conditions not changed when Prometheus Available", + previousConditions: []v1alpha1.Condition{ { Type: v1alpha1.ReconciledCondition, Status: v1alpha1.ConditionTrue, ObservedGeneration: 1, Reason: ReconciledReason, Message: SuccessfullyReconciledMessage, - LastTransitionTime: transitionTime, - }}, - sameTransitionTimes: true, + }, + }, + recError: nil, + generation: 1, + prometheus: monv1.Prometheus{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + Status: monv1.PrometheusStatus{ + Conditions: []monv1.PrometheusCondition{ + { + Type: monv1.PrometheusReconciled, + Status: monv1.PrometheusConditionTrue, + ObservedGeneration: 1, + }, + }}}, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.ReconciledCondition, + Status: v1alpha1.ConditionTrue, + ObservedGeneration: 1, + Reason: ReconciledReason, + Message: SuccessfullyReconciledMessage, + }, }, { name: "cannot read Prometheus conditions", - originalMSConditions: []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionTrue, - ObservedGeneration: 1, - Reason: AvailableReason, - Message: AvailableMessage, - LastTransitionTime: transitionTime, - }, + previousConditions: []v1alpha1.Condition{ { Type: v1alpha1.ReconciledCondition, Status: v1alpha1.ConditionTrue, ObservedGeneration: 1, Reason: ReconciledReason, Message: SuccessfullyReconciledMessage, - LastTransitionTime: transitionTime, }, }, - generation: 1, recError: nil, + generation: 1, prometheus: monv1.Prometheus{}, - expectedResults: []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionUnknown, - ObservedGeneration: 1, - Reason: PrometheusNotAvailable, - Message: CannotReadPrometheusConditions, - }, - { - Type: v1alpha1.ReconciledCondition, - Status: v1alpha1.ConditionUnknown, - ObservedGeneration: 1, - Reason: PrometheusNotReconciled, - Message: CannotReadPrometheusConditions, - }}, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.ReconciledCondition, + Status: v1alpha1.ConditionUnknown, + ObservedGeneration: 1, + Reason: PrometheusNotReconciled, + Message: CannotReadPrometheusConditions, + }, }, { name: "degraded Prometheus conditions", - originalMSConditions: []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionTrue, - ObservedGeneration: 1, - Reason: AvailableReason, - Message: AvailableMessage, - LastTransitionTime: transitionTime, - }, + previousConditions: []v1alpha1.Condition{ { Type: v1alpha1.ReconciledCondition, Status: v1alpha1.ConditionTrue, ObservedGeneration: 1, Reason: ReconciledReason, Message: SuccessfullyReconciledMessage, - LastTransitionTime: transitionTime, }, }, - generation: 1, recError: nil, + generation: 1, prometheus: monv1.Prometheus{ ObjectMeta: metav1.ObjectMeta{ Generation: 1, }, Status: monv1.PrometheusStatus{ Conditions: []monv1.PrometheusCondition{ - { - Type: monv1.PrometheusAvailable, - Status: monv1.PrometheusConditionDegraded, - ObservedGeneration: 1, - }, { Type: monv1.PrometheusReconciled, Status: monv1.PrometheusConditionDegraded, ObservedGeneration: 1, }, }}}, - expectedResults: []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionFalse, - ObservedGeneration: 1, - Reason: PrometheusDegraded, - }, - { - Type: v1alpha1.ReconciledCondition, - Status: v1alpha1.ConditionFalse, - ObservedGeneration: 1, - Reason: PrometheusNotReconciled, - }}, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.ReconciledCondition, + Status: v1alpha1.ConditionFalse, + ObservedGeneration: 1, + Reason: PrometheusNotReconciled, + }, }, { name: "Prometheus observed generation is different from the Prometheus generation", - originalMSConditions: []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionTrue, - ObservedGeneration: 2, - Reason: AvailableReason, - Message: AvailableMessage, - LastTransitionTime: transitionTime, - }, + previousConditions: []v1alpha1.Condition{ { Type: v1alpha1.ReconciledCondition, Status: v1alpha1.ConditionTrue, ObservedGeneration: 2, Reason: ReconciledReason, Message: SuccessfullyReconciledMessage, - LastTransitionTime: transitionTime, }, }, - generation: 2, recError: nil, + generation: 1, prometheus: monv1.Prometheus{ ObjectMeta: metav1.ObjectMeta{ Generation: 3, }, Status: monv1.PrometheusStatus{ Conditions: []monv1.PrometheusCondition{ - { - Type: monv1.PrometheusAvailable, - Status: monv1.PrometheusConditionFalse, - ObservedGeneration: 2, - }, { Type: monv1.PrometheusReconciled, Status: monv1.PrometheusConditionFalse, ObservedGeneration: 2, }, }}}, - expectedResults: []v1alpha1.Condition{ - { - Type: v1alpha1.AvailableCondition, - Status: v1alpha1.ConditionTrue, - ObservedGeneration: 2, - Reason: AvailableReason, - Message: AvailableMessage, - LastTransitionTime: transitionTime, - }, - { - Type: v1alpha1.ReconciledCondition, - Status: v1alpha1.ConditionTrue, - ObservedGeneration: 2, - Reason: ReconciledReason, - Message: SuccessfullyReconciledMessage, - LastTransitionTime: transitionTime, - }}, - sameTransitionTimes: true, + expectedResult: v1alpha1.Condition{ + Type: v1alpha1.ReconciledCondition, + Status: v1alpha1.ConditionTrue, + ObservedGeneration: 2, + Reason: ReconciledReason, + Message: SuccessfullyReconciledMessage, + }, }, } for _, test := range tt { - res := updateConditions(test.originalMSConditions, test.prometheus, test.generation, test.recError) - for _, c := range res { - expectedC := getConditionByType(test.expectedResults, c.Type) - assert.Check(t, expectedC.Equal(c), "%s - expected:\n %v\n and got:\n %v\n", test.name, expectedC, c) - if test.sameTransitionTimes { - assert.Check(t, expectedC.LastTransitionTime.Equal(&c.LastTransitionTime)) - } else { - assert.Check(t, c.LastTransitionTime.After(transitionTime.Time)) - } - } + res := updateReconciled(test.previousConditions, test.prometheus, test.generation, test.recError) + assert.Check(t, test.expectedResult.Equal(res), "%s - expected:\n %v\n and got:\n %v\n", test.name, test.expectedResult, res) } } -func getConditionByType(conditions []v1alpha1.Condition, ctype v1alpha1.ConditionType) *v1alpha1.Condition { - for _, c := range conditions { - if c.Type == ctype { - return &c - } +func TestUpdateResourceDiscovery(t *testing.T) { + transitionTime := metav1.Now() + tt := []struct { + name string + msWithConditions v1alpha1.MonitoringStack + expectedResults v1alpha1.Condition + }{ + { + name: "set resource discovery true when ResourceSelector not nil", + msWithConditions: v1alpha1.MonitoringStack{ + Spec: v1alpha1.MonitoringStackSpec{ + ResourceSelector: &metav1.LabelSelector{}, + }, + }, + expectedResults: v1alpha1.Condition{ + Type: v1alpha1.ResourceDiscoveryCondition, + Status: v1alpha1.ConditionTrue, + Reason: NoReason, + Message: ResourceDiscoveryOnMessage, + }, + }, + { + name: "set resource discovery false when ResourceSelector is nil", + msWithConditions: v1alpha1.MonitoringStack{ + Spec: v1alpha1.MonitoringStackSpec{ + ResourceSelector: nil, + }, + }, + expectedResults: v1alpha1.Condition{ + Type: v1alpha1.ResourceDiscoveryCondition, + Status: v1alpha1.ConditionFalse, + Reason: ResourceSelectorIsNil, + Message: ResourceSelectorIsNilMessage, + LastTransitionTime: transitionTime, + }, + }, + } + + for _, test := range tt { + res := updateResourceDiscovery(&test.msWithConditions) + assert.Check(t, test.expectedResults.Equal(res), "%s - expected:\n %v\n and got:\n %v\n", test.name, test.expectedResults, res) } - return nil + } diff --git a/pkg/controllers/monitoring/monitoring-stack/controller.go b/pkg/controllers/monitoring/monitoring-stack/controller.go index 4c80d98f..d811c73e 100644 --- a/pkg/controllers/monitoring/monitoring-stack/controller.go +++ b/pkg/controllers/monitoring/monitoring-stack/controller.go @@ -163,7 +163,7 @@ func (rm resourceManager) updateStatus(ctx context.Context, req ctrl.Request, ms logger.Info("Failed to get prometheus object", "err", err) return ctrl.Result{RequeueAfter: 2 * time.Second} } - ms.Status.Conditions = updateConditions(ms.Status.Conditions, prom, ms.Generation, recError) + ms.Status.Conditions = updateConditions(ms, prom, recError) err = rm.k8sClient.Status().Update(ctx, ms) if err != nil { logger.Info("Failed to update status", "err", err) diff --git a/test/e2e/monitoring_stack_controller_test.go b/test/e2e/monitoring_stack_controller_test.go index 17f98d52..f55d95fb 100644 --- a/test/e2e/monitoring_stack_controller_test.go +++ b/test/e2e/monitoring_stack_controller_test.go @@ -30,6 +30,8 @@ import ( "gotest.tools/v3/assert" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "k8s.io/client-go/kubernetes/scheme" ) type alert struct { @@ -43,6 +45,7 @@ func assertCRDExists(t *testing.T, crds ...string) { } func TestMonitoringStackController(t *testing.T) { + stack.AddToScheme(scheme.Scheme) assertCRDExists(t, "prometheuses.monitoring.rhobs", "alertmanagers.monitoring.rhobs", @@ -57,6 +60,9 @@ func TestMonitoringStackController(t *testing.T) { }, { name: "Empty stack spec must create a Prometheus", scenario: emptyStackCreatesPrometheus, + }, { + name: "resource selector nil propagates to Prometheus", + scenario: nilResrouceSelectorPropagatesToPrometheus, }, { name: "stack spec are reflected in Prometheus", scenario: reconcileStack, @@ -122,6 +128,35 @@ func emptyStackCreatesPrometheus(t *testing.T) { f.GetResourceWithRetry(t, ms.Name, ms.Namespace, &prometheus) } +func nilResrouceSelectorPropagatesToPrometheus(t *testing.T) { + ms := newMonitoringStack(t, "nil-selector") + err := f.K8sClient.Create(context.Background(), ms) + assert.NilError(t, err, "failed to create a monitoring stack") + f.AssertResourceEventuallyExists(ms.Name, ms.Namespace, &monv1.Prometheus{})(t) + + updatedMS := &stack.MonitoringStack{} + f.GetResourceWithRetry(t, ms.Name, ms.Namespace, updatedMS) + updatedMS.Spec.ResourceSelector = nil + err = f.K8sClient.Update(context.Background(), updatedMS) + assert.NilError(t, err, "failed to patch monitoring stack with nil resource selector") + + prometheus := monv1.Prometheus{} + err = wait.Poll(5*time.Second, wait.ForeverTestTimeout, func() (bool, error) { + if err := f.K8sClient.Get(context.Background(), types.NamespacedName{Name: updatedMS.Name, Namespace: updatedMS.Namespace}, &prometheus); errors.IsNotFound(err) { + return false, nil + } + + if prometheus.Spec.ServiceMonitorSelector != updatedMS.Spec.ResourceSelector { + return false, nil + } + return true, nil + }) + + if err == wait.ErrWaitTimeout { + t.Fatal(fmt.Errorf("nil ResourceSelector did not propagate to Prometheus object")) + } +} + func promConfigDefaultsAreApplied(t *testing.T) { tests := []struct { name string @@ -374,7 +409,7 @@ func assertPrometheusScrapesItself(t *testing.T) { err = f.StartServicePortForward("self-scrape-prometheus", e2eTestNamespace, "9090", stopChan) return err == nil, nil }); err != nil { - t.Fatal(err) + t.Fatal(fmt.Errorf("Failed to poll for port-forward: %w", err)) } promClient := framework.NewPrometheusClient("http://localhost:9090") @@ -408,7 +443,7 @@ func assertPrometheusScrapesItself(t *testing.T) { return correct == len(expectedResults), nil }); err != nil { - t.Fatal(err) + t.Fatal(fmt.Errorf("Could not query prometheus: %w", err)) } } @@ -626,10 +661,17 @@ func msNamespaceSelector(labels map[string]string) stackModifier { func newMonitoringStack(t *testing.T, name string, mods ...stackModifier) *stack.MonitoringStack { ms := &stack.MonitoringStack{ + TypeMeta: metav1.TypeMeta{ + APIVersion: stack.GroupVersion.String(), + Kind: "MonitoringStack", + }, ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: e2eTestNamespace, }, + Spec: stack.MonitoringStackSpec{ + ResourceSelector: &metav1.LabelSelector{}, + }, } for _, mod := range mods { mod(ms)