Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

Conversation

@den-is
Copy link
Contributor

@den-is den-is commented Oct 26, 2019

What this PR does / why we need it:

  • First of all this PR has synced all rules, alerts and dashboards definitions from the upstream
  • Updated sync scripts in the hack directory

Which issue this PR fixes

Special notes for your reviewer:

Kubernetes / Networking / Cluster
Kubernetes / Networking / Namespace (Pods) 
Kubernetes / Networking / Namespace (Workload) 
Kubernetes / Networking / Pod 
Kubernetes / Networking / Workload 
  • Also now you have to install grafana plugin for this Networking dashboards to display correctly
  plugins:
  - grafana-piechart-panel

Checklist

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/mychartname])

@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Oct 26, 2019
@helm-bot helm-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 26, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @den-is. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vsliouniaev
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 26, 2019
@den-is
Copy link
Contributor Author

den-is commented Oct 26, 2019

preparing fix for networking dashboards in this chart.
hopefully someone fixes upstream too

@vsliouniaev
Copy link
Collaborator

@vsliouniaev
Copy link
Collaborator

@den-is At least some of the rules in this PR are invalid. I just tried installing this into an existing cluster with the validating webhooks enabled, which prevented the rules from being applied

@den-is
Copy link
Contributor Author

den-is commented Oct 27, 2019

@vsliouniaev
list of broken rules ? what k8s version? where can I find/enable validation webhooks ?

In addition, I'm reverting coreos/kube-prometheus release from 0.2 to 0.1 - that was false upgrade breaking compatibility with k8s <1.14

@den-is
Copy link
Contributor Author

den-is commented Oct 27, 2019

hopefully garbage cleanup after previous false-upgrade helps situation a little bit more for those who run k8s v<1.14 .. other than that I can't see other issue.

I had once question to community:
Right now I've "manually" removed upper limit for max k8s version which was '1.16.0-0'.
Nasty solution.
So what is better: set it to some higher version like 6.6.6 or maybe completely remove upper bound for resources which are intended for k8s v>1.14+

@vsliouniaev
Copy link
Collaborator

It looks like the issue is being caused by the use of the humanizePercentage function in some of the templates. This was added in https://github.com/prometheus/prometheus/blob/master/CHANGELOG.md#2110--2019-07-09 and will need to get support from Prometheus-Operator validating hooks first, otherwise anyone deploying this update with the webhooks enabled will get this error reported and the upgrade will fail

@den-is
Copy link
Contributor Author

den-is commented Oct 27, 2019

Unfortunately I don't have enough experience with admission webhooks in k8s in general.

After reading docs for "couple minutes" I still can't understand how abstract-for-k8s string value with ... | humanizePercentage or whatever else string is validated.
I will try to do older release upgrade test tomorrow.

Not to hold this PR, any contribution help will be highly appreciated.

@hatemosphere
Copy link

@vsliouniaev should we just wait until this one prometheus-operator/prometheus-operator#2779 goes to the next prometheus-operator release? it should also cover humanizePercentage that was introduced in 2.11

@vsliouniaev
Copy link
Collaborator

@hatemosphere Yes, this should be quite easy to solve. I've opened this issue: prometheus-operator/prometheus-operator#2837

@helm-bot helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Oct 29, 2019
@zanhsieh
Copy link
Collaborator

@den-is
Would you mind to resolve the conflicting file please? Thank you.
stable/prometheus-operator/Chart.yaml

@vsliouniaev
Copy link
Collaborator

Going to try get another PR in to fix more test flakes #18430

@den-is
Copy link
Contributor Author

den-is commented Oct 30, 2019

Would like to hear any ideas if how it is possible to automate migration/fix of string from upstream which already contains backticks

Current manual workflow in 17340c2 :

  • split string in multiple raw strings `text`
  • add string representaion of backtick "`"
  • use print function to output final string {{ print `str1` "`" "str2" "`" }}

Proposal: Probably it is possible to have list of such exceptional strings or rule names. Catch them during iteration. and just completely replace them with hardcoded stored values for the given key rather than just running them through:

def escape(s):
    return s.replace("{{", "{{`{{").replace("}}", "}}`}}")

Because, Imo it is almost impossible to correctly determine where to insert:

  • {{print `{{
  • ` "`" ` x2
  • }}`}}
  • while having above "dumb" escape(s)

