Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup thanos ruler #700

Merged
merged 2 commits into from Mar 16, 2020
Merged

Cleanup thanos ruler #700

merged 2 commits into from Mar 16, 2020

Conversation

simonpasquier
Copy link
Contributor

@simonpasquier simonpasquier commented Mar 12, 2020 •
edited

This needs #696 first.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 12, 2020
@s-urbaniak
Copy link
Contributor

looking good so far 👍 needs a rebase

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2020
@simonpasquier simonpasquier changed the title [WIP] Cleanup thanos ruler Cleanup thanos ruler Mar 13, 2020
@openshift-ci-robot openshift-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 13, 2020
@simonpasquier
Copy link
Contributor Author

/test images

@simonpasquier
Copy link
Contributor Author

/restest

lilic
lilic reviewed Mar 16, 2020
View reviewed changes
assets/thanos-ruler/service-monitor.yaml Outdated
@@ -2,17 +2,18 @@ apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
labels:
k8s-app: alertmanager
k8s-app: thanos
Copy link
Contributor

Choose a reason for hiding this comment

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

should this not be thanos-ruler instead?

jsonnet/thanos-ruler.jsonnet Outdated
@@ -191,19 +158,18 @@ local authorizationRole =
name: 'thanos-ruler',
namespace: thanosRulerConfig.namespace,
labels: {
'k8s-app': 'alertmanager',
'k8s-app': 'thanos',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@lilic
Copy link
Contributor

lilic commented Mar 16, 2020 

    user_workload_monitoring_test.go:92: timed out waiting for the condition: statefulsets.apps "prometheus-user-workload" not found

Seems like a valid failure. 🤔

@simonpasquier
jsonnet,pkg: add {kube-rbac,prom-label}-proxy to Thanos ruler
779d22a 
This change also fixes a few glitches in the service and service monitor
definitions.
@simonpasquier
assets,pkg/manifests: regenerate
2ae2ecb 
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier
Copy link
Contributor Author

@lilic yes the operator was using the now removed asset.

@lilic
Copy link
Contributor

lilic commented Mar 16, 2020

/retest

Unrelated failure.

lilic
lilic reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilic, simonpasquier

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:
  • OWNERS [lilic,simonpasquier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c3245b9 into openshift:master Mar 16, 2020
@simonpasquier simonpasquier deleted the cleanup-thanos-ruler branch March 16, 2020 16:06
pgier added a commit to pgier/cluster-monitoring-operator that referenced this pull request Apr 6, 2020
@pgier
pkg/manifests: remove thanos ruler rbac proxy secret
f862b1e 
This is no longer needed after cleanup in openshift#700
@pgier pgier mentioned this pull request Apr 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilic lilic lilic left review comments

@pgier pgier Awaiting requested review from pgier

@squat squat Awaiting requested review from squat

Assignees

@lilic lilic

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@simonpasquier @s-urbaniak @lilic @openshift-ci-robot @openshift-merge-robot