Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Label service-monitors so that they are discovered by Prometheus #200

Merged
merged 16 commits into from
Mar 26, 2020

Conversation

surajssd
Copy link
Member

This PR adds labels to the ServiceMonitors that are shipped by default by the components. And few components were missing the config service_monitor bool which is added.

The components that ship ServiceMonitor and are labelled as follows.

  • MetalLB
  • Contour
  • Velero
  • Cert Manager
  • Cluster Autoscaler
  • External DNS

@surajssd
Copy link
Member Author

I would like help testing three components if they are creating ServiceMonitors:

  • Velero
  • Cluster Autoscaler
  • External DNS

@surajssd
Copy link
Member Author

TODOs:

  • Add tests to verify that the corresponding component's metrics are being scraped and available in Prometheus. But before that I would really like to get refactor prometheus test code #185

@surajssd surajssd requested a review from rata March 19, 2020 16:27
@@ -88,6 +96,9 @@ type component struct {
ScaleDownUnreadyTime time.Duration
ScaleDownUnreadyTimeRaw string `hcl:"scale_down_unready_time,optional"`

// Prometheus parameters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Prometheus parameters
// Prometheus parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

none of the comments above have full stops, is it an uneven thing to add full stop only here?

Copy link
Member

Choose a reason for hiding this comment

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

Make sure that the ServiceMonitor is selected to be monitored by the
prometheus operator shipped with Lokomotive.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Make sure that the ServiceMonitors created for contour are selected by
the prometheus operator shipped with Lokomotive.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Make sure that the ServiceMonitor created for velero is selected by the
prometheus operator shipped with Lokomotive.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
* This variable will enable or disable creation of `ServiceMonitor` for
cert-manager.

* This commit also adds a label makes sure that the `ServiceMonitor`
created for cert-manager is selected by the prometheus operator shipped
with Lokomotive.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
* This variable will enable or disable creation of `ServiceMonitor` for
cluster autoscaler.

* This commit also adds a label makes sure that the `ServiceMonitor`
created for cluster autoscaler is selected by the prometheus operator
shipped with Lokomotive.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
* This variable will enable or disable creation of `ServiceMonitor` for
external DNS.

* This commit also adds a label makes sure that the `ServiceMonitor`
created for external DNS is selected by the prometheus operator
shipped with Lokomotive.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd surajssd force-pushed the surajssd/update-servicemonitors branch 4 times, most recently from df60a91 to c66eadd Compare March 24, 2020 11:03
Copy link
Member

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

test/monitoring/components_metrics_test.go Outdated Show resolved Hide resolved
test/monitoring/components_metrics_test.go Outdated Show resolved Hide resolved
@surajssd surajssd force-pushed the surajssd/update-servicemonitors branch 4 times, most recently from 3299255 to 5150f00 Compare March 25, 2020 07:24
@surajssd surajssd force-pushed the surajssd/update-servicemonitors branch 2 times, most recently from 9e91375 to f866828 Compare March 25, 2020 10:59
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

In general, looks OK.

}

// The platform should be present in the list of supported platforms.
if strings.Contains(platforms, platform) {
Copy link
Member

Choose a reason for hiding this comment

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

What about packet and packer-arm? If run on packet and test is only intended to run on packet-arm, it will be incorrectly triggered.

Copy link
Member

Choose a reason for hiding this comment

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

Also it could be just return strings.Contains(platforms, platform).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is possible, but all the tests that run on packet-arm run on packet. It is super set of the other.

Copy link
Member

Choose a reason for hiding this comment

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

This may produce unexpected behavior in some cases, that's not good :/

{
componentName: "contour",
query: "contour_dagrebuild_timestamp",
platforms: "packet,aws",
Copy link
Member

Choose a reason for hiding this comment

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

Why not a slice or map?

Copy link
Member Author

Choose a reason for hiding this comment

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

This way it is easier for the user writing test.

Copy link
Member

Choose a reason for hiding this comment

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

We could use strings for all values when programming, but it's generally not a good idea 😄

Makefile Show resolved Hide resolved
iaguis
iaguis previously approved these changes Mar 25, 2020
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

This commit adds a function which verifies if the platform on which this
test is running is supported to run test.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
This exports a variable for the post e2e tests. This variable specifies
the current platform which will be used to detect if the corresponding
component test we are running is supposed to be run on that platform or
not.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Install components in following order, mandatorily, to ensure that other
components can install Prometheus Operator CRs like ServiceMonitor and
PrometheusRule:

- OpenEBS
- OpenEBS Storage Class
- Prometheus Operator

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Install components in following order, mandatorily, to ensure that other
components can install Prometheus Operator CRs like ServiceMonitor and
PrometheusRule:

- OpenEBS
- OpenEBS Storage Class
- Prometheus Operator

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
Copy link
Member Author

@iaguis @invidian can I get a LGTM again?

Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

@surajssd
Copy link
Member Author

surajssd commented Mar 26, 2020

This is protected hence I can't merge it so either @invidian or @ipochi also has to review and approve it.

Copy link
Member

@ipochi ipochi left a comment

Choose a reason for hiding this comment

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

lgtm

@invidian invidian dismissed their stale review March 26, 2020 13:33

All changes has been addressed.

@surajssd surajssd merged commit f95732b into master Mar 26, 2020
@surajssd surajssd deleted the surajssd/update-servicemonitors branch March 26, 2020 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants