[prometheus-operator] chore: upgrade to v9.3.1 #743

samvantran · 2020-08-17T14:59:38Z

What type of PR is this?
Chore

What this PR does/ why we need it:
Upgrades prometheus-operator to v9.3.1

I used the upgrade scripts from PR #742 to generate these changes.

Which issue(s) this PR fixes:

Special notes for your reviewer:
Right now this PR includes changes from PR #742. Please review that one first.

Does this PR introduce a user-facing change?:

[prometheus-operator] chore: upgrade to v9.3.1

Checklist

The commit message explains the changes and why are needed.
The code builds and passes lint/style checks locally.
The relevant subset of integration tests pass locally.
The core changes are covered by tests.
The documentation is updated where needed.

alejandroEsc

This is a lot to review, for the most part lgtm, just a few comments, nothing major.

alejandroEsc · 2020-08-17T18:50:14Z

staging/prometheus-operator/templates/prometheus/ingressperreplica.yaml

@@ -6,7 +6,7 @@ apiVersion: v1
 kind: List
 metadata:
  name: {{ include "prometheus-operator.fullname" $ }}-prometheus-ingressperreplica
-  namespace: {{ $.Release.Namespace }}
+  namespace: {{ template "prometheus-operator.namespace" $ }}


namespace: {{ template "prometheus-operator.namespace" . }}now we have $ is this a consistency issue?

upstream 🤷‍♂️ https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/prometheus/ingressperreplica.yaml#L9

alejandroEsc · 2020-08-17T18:50:46Z

staging/prometheus-operator/templates/prometheus/psp.yaml

@@ -3,7 +3,6 @@ apiVersion: policy/v1beta1
 kind: PodSecurityPolicy
 metadata:
  name: {{ template "prometheus-operator.fullname" . }}-prometheus
-  namespace: {{ $.Release.Namespace }}


we do not need namespace here?

same as upstream: https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/prometheus/psp.yaml#L5-L7

that said, here is the history for why they removed it helm/charts@30b43ea

staging/prometheus-operator/patch/mesosphere/templates/rules/etcd-rules.yaml

staging/prometheus-operator/patch/mesosphere/templates/rules/velero-rules.yaml

staging/prometheus-operator/templates/_helpers.tpl

alejandroEsc · 2020-08-17T18:54:14Z

staging/prometheus-operator/templates/alertmanager/ingressperreplica.yaml

@@ -17,7 +17,7 @@ items:
    {{ end -}}
    metadata:
      name: {{ include "prometheus-operator.fullname" $ }}-alertmanager-{{ $i }}
-      namespace: {{ $.Release.Namespace }}
+      namespace: {{ template "prometheus-operator.namespace" $ }}


consistency here? should use . insteady of $?

this is just a copy from upstream: https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/alertmanager/ingressperreplica.yaml#L9-L20

If you look at the list of commits, starting from chore: copy upstream chart version: 9.3.1 each one applies a set of patches that are different from upstream. Everything else is a direct copy from helm/charts.

that said, do we want to fix it/change it? Is it required? If so I can work on a patch file but not sure we really need to

alejandroEsc · 2020-08-17T18:55:09Z

staging/prometheus-operator/templates/alertmanager/psp.yaml

@@ -3,7 +3,6 @@ apiVersion: policy/v1beta1
 kind: PodSecurityPolicy
 metadata:
  name: {{ template "prometheus-operator.fullname" . }}-alertmanager
-  namespace: {{ $.Release.Namespace }}


namespace not needed? This would be installed in kubeaddons in helm3 is this intentional?

Looks like they took it out upstream: https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/alertmanager/psp.yaml#L5-L7

Do we need it back? If so, I can add another patch to the set in PR #742

gracedo · 2020-08-17T19:11:49Z

Here's a diff I ran against upstream so it's easier to see what changes we've applied on top of upstream
https://gist.github.com/gracedo/004a491ed6ee3557aa032fd0c5f37ad4

samvantran

Thanks for the review @alejandroEsc! I'll add in the needed new lines but for the other comments you pointed out, it's all the same as the upstream chart. Should we fix in here or try to fix upstream? Do we even need to?

samvantran · 2020-08-17T20:26:27Z

staging/prometheus-operator/templates/alertmanager/ingressperreplica.yaml

@@ -17,7 +17,7 @@ items:
    {{ end -}}
    metadata:
      name: {{ include "prometheus-operator.fullname" $ }}-alertmanager-{{ $i }}
-      namespace: {{ $.Release.Namespace }}
+      namespace: {{ template "prometheus-operator.namespace" $ }}


this is just a copy from upstream: https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/alertmanager/ingressperreplica.yaml#L9-L20

If you look at the list of commits, starting from chore: copy upstream chart version: 9.3.1 each one applies a set of patches that are different from upstream. Everything else is a direct copy from helm/charts.

that said, do we want to fix it/change it? Is it required? If so I can work on a patch file but not sure we really need to

samvantran · 2020-08-17T22:04:38Z

staging/prometheus-operator/templates/alertmanager/psp.yaml

@@ -3,7 +3,6 @@ apiVersion: policy/v1beta1
 kind: PodSecurityPolicy
 metadata:
  name: {{ template "prometheus-operator.fullname" . }}-alertmanager
-  namespace: {{ $.Release.Namespace }}


Looks like they took it out upstream: https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/alertmanager/psp.yaml#L5-L7

Do we need it back? If so, I can add another patch to the set in PR #742

samvantran · 2020-08-17T22:24:41Z

staging/prometheus-operator/templates/prometheus/ingressperreplica.yaml

@@ -6,7 +6,7 @@ apiVersion: v1
 kind: List
 metadata:
  name: {{ include "prometheus-operator.fullname" $ }}-prometheus-ingressperreplica
-  namespace: {{ $.Release.Namespace }}
+  namespace: {{ template "prometheus-operator.namespace" $ }}


upstream 🤷‍♂️ https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/prometheus/ingressperreplica.yaml#L9

samvantran · 2020-08-17T22:26:09Z

staging/prometheus-operator/templates/prometheus/psp.yaml

@@ -3,7 +3,6 @@ apiVersion: policy/v1beta1
 kind: PodSecurityPolicy
 metadata:
  name: {{ template "prometheus-operator.fullname" . }}-prometheus
-  namespace: {{ $.Release.Namespace }}


same as upstream: https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/prometheus/psp.yaml#L5-L7

that said, here is the history for why they removed it helm/charts@30b43ea

alejandroEsc · 2020-08-18T21:44:09Z

I am good to merge whenever you are, thanks!

…w changes)

…pstream chart version: 9.3.1 * chore: apply patch_1_mesosphere_dashboards.sh * chore: apply patch_2_mesosphere_cron_jobs.sh * chore: apply patch_3_mesosphere_hooks.sh * chore: apply patch_4_ingress_rbac.sh * chore: apply patch_5_mesosphere_rules.sh * chore: apply patch_6_mesosphere_values.sh * chore: apply patch_7_prometheus_crd.sh - reformat yaml with yq (no new changes) * chore: apply patch_7_prometheus_crd.sh - update volumeClaimTemplate * chore: apply patch_8_prometheus_operator_crd.sh

…: copy upstream chart version: 9.3.1 * chore: apply patch_1_mesosphere_dashboards.sh * chore: apply patch_2_mesosphere_cron_jobs.sh * chore: apply patch_3_mesosphere_hooks.sh * chore: apply patch_4_ingress_rbac.sh * chore: apply patch_5_mesosphere_rules.sh * chore: apply patch_6_mesosphere_values.sh * chore: apply patch_7_prometheus_crd.sh - reformat yaml with yq (no new changes) * chore: apply patch_7_prometheus_crd.sh - update volumeClaimTemplate * chore: apply patch_8_prometheus_operator_crd.sh" This reverts commit b88c046.

* Revert "[prometheus-operator] chore: upgrade to v9.3.1 (#743) * chore: copy upstream chart version: 9.3.1 * chore: apply patch_1_mesosphere_dashboards.sh * chore: apply patch_2_mesosphere_cron_jobs.sh * chore: apply patch_3_mesosphere_hooks.sh * chore: apply patch_4_ingress_rbac.sh * chore: apply patch_5_mesosphere_rules.sh * chore: apply patch_6_mesosphere_values.sh * chore: apply patch_7_prometheus_crd.sh - reformat yaml with yq (no new changes) * chore: apply patch_7_prometheus_crd.sh - update volumeClaimTemplate * chore: apply patch_8_prometheus_operator_crd.sh" This reverts commit b88c046. * Revert "[prometheus-operator] feat: add scripts to make upgrading easier (#742) * chore: move all mesosphere files to patch This moves all the mesosphere-related templates to the patch folder. This makes it easier to update the operator with a copy and replace as none of the mesosphere files will get lost. Signed-off-by: Sam Tran <[email protected]> * feat: add patch files This adds patch files that copy all the mesosphere files into the templates/ folder. This way they get deployed as part of the chart. It makes it easier to keep all the patches in one place and know what changes are needed/differ from upstream. Signed-off-by: Sam Tran <[email protected]> * feat: add helpers script for git commands This adds a helper file so that all the patch files can use the git_add_and_commit function which adds and commits all the files that were changed as part of the patch script. This should make it easy to see the commit history of what changed. Signed-off-by: Sam Tran <[email protected]> * chore: update prometheus crd patch script This commit updates the patch script to use a docker image as the previous code did not run on OSX. Also, due to the underlying go-parser lib used in the yq docker image, updating a yaml file automatically reformats the yaml removing empty newlines and converting multiline strings into one line[1]. This makes it hard to see the actual needed change for patch7 which is just to update the crd volumeClaimTemplate.metadata stanza. Thus, patch7 is changed to apply two commits, one to simply let yq format the file with no new changes and a second commit with the needed change. Similar to what we do with chart revision changes. Also updated the helper script to accept a commit message. [1] mikefarah/yq#465 (comment) Signed-off-by: Sam Tran <[email protected]> * feat: add update_operator script This script does a sparse clone of helm/stable/prometheus-operator and replaces all the files in our fork with latest upstream. Then it applies all the patches in the /patch folder which should now make updating our prometheus chart a bit easier. Signed-off-by: Sam Tran <[email protected]> * chore: add newlines from PR review Signed-off-by: Sam Tran <[email protected]> * chore: add doc for upgrading + patch comments Add an UPGRADING doc so users know what to do to upgrade. Add comments to the patch scripts so users know what each one does. crd patch script needed explaining. Signed-off-by: Sam Tran <[email protected]> * chore: add patch for checking CRD exists again This adds back the check for that was removed from upstream. Signed-off-by: Sam Tran <[email protected]> * chore: fixup shellcheck errors Co-authored-by: Joe Julian <[email protected]>" This reverts commit 54b4815.

This reverts commit 9a2b458.

* Revert "[prometheus-operator] chore: upgrade to v9.3.1 (#743)" This reverts commit 9a2b458. * Revert "[prometheus-operator] feat: add scripts to make upgrading easier (#742)" This reverts commit dcdf015. * chore: Bump chart version

* Revert [prometheus-operator] chore: upgrade to v9.3.1 (#743) This reverts commit b88c046. * Revert [prometheus-operator] feat: add scripts to make upgrading easier (#742) This reverts commit 54b4815.

samvantran requested a review from a team August 17, 2020 14:59

alejandroEsc approved these changes Aug 17, 2020

View reviewed changes

samvantran commented Aug 17, 2020

View reviewed changes

samvantran force-pushed the svt/update-prom-931 branch from 67028e6 to 37ae575 Compare August 18, 2020 00:14

samvantran requested review from branden and gracedo August 18, 2020 01:24

shaneutt assigned samvantran Aug 18, 2020

shaneutt added the ready ready label Aug 18, 2020

gracedo approved these changes Aug 18, 2020

View reviewed changes

branden approved these changes Aug 18, 2020

View reviewed changes

samvantran added 10 commits August 18, 2020 16:08

chore: copy upstream chart version: 9.3.1

a6da190

chore: apply patch_1_mesosphere_dashboards.sh

8cf8602

chore: apply patch_2_mesosphere_cron_jobs.sh

0afbc1f

chore: apply patch_3_mesosphere_hooks.sh

2fde738

chore: apply patch_4_ingress_rbac.sh

8c470d2

chore: apply patch_5_mesosphere_rules.sh

d64f052

chore: apply patch_6_mesosphere_values.sh

f043f86

chore: apply patch_7_prometheus_crd.sh - reformat yaml with yq (no ne…

ecbbdc7

…w changes)

chore: apply patch_7_prometheus_crd.sh - update volumeClaimTemplate

c04cdef

chore: apply patch_8_prometheus_operator_crd.sh

8de4138

samvantran force-pushed the svt/update-prom-931 branch from e5d5bb5 to 8de4138 Compare August 18, 2020 23:08

samvantran merged commit 9a2b458 into master Aug 18, 2020

samvantran deleted the svt/update-prom-931 branch August 18, 2020 23:31

gracedo added a commit to gracedo/charts that referenced this pull request Aug 19, 2020

Revert "[prometheus-operator] chore: upgrade to v9.3.1 (mesosphere#743)"

3a2e1fd

This reverts commit 9a2b458.

samvantran mentioned this pull request Aug 19, 2020

[prometheus-operator] chore: upgrade to v9.3.1 and patch scripts #752

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[prometheus-operator] chore: upgrade to v9.3.1 #743

[prometheus-operator] chore: upgrade to v9.3.1 #743

samvantran commented Aug 17, 2020

alejandroEsc left a comment

alejandroEsc Aug 17, 2020

samvantran Aug 17, 2020

alejandroEsc Aug 17, 2020

samvantran Aug 17, 2020

alejandroEsc Aug 17, 2020

samvantran Aug 17, 2020

alejandroEsc Aug 17, 2020

samvantran Aug 17, 2020

gracedo commented Aug 17, 2020

samvantran left a comment

samvantran Aug 17, 2020

samvantran Aug 17, 2020

samvantran Aug 17, 2020

samvantran Aug 17, 2020

alejandroEsc commented Aug 18, 2020

[prometheus-operator] chore: upgrade to v9.3.1 #743

[prometheus-operator] chore: upgrade to v9.3.1 #743

Conversation

samvantran commented Aug 17, 2020

alejandroEsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gracedo commented Aug 17, 2020

samvantran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandroEsc commented Aug 18, 2020