feat(chart): added ability to configure persistence

[gedimin45]

In the future IMO Deis could use https://github.com/kubernetes/charts/tree/master/stable/redis but the problem is that chart requires to password-protect Redis which is unnecessary IMO.

[deis-admin]

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

[deis-bot]

@kmala is a potential reviewer of this pull request based on my analysis of git blame information. Thanks @Ged15!

[rimusz]

you need to change commit message to something like feat(chart): add optional persistent storage as per styele giude

[rimusz]

also storageClass is missing in values.yaml file

[rimusz]

Support for the volume.alpha.kubernetes.io/storage-class is deprecated in kubernetes v1.4 so there are no needs to have it in the pvc template

[gedimin45]

Took a better look at this and it might make more sense to just use Redis from https://github.com/kubernetes/charts/tree/master/stable/redis. Would the Deis team accept this kind of change?

[rimusz]

I think it is better to use the deis redis :)
You should also remove emptyDir() e6ff26e#diff-acc297c1c8ac259658cb1dd1ce5d0a28R49
Take a look to my PR deis/monitor#169 I did for influxdb/grafana persistent storage.

[bacongobbler]

We would accept a PR to use the kubernetes chart as long as everything's backwards compatible. In fact, we would prefer that.

[gedimin45]

I didn't come up with a way to disable deployment when off-cluster is set as Redis location with the chart from stable/redis.
@rimusz I fixed the PR according to your comments. Just did not remove emptyDir: {}. I cannot wrap the volumeMounts field of the container in the same conditional as volumes of the deployment as in your PR - the container has other mounts.

[rimusz]

LGTM, ok and emptyDir: {} can stay will not make any harm
also you need to change commit message to something like feat(chart): add optional persistent storage

[gedimin45]

@rimusz I did :)

[bacongobbler]

I didn't come up with a way to disable deployment when off-cluster is set as Redis location with the chart from stable/redis.

Just as an idea, you could contribute a new feature to helm that would allow you to do selective disabling of the chart: helm/helm#1568 (comment)

[gedimin45]

Is there anything else I can do to get this in?

[vdice]

I'll test this PR today as I'll be doing similar for the monitor and workflow charts... I'll let @rimusz and @bacongobbler hash out any remaining code/design questions in the meantime, if any.

[vdice]

Jenkins, add to whitelist.

[vdice]

@Ged15 I'm unable to verify log persistence, or perhaps I am missing something. I see the pv,pvc resources being added and it appears the volume mount is set up properly; yet I don't see the mounted volume getting log data or logs persisting through pod destruction:

$ helm install workflow-pr/workflow/ --namespace deis --set redis.persistence.enabled=true
...

$ kd get pv,pvc
NAME                                          CAPACITY   ACCESSMODES   RECLAIMPOLICY   STATUS    CLAIM                    REASON    AGE
pv/pvc-311ae7d6-e296-11e6-9a8a-42010af000ec   2Gi        RWO           Delete          Bound     deis/deis-logger-redis             15m

NAME                    STATUS    VOLUME                                     CAPACITY   ACCESSMODES   AGE
pvc/deis-logger-redis   Bound     pvc-311ae7d6-e296-11e6-9a8a-42010af000ec   2Gi        RWO           15m

$ kd exec deis-logger-redis-1864853955-nq69r -it -- sh -c 'df -h'
Filesystem      Size  Used Avail Use% Mounted on
...
/dev/sdb        2.0G  6.0M  1.8G   1% /var/lib/redis
...

$ deis create myapp --no-remote
Creating Application... done, created myapp
If you want to add a git remote for this app later, use `deis git:remote -a myapp`

$ deis logs -a myapp
2017-01-25T00:38:16+00:00 deis[controller]: INFO config myapp-16e3c82 updated
2017-01-25T00:38:16+00:00 deis[controller]: INFO v created initial release
2017-01-25T00:38:16+00:00 deis[controller]: INFO appsettings myapp-f292132 updated
2017-01-25T00:38:16+00:00 deis[controller]: INFO domain myapp added

$ kd exec deis-logger-redis-1864853955-nq69r -it -- sh -c 'ls /var/lib/redis'
lost+found

$ kd delete po deis-logger-redis-1864853955-nq69r
pod "deis-logger-redis-1864853955-nq69r" deleted

$ deis logs -a myapp
Error: There are currently no log messages. Please check the following things:
1) Logger and fluentd pods are running: kubectl --namespace=deis get pods.
2) The application is writing logs to the logger component by checking that an entry in the ring buffer was created: kubectl --namespace=deis logs <logger pod>
3) Making sure that the container logs were mounted properly into the fluentd pod: kubectl --namespace=deis exec <fluentd pod> ls /var/log/containers
3a) If the above command returns saying /var/log/containers cannot be found then please see the following github issue for a workaround: https://github.com/deis/logger/issues/50

[mboersma]

This still needs some work to address problems that @vdice was seeing. I'm going to reset its review status and move it to the next milestone since v2.12 is otherwise about ready to roll.

[bacongobbler]

ping @Ged15, any news on whether you'll be continuing to work on this?

[gedimin45]

@bacongobbler sorry for the inactivity. I might take a crack at it this weekend. If I do not, feel free to close this.

[bacongobbler]

I'm going to close this ticket due to inactivity, but please re-open if this still needs to be addressed. Thanks!