PS: Just kinda don't want to create mega monster parser :D

@vsliouniaev
Copy link
Collaborator

@den-is What if you change the sync process around?
Instead of producing helm rules resource templates you instead produce prometheus rules files and just leave them on the file system?You can then use helm globbing to find all the files.

You'd still have to do some substitutions for job name and namespace but that may be a simpler undertaking because you can treat the content as a string

@den-is
Copy link
Contributor Author

den-is commented Oct 31, 2019

that's better to happen in individual PR

@vsliouniaev
Copy link
Collaborator

Feel free to do that separately or all together. It seems like the current merge process is blocked cos we need to get another operator release out anyway

@vsliouniaev
Copy link
Collaborator

yea, just upgrading operator image to 0.34 didn't work

I have tested your changes against 0.34.0 and they work, but there is a caveat:

You cannot do it simultaneously with applying rules because Helm applies the changes to the cluster all at once and the validation is still occurring with the old image. For this reason. the 0.34.0 change will be made on version 7.x.x of the chart and the new rules will be marked as 8.0.0. That way we can signal to users that they must jump through some hoops by upgrading to the latest 7.x.x version, then go to 8, rather than going straight to 8.

@vsliouniaev
Copy link
Collaborator

Updated images are merged

@zanhsieh
Copy link
Collaborator

zanhsieh commented Nov 3, 2019

@den-is
Need another fix for the conflicting file.
stable/prometheus-operator/Chart.yaml

@den-is
Copy link
Contributor Author

den-is commented Nov 3, 2019

@zanhsieh you don't say.

@vsliouniaev
Copy link
Collaborator

@den-is Could you please add a section to the README.md file, under https://github.com/helm/charts/tree/master/stable/prometheus-operator#upgrading-an-existing-release-to-a-new-major-version, describing how someone can successfully upgrade to 8.x.x correctly by upgrading to the latest 7.x.x first (currently 7.4.0), and then proceeding on to 8?

@vsliouniaev
Copy link
Collaborator

/lgtm

(but needs a DCO fix)

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 4, 2019
author Denis Iskandarov <d.iskandarov@gmail.com> 1572070164 +0400
committer Denis Iskandarov <d.iskandarov@gmail.com> 1572862763 +0400

parent 88dbc36
author Denis Iskandarov <d.iskandarov@gmail.com> 1572070164 +0400
committer Denis Iskandarov <d.iskandarov@gmail.com> 1572862723 +0400

parent 88dbc36
author Denis Iskandarov <d.iskandarov@gmail.com> 1572070164 +0400
committer Denis Iskandarov <d.iskandarov@gmail.com> 1572861866 +0400

added pip requirements.txt to make setup more clear

Signed-off-by: Denis Iskandarov <d.iskandarov@gmail.com>
Signed-off-by: Denis Iskandarov <d.iskandarov@gmail.com>
Signed-off-by: Denis Iskandarov <d.iskandarov@gmail.com>
Signed-off-by: Denis Iskandarov <d.iskandarov@gmail.com>
@vsliouniaev
Copy link
Collaborator

/lgtm
Thanks a lot @den-is !

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: den-is, vsliouniaev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@den-is den-is closed this Nov 4, 2019
@den-is den-is reopened this Nov 4, 2019
@vsliouniaev
Copy link
Collaborator

/retest all

@nmaupu
Copy link
Contributor

nmaupu commented Nov 4, 2019

Hi,

Is it possible to add the missing label for this PR to be merged ?
@vsliouniaev
Thanks 👍

@vsliouniaev
Copy link
Collaborator

I asked in #charts on the Kubernetes slack about that. I'm afraid I'm only a collaborator and don't have rights to do that myself

@vsliouniaev
Copy link
Collaborator

@den-is since this seems not to be quick, mind just closing this PR and reopening a new one? I don't get why the bot isn't adding the label

@den-is
Copy link
Contributor Author

den-is commented Nov 4, 2019

Going to close that that PR and create duplicate which hopefully will work as intended.

@den-is den-is closed this Nov 4, 2019
@den-is
Copy link
Contributor Author

den-is commented Nov 4, 2019

Fresh duplicate PR here #18560

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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. ok-to-test size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[stable/prometheus-operator] prometheus rules not properly synced with kube-prometheus

7 participants