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

Grafana: Prepare chart for 5.1.x #5576

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@sylr
Copy link
Contributor

sylr commented May 15, 2018

When upgrading to 5.1.x we need to chown the whole /var/lib/grafana dir in order to comply with the new grafana user id used in the latest docker image.

@sylr sylr force-pushed the sylr:grafana branch May 15, 2018

@sylr sylr changed the title Grfana: Prepare chart for 5.1.x Grafana: Prepare chart for 5.1.x May 15, 2018

@sylr

This comment has been minimized.

Copy link
Contributor

sylr commented May 15, 2018

/assign unguiculus

stable/grafana/templates/deployment.yaml Outdated
- chown
- -R
- 472:472
- /var/lib/grafana

This comment has been minimized.

@christian-roggia

christian-roggia May 15, 2018

Collaborator

I would use command: ["chown", "-R", "472:472", "/var/lib/grafana"] here.

This comment has been minimized.

@sylr

sylr May 15, 2018

Contributor

All right

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 15, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sylr
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: unguiculus

Assign the PR to them by writing /assign @unguiculus in a comment when ready.

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:

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

@unguiculus

This comment has been minimized.

Copy link
Member

unguiculus commented May 16, 2018

#5386 updated Grafana to 5.1.2. I think this PR is now obsolete.

@sylr

This comment has been minimized.

Copy link
Contributor

sylr commented May 16, 2018

It's not if you think about upgrading your helm installed grafana from anything below 5.1.0.

If the permissions are wrong grafana will crash.

It's why I did this.

@sylr sylr force-pushed the sylr:grafana branch May 16, 2018

@OndroNR

This comment has been minimized.

Copy link

OndroNR commented May 16, 2018

I wanted to deploy grafana with clean persistent volume and encountered this issue. I made one-run Job, based on init container in this PR, to fix ownerhip. This PR is still needed.

@sylr

This comment has been minimized.

Copy link
Contributor

sylr commented May 17, 2018

Could we merge this ASAP for people like @OndroNR that are going to upgrade grafana and be stuck.

@OndroNR

This comment has been minimized.

Copy link

OndroNR commented May 17, 2018

I was doing clean install with enabled persistence.

@unguiculus

This comment has been minimized.

Copy link
Member

unguiculus commented May 17, 2018

/ok-to-test

stable/grafana/templates/deployment.yaml Outdated
subPath: {{ .Values.persistence.subPath }}
{{- /* Since grafana 5.1.0 the files need to be owned by user id 472 */ -}}
{{- /* http://docs.grafana.org/installation/docker/#migration-from-a-previous-version-of-the-docker-container-to-5-1-or-later */ -}}
{{- if semverCompare ">= 5.1.0" .Values.image.tag }}

This comment has been minimized.

@unguiculus

unguiculus May 17, 2018

Member

This is not ideal. If you use a custom image that has a different versioning scheme, this won't work. I think it would be best to make this optional with a flag.

This comment has been minimized.

@sylr

sylr May 18, 2018

Contributor

I prefer to let it like this, if you want to use custom images you should tag them correctly.

@FrederikNS

This comment has been minimized.

Copy link

FrederikNS commented May 18, 2018

As I mentioned in #5635, I also just ran into the permission problem with persistent volumes.

Grafana: Prepare chart for 5.1.x
When upgrading to 5.1.x we need to chown the whole /var/lib/grafana
dir in order to comply with the new grafana user id used in the latest
docker image.
@scottrigby

This comment has been minimized.

Copy link
Member

scottrigby commented May 19, 2018

Hi everyone, after a Charts maintainers conversation, the root of the problem appears to be that semver was not properly followed in #5386 in a way that was non-obvious:

  • The MINOR version was bumped (from 1.8.0 to 1.9.0) which made sense, because a new feature was added (RBAC)
  • However, the image and app version was also bumped (from 5.0.4 to 5.1.2), which is the Grafana version that introduced backwards incompatible changes. Even though Grafana version did not follow semver here, the chart should.
    • Note that while our automated do semver comparison, AFAIK we don't explicitly mention semver as a requirement anywhere except in the Un-deprecating A Chart section of our PROCESSES doc

Given that we accidentally missed this in #5386, we should change this PR from attempting to automatically handle this change, to adding a note to the user that if upgrading from < 1.9.0, to run a k8s-specific command (kubectl exec ... -it chown -R 472:472 /var/lib/grafana), and link to the Grafana documentation page on this in the same note rather than in an automated template step.

Additionally this PR should address the earlier oversight by properly bumping the MAJOR version (to 2.0.0).

Let's use #5657 for tracking creating and documenting the charts-wide policy for handling cases like this. Practically, the question for this PR now is where to communicate the above info to upgrading users so that they're less likely to miss it? I'm thinking maybe a new Upgrading section just above Installing the chart?

@willejs

This comment has been minimized.

Copy link
Contributor

willejs commented May 22, 2018

Would setting the fsGroup in securitycontext of the pod fix this for new installs?

@thomasbarton

This comment has been minimized.

Copy link

thomasbarton commented May 29, 2018

currently stable/grafana is broken when doing a clean install with a persistent volume. This pr fixes the issue. Any update on merging?

@xmartinez

This comment has been minimized.

Copy link

xmartinez commented Jun 8, 2018

@scottrigby Not sure the semver reasoning applies to this issue. This is broken for fresh installs, where backwards compatibility consideration do not apply (there is no previous install to be compatible with).

For the sake of anyone encountering this issue, the following workaround can be applied:

kubectl create --filename=- <<'EOF'
apiVersion: batch/v1
kind: Job
metadata: {name: grafana-chown}
spec:
  template:
    spec:
      restartPolicy: Never
      containers:
      - name: grafana-chown
        command: [chown, -R, "472:472", /var/lib/grafana]
        image: busybox:latest
        volumeMounts:
        - {name: storage, mountPath: /var/lib/grafana}
      volumes:
      - name: storage
        persistentVolumeClaim:
          claimName: grafana
EOF

Note that kubectl exec cannot be used, because the container is crash-looping on startup, so it does not live long enough to be able to run any command.

@monotek

This comment has been minimized.

Copy link
Contributor

monotek commented Jul 1, 2018

@sylr
could you resolve the merge conflicts please?

@sylr

This comment has been minimized.

Copy link
Contributor

sylr commented Jul 2, 2018

@monotek AFAIU This is not going to be merged.

@monotek

This comment has been minimized.

Copy link
Contributor

monotek commented Jul 2, 2018

ok, thanks for the info anyway :-)

@immunda

This comment has been minimized.

Copy link

immunda commented Jul 5, 2018

This was a real pain to get to the bottom of and is still broken on fresh installs as outlined above.

I independently came up with the same initContainer solution as the only viable option after exploring fsGroup for the pod's security context.

Any chance of a revisit @scottrigby?

@stale

This comment has been minimized.

Copy link

stale bot commented Aug 8, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Activing will cause the issue to no longer be considered stale. Thank you for your contributions.

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