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.

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

Enforce parity between Jsonnet and Helm #2067

Merged
merged 42 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
e671948
Add kustomize plugins for config and k8s defaults
krajorama Jul 21, 2022
6f9abe6
Add kustomize config for removing diffs
Logiraptor Jun 22, 2022
0aeded8
Add github action for comparing manifests
Logiraptor Jun 22, 2022
6c98b66
Pin yq action to a trusted commit
Logiraptor Jun 22, 2022
73a2f59
Add dependencies to makefile target
Logiraptor Jun 22, 2022
cc5b46b
Fix diff invocation in script
Logiraptor Jun 22, 2022
dc6c932
Use SM patch instead of JSONPatch for compactor service
Logiraptor Jun 22, 2022
c8cc1ea
Add metadata.managedFields to list of ignored fields
Logiraptor Jun 22, 2022
f268896
Set PDB api version
Logiraptor Jun 22, 2022
0df3976
Fix jsonnet lint
Logiraptor Jun 22, 2022
01a9213
Fix golang lint
Logiraptor Jun 22, 2022
f2e233a
Add docs for using the comparison tool
krajorama Jul 21, 2022
5a8c03f
Fix goimports
Logiraptor Jun 22, 2022
dcf0bc7
fix docs lint
Logiraptor Jun 23, 2022
f151034
Add tests for resolve-config, address PR feedback
Logiraptor Jun 23, 2022
dbf2e1a
Update compare-helm-with-jsonnet to run against all pull requests
Logiraptor Jun 23, 2022
98e2741
Update kustomizations for memcached refactor
Logiraptor Jun 23, 2022
d857eef
Fix doc lint
Logiraptor Jun 23, 2022
ab77f28
Add License header
Logiraptor Jun 24, 2022
f390a3e
Address PR feedback
Logiraptor Jul 20, 2022
cc27eba
Fix import formatting
Logiraptor Jul 20, 2022
e765462
Update memcached naming to match recent changes
Logiraptor Jul 20, 2022
206f9f6
Update rendered jsonnet
Logiraptor Jul 20, 2022
6e100d8
Update kustomization for jsonnet changes
Logiraptor Jul 20, 2022
cb01036
Apply suggestions from code review
Logiraptor Jul 21, 2022
c56197a
remove pdb apiVersion hack
Logiraptor Jul 21, 2022
afd04f4
Merge remote-tracking branch 'origin/main' into logiraptor/use-k8s-diff
Logiraptor Jul 21, 2022
04315b8
Add setup-tanka
Logiraptor Jul 21, 2022
5b15138
Set BUILD_IN_CONTAINER=false
Logiraptor Jul 21, 2022
46af55a
Use mimir-build-image
Logiraptor Jul 21, 2022
f259797
Remove all container logic
Logiraptor Jul 21, 2022
56f9ed5
Use v 1.0.0-alpha.1 of setup-tanka
Logiraptor Jul 21, 2022
90fdb13
Merge branch 'main' into logiraptor/use-k8s-diff
Logiraptor Jul 21, 2022
fb0afd2
Update jsonnet tests
Logiraptor Jul 21, 2022
02141ad
Download tanka directly
Logiraptor Jul 21, 2022
a0016d7
Add executable permissions for tanka
Logiraptor Jul 21, 2022
e3b5ab8
Fix typo in github action
Logiraptor Jul 21, 2022
9b05369
Add jsonnet-bundler
Logiraptor Jul 21, 2022
e11cc38
Comment on failed PRs instead of blocking the merge
Logiraptor Jul 22, 2022
e18b3f7
Merge branch 'main' into logiraptor/use-k8s-diff
Logiraptor Jul 22, 2022
4e63d26
Update kustomization for podTopologySpreadConstraints
Logiraptor Jul 22, 2022
3ef8e5e
Fix duplicate comments
Logiraptor Jul 22, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
127 changes: 127 additions & 0 deletions .github/workflows/compare-helm-with-jsonnet.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
name: compare-helm-with-jsonnet

on: pull_request

concurrency:
# Cancel any running workflow for the same branch when new commits are pushed.
# We group both by ref_name (available when CI is triggered by a push to a branch/tag)
# and head_ref (available when CI is triggered by a PR).
group: "${{ github.workflow }}-${{ github.ref_name }}-${{ github.head_ref }}"
cancel-in-progress: true

