Add service monitor to Thanos Query #100
Conversation
Signed-off-by: Coleen Iona Quadros <[email protected]>
Signed-off-by: Coleen Iona Quadros <[email protected]>
Signed-off-by: Coleen Iona Quadros <[email protected]>
Signed-off-by: Coleen Iona Quadros <[email protected]>
coleenquadros
changed the title
coleenquadros
force-pushed
the
add_service_monitor
branch
2 times, most recently
from
54b2ff9
to
142ce7e
Compare
Signed-off-by: Coleen Iona Quadros <[email protected]>
coleenquadros
force-pushed
the
add_service_monitor
branch
from
b52194f
to
4d73886
Compare
|
@philipgough @saswatamcode its ready for review. I cannot add you as reviewers. |
philipgough
requested review from
saswatamcode and
philipgough
and removed request for
saswatamcode
| // Enable self monitoring for the Thanos component. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default:=true | ||
| EnableSelfMonitor *bool `json:"enableSelfMonitor,omitempty"` |
There was a problem hiding this comment.
interested to hear with @saswatamcode thinks, but I think this should be its own struct because there are certain requirements for how service monitors can be created and where they should live to make this useful (thinking rhobs for example).
It could look something like this
type ServiceMonitorConfig struct {
// default true
Enabled
Labels map[string]string
// Can leave this out for now but it might be required or we can just hard default to current for now
Namespace
//
}
There was a problem hiding this comment.
Let's have it in separate struct and maybe as part of commonthanosfields?
| CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, | ||
| CRDDirectoryPaths: []string{ | ||
| filepath.Join("..", "..", "config", "crd", "bases"), | ||
| filepath.Join("..", "..", "test", "configs", "service-monitor.yaml"), |
There was a problem hiding this comment.
i think i misled you here because i thought that we already had those resources locally :/
a better test indeed might be in e2e - we could simply, in the first step, check against the prometheus to see if it reports the up metric for the component.
after deleting the SM we can just check that its gone. My apologies there I didnt check thoroughly enough the structure
There was a problem hiding this comment.
I think it is fine to add the test here, but I wonder if we need to include CRD this way 🤔
@coleenquadros, I think the Environment struct has a CRDs field that can take in a Go CustomResourceDefinition struct. I wonder if we can import that struct from Prometheus Operator somehow? That way we don't need to maintain the CRD in-tree
There was a problem hiding this comment.
maintaining the CRD in-tree is what I have the issue with. Ideally we would keep the integration tests as light as possible and enable running locally with out any network for example, as they are now.
Why I am suggesting adding this to e2e now instead is because we already take care of installing Prom operator there from my understanding and we can push much of the slower e2e work to ci. Thoughts?
There was a problem hiding this comment.
Yup that works too, essentially skip testing this in integration.
If CRD isn't really available in Go way, makes sense to skip it. But I think Prometheus Operator may have some method that spits out Go CRD struct. We can then just use that?
| @@ -75,6 +75,10 @@ type CommonThanosFields struct { | |||
| // +kubebuilder:default:=logfmt | |||
| // +kubebuilder:validation:Optional | |||
| LogFormat *string `json:"logFormat,omitempty"` | |||
| // Enable self monitoring for the Thanos component. | |||
There was a problem hiding this comment.
Let's also mention Prometheus Operator version number in comment here?
| CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, | ||
| CRDDirectoryPaths: []string{ | ||
| filepath.Join("..", "..", "config", "crd", "bases"), | ||
| filepath.Join("..", "..", "test", "configs", "service-monitor.yaml"), |
There was a problem hiding this comment.
I think it is fine to add the test here, but I wonder if we need to include CRD this way 🤔
@coleenquadros, I think the Environment struct has a CRDs field that can take in a Go CustomResourceDefinition struct. I wonder if we can import that struct from Prometheus Operator somehow? That way we don't need to maintain the CRD in-tree
| return errors.IsNotFound(err) | ||
| } | ||
|
|
||
| func QueryPrometheus(query string) (*PrometheusResponse, error) { |
There was a problem hiding this comment.
Thanks for adding this! Maybe for future, but we can probably copy some e2e thanos methods that do similar, if we want to check metrics.
For now I think this works well!
| @@ -566,3 +582,48 @@ func VerifyCfgMapOrSecretEnvVarExists(c client.Client, expectResource ExpectApiR | |||
| return false | |||
| } | |||
| } | |||
|
|
|||
| func ApplyPrometheusCRD() error { | |||
There was a problem hiding this comment.
Refer to comment above, but I think InstallPrometheusOperator method, already provides us with the CRD?
There was a problem hiding this comment.
Right, this is for the prometheus object. But in that case, can we just define it in Go and apply? instead of having it as YAML.
I would avoid having definitions for tests in YAML as much as possible here, as things might change between versions which lead to confusing broken tests
No description provided.