jobs:
compare-manifests:
runs-on: ubuntu-latest
steps:
- name: Find Comment
uses: peter-evans/find-comment@v2
id: fc
with:
issue-number: ${{ github.event.pull_request.number }}
comment-author: 'github-actions[bot]'
body-includes: '**Helm <> Jsonnet Diff**'

- name: Create or update comment
uses: peter-evans/create-or-update-comment@v2
id: progress-comment
with:
comment-id: ${{ steps.fc.outputs.comment-id }}
issue-number: ${{ github.event.pull_request.number }}
body: |
**Helm <> Jsonnet Diff**

:hourglass_flowing_sand: Automated comparison in progress: https://github.com/${{github.repository}}/actions/runs/${{github.run_id}}

This process will check for any unexpected differences between the Helm chart and the Jsonnet library.
edit-mode: replace

- uses: actions/checkout@v2
- uses: helm/kind-action@v1.2.0
- uses: frenck/action-setup-yq@a2ad11c46c5d7ba576861216963c9365b53f35bc
- uses: dsaltares/fetch-gh-release-asset@d9376dacd30fd38f49238586cd2e9295a8307f4c
with:
repo: 'grafana/tanka'
version: 'tags/v0.22.1'
file: 'tk-linux-amd64'
target: 'bin/tk'
token: ${{ secrets.GITHUB_TOKEN }}
- uses: dsaltares/fetch-gh-release-asset@d9376dacd30fd38f49238586cd2e9295a8307f4c
with:
repo: 'jsonnet-bundler/jsonnet-bundler'
version: 'tags/v0.5.1'
file: 'jb-linux-amd64'
target: 'bin/jb'
token: ${{ secrets.GITHUB_TOKEN }}
- run: |
set -e
chmod +x $PWD/bin/tk
chmod +x $PWD/bin/jb
echo $PWD/bin >> $GITHUB_PATH

- id: compare-manifests
run: |
set +e
# Make dependencies first so their output doesn't appear in the PR comment
make operations/helm/charts/mimir-distributed/charts
make build-jsonnet-tests

OUTPUT="$(./operations/compare-helm-with-jsonnet/compare-helm-with-jsonnet.sh 2>&1)"

if [ $? -eq 0 ]; then
echo "::set-output name=changed::false"
else
echo "$OUTPUT"
# This makes a multiline output work
# https://trstringer.com/github-actions-multiline-strings/
OUTPUT="${OUTPUT//'%'/'%25'}"
OUTPUT="${OUTPUT//$'\n'/'%0A'}"
OUTPUT="${OUTPUT//$'\r'/'%0D'}"
echo "::set-output name=changed::true"
echo "::set-output name=diff::$OUTPUT"
fi

- name: Create or update comment
uses: peter-evans/create-or-update-comment@v2
if: ${{ steps.compare-manifests.outputs.changed == 'true' }}
with:
comment-id: ${{ steps.progress-comment.outputs.comment-id }}
issue-number: ${{ github.event.pull_request.number }}
body: |
**Helm <> Jsonnet Diff**

:warning: A difference was detected between the Helm chart and the Jsonnet library.

1. Use `make check-helm-jsonnet-diff` to reproduce the output locally.
2. This test is experimental while we gather feedback about its usefulness.
3. It does not block your PR from being merged, but we would appreciate you trying to keep feature parity between the Helm chart and Jsonnet library if possible.

**If you get stuck on this step and the Mimir maintainers aren't able to help, feel free to merge without making this step pass and tag `@Logiraptor` so the Mimir maintainers can gather feedback later.**

[Please see the contribution docs here for more info.](https://github.com/grafana/mimir/blob/main/docs/internal/contributing/contributing-to-helm-chart.md)

<details>

<summary>
Expand to see the output
</summary>

Output of https://github.com/${{github.repository}}/actions/runs/${{github.run_id}}

```diff
${{ steps.compare-manifests.outputs.diff }}
```

</details>
edit-mode: replace

- name: Create or update comment
uses: peter-evans/create-or-update-comment@v2
if: ${{ steps.compare-manifests.outputs.changed == 'false' }}
with:
comment-id: ${{ steps.progress-comment.outputs.comment-id }}
issue-number: ${{ github.event.pull_request.number }}
body: |
**Helm <> Jsonnet Diff**

:tada: No differences where detected between Helm and Jsonnet :tada:
edit-mode: replace
8 changes: 7 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,13 @@ check-jsonnet-getting-started:
| sed 's/\(jb install github.com\/grafana\/mimir\/operations\/mimir@main\)/\1 \&\& rm -fr .\/vendor\/mimir \&\& cp -r ..\/operations\/mimir .\/vendor\/mimir\//g' \
| bash

build-helm-tests:
operations/helm/charts/mimir-distributed/charts: operations/helm/charts/mimir-distributed/Chart.yaml operations/helm/charts/mimir-distributed/Chart.lock
@cd ./operations/helm/charts/mimir-distributed && helm dependency update

check-helm-jsonnet-diff: operations/helm/charts/mimir-distributed/charts build-jsonnet-tests
@./operations/compare-helm-with-jsonnet/compare-helm-with-jsonnet.sh

build-helm-tests: operations/helm/charts/mimir-distributed/charts
@./operations/helm/tests/build.sh

check-helm-tests: build-helm-tests
Expand Down
78 changes: 78 additions & 0 deletions docs/internal/contributing/contributing-to-helm-chart.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,81 @@ Install [ct](https://github.com/helm/chart-testing) and run
```bash
ct lint --config operations/helm/ct.yaml --charts operations/helm/charts/mimir-distributed
```

## Automated comparison with Jsonnet

In order to prevent configuration drift between the Mimir jsonnet library and the Mimir helm chart, an automated diff is performed against every pull request.
This diff makes extensive use of [kustomize](https://kustomize.io) to remove unimportant or known differences between the two sets of manifests.
A custom kustomize function is used to extract the Mimir configuration from kubernetes manifests so that it can also be compared.
The end goal is to ensure that only "useful" differences appear in the diff output.
Deciding which differences are useful is a complicated topic, but at a high level we use the following heuristics:

- Differences in Kubernetes annotations and labels are typically not useful, since changes in those will appear in other tests (ie golden record, functionality, etc).
- Differences in configuration parameters related to urls and file paths are typically not interesting, since they don't change _what_ the cluster does, only _where_ it happens.
- Differences in performance or scale related properties are typically useful, since these often have difficult-to-test implications on the cluster.

In order to keep the kustomize configuration manageable, it is divided into overlays, each named based on the order they are applied.

For example, at the time of writing, the Helm chart is passed through the following overlays:

```
$ ls -1 operations/compare-helm-with-jsonnet/helm
00-base
01-ignore
02-configs-and-k8s-defaults
03-set-namespace
04-labels
05-memberlist
06-memcached
07-services
08-pods
09-config
```

Each directory contains a `kustomize.yaml` file that describes the transformations made in that overlay.
A full explanation of kustomize is outside the scope of this document, but generally the overlays do one or all of the following:

1. Remove properties that are not useful for diffing
2. Modify property values so that they match between Helm and Jsonnet
3. Remove entire objects that only exist in either Helm or Jsonnet

### make check-helm-jsonnet-diff

You can use the `make check-helm-jsonnet-diff` target to perform an automatic diff of the Helm and Jsonnet templates.
This target requires the following:

- `yq`, `kubectl`, and `kustomize` installed and on the PATH
- A running kubernetes API server selected as the currently active `kubectl` context.

The API server is only used to perform dry-runs of server-side apply.
No resources are actually created in kubernetes, but API calls will be made against the server.
It is recommended to use kind, k3d, or some other local development cluster.
This allows the final diff to ignore any fields that match kubernetes defaults.
Ideally, there will be no differences between helm and jsonnet, so it's expected that the kustomize configuration will shrink over time rather than grow.
Achieving perfect parity is difficult, so this process allows us to incrementally fix differences while also preventing new differences from being added.

If CI reports a difference, you have several options:

1. Modify the Helm chart or Jsonnet library such that the differences are no longer present
2. Modify the Helm values file or Jsonnet configuration such that the differences are no longer present
3. If the difference is not useful, modify the kustomize configuration to remove that field or otherwise patch the manifests such that the differences are no longer present
Logiraptor marked this conversation as resolved.
Show resolved Hide resolved

### operations/compare-helm-with-jsonnet/compare-kustomize-outputs.sh

There is another script, `operations/compare-helm-with-jsonnet/compare-kustomize-outputs.sh`, which can be used to compare any two overlays in the diffing process.
This can be a useful tool when debugging kustomize configuration.

For example, the following invocation will show only changes that have been applied by the 9th helm overlay.

```
cd operations/compare-helm-with-jsonnet
./compare-kustomize-outputs.sh ./helm/08-* ./helm/09-*
```

The output can be filtered further by supplying a [yq](https://mikefarah.gitbook.io/yq/operators/select) `select` expression.
This is useful to limit otherwise noisy output to only show objects of a certain kind, name, or other property.

```
cd operations/compare-helm-with-jsonnet
./compare-kustomize-outputs.sh ./helm/09-* ./jsonnet/09-* 'select(.kind == "StatefulSet")'
```
11 changes: 11 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ require (
go.opentelemetry.io/collector/pdata v0.54.0
golang.org/x/sys v0.0.0-20220627191245-f75cf1eec38b
gopkg.in/alecthomas/kingpin.v2 v2.2.6
sigs.k8s.io/kustomize/kyaml v0.13.7
)

require (
Expand Down Expand Up @@ -115,6 +116,7 @@ require (
github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb // indirect
github.com/fatih/color v1.13.0 // indirect
github.com/felixge/httpsnoop v1.0.3 // indirect
github.com/go-errors/errors v1.0.1 // indirect
github.com/go-logfmt/logfmt v0.5.1 // indirect
github.com/go-logr/logr v1.2.3 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
Expand All @@ -133,6 +135,7 @@ require (
github.com/golang-jwt/jwt/v4 v4.4.1 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/google/btree v1.0.1 // indirect
github.com/google/gnostic v0.5.7-v3refs // indirect
github.com/google/go-cmp v0.5.8 // indirect
github.com/google/go-querystring v1.1.0 // indirect
github.com/google/pprof v0.0.0-20220608213341-c488b8fa1db3 // indirect
Expand All @@ -155,6 +158,7 @@ require (
github.com/hashicorp/go-uuid v1.0.2 // indirect
github.com/hashicorp/memberlist v0.3.1 // indirect
github.com/hashicorp/serf v0.9.6 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/jessevdk/go-flags v1.5.0 // indirect
github.com/jmespath/go-jmespath v0.4.0 // indirect
github.com/josharian/intern v1.0.0 // indirect
Expand All @@ -176,6 +180,7 @@ require (
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f // indirect
github.com/ncw/swift v1.0.52 // indirect
github.com/oklog/run v1.1.0 // indirect
Expand All @@ -191,10 +196,13 @@ require (
github.com/shurcooL/httpfs v0.0.0-20190707220628-8d4bc4ba7749 // indirect
github.com/shurcooL/vfsgen v0.0.0-20200824052919-0d455de96546 // indirect
github.com/spaolacci/murmur3 v1.1.0 // indirect
github.com/spf13/cobra v1.4.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/stretchr/objx v0.4.0 // indirect
github.com/uber/jaeger-lib v2.4.1+incompatible // indirect
github.com/vimeo/galaxycache v0.0.0-20210323154928-b7e5d71c067a // indirect
github.com/weaveworks/promrus v1.2.0 // indirect
github.com/xlab/treeprint v1.1.0 // indirect
go.etcd.io/etcd v3.3.25+incompatible // indirect
go.etcd.io/etcd/api/v3 v3.5.4 // indirect
go.etcd.io/etcd/client/pkg/v3 v3.5.4 // indirect
Expand Down Expand Up @@ -223,6 +231,9 @@ require (
google.golang.org/protobuf v1.28.0 // indirect
gopkg.in/ini.v1 v1.66.4 // indirect
gopkg.in/telebot.v3 v3.0.0 // indirect
k8s.io/kube-openapi v0.0.0-20220401212409-b28bf2818661 // indirect
k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 // indirect
sigs.k8s.io/yaml v1.3.0 // indirect
)

// Override since git.apache.org is down. The docs say to fetch from github.
Expand Down
Loading