Add helm chart #59
Add helm chart #59
Conversation
|
|
swapnilgm
added
kind/enhancement
| "Failed") | ||
| continue;; | ||
| "Successful") | ||
| exec etcd --data-dir=/var/etcd/data \ |
There was a problem hiding this comment.
I was anyways thinking of using --config-file parameter pointing to etcd.conf.yaml which looks better. So lets add another confimap data field called etcd.conf.yaml and move the parameters there. We will probably mount this file under /var/etcd/config. Also, bootstrap.sh file under /var/etcd/bin
There was a problem hiding this comment.
I agree on using etcd.conf.yaml is a better approach and I was actually about to compleet this change request. But I just realised that this file's structure seems to be in sync with current examples. Once we have the chart in the repo, we'll need to update also the chart with each change in the examples.
Doing the suggested changes will diverge the structure in the examples, and will make the chart relatively harder to maintain in long term. Rather than having chart best practices, at this stage it seems to be more important to me to keep the chart maintenance more hassle-free in short term. We can start doing such improvements after seeing the chart does worth spending such maintenance overhead. I'm just concerned about keeping the chart up-to-date as the project evolves.
Here are the recent changes happened in the examples directory, there seems to be regular updates on examples for a while, i'm not sure if that will change soon:
$ git lg example/
* 6712697 - Update examples (4 months ago) <Swapnil Mhamane>
* c45e7b3 - Update changelog (5 months ago) <Swapnil Mhamane>
* ae89094 - Add generate-example script (5 months ago) <Swapnil Mhamane>
* 7711680 - Fix to make sure etcd is not running when the backup container is failing to take a backup to cloud. Doesnt need to restart the backup container but keep retrying instead keeping etcd down. (5 months ago) <I308301>
* 32a226b - TLS Support for snapshotter. (6 months ago) <I308301>
* 4d03f04 - Fix: check member directory instead of data directory. Cannot delete data directory as its the mount point. (6 months ago) <I308301>
* 779f6b3 - Changed data directory deletion to delete contents in the directory instead of the directory. (6 months ago) <I308301>
* 926f0ad - Remove dependency of gcs snapstore on project-id enviornment variable (7 months ago) <Swapnil Mhamane>
* ddd683c - GCS object store support. (7 months ago) <I308301>
* a1301a5 - Add store prefix option (7 months ago) <Swapnil Mhamane>
* b7e95fc - Asynchronize http call for initialization (7 months ago) <Swapnil Mhamane>
* 0e0ad3a - Expose etcd initialiation functionaliy over http (7 months ago) <Swapnil Mhamane>
* c4449af - Adding comments in example statefulset yaml. (8 months ago) <I308301>
* 7bb28ba - Integrating restore code. (8 months ago) <I308301>
* 52ed7bd - Refactor code to have single root command (8 months ago) <Swapnil Mhamane>
* e8bb35b - Add restore support (8 months ago) <Swapnil Mhamane>
Would you still prefer me to proceed with this change?
There was a problem hiding this comment.
Oh, i see the examples are generated from hack/templates/etcd-statefulset.yaml.tpl, I think it would be even better to make this file a synchronized structural copy of it.
There was a problem hiding this comment.
Once we moved to chart, I don't feel need of maintaining separate example yamls. You can simply remove those. So, in my opinion you can go ahead and use etcd.conf.yaml.
There was a problem hiding this comment.
Cool, thanks for the clarification, i'll follow up.
There was a problem hiding this comment.
Thank you for the changes. I have suggested some more changes, following the document. PTAL. We will incorporate template structural changes in gardener as well. So don't worry about diverging from gardener :).
Please add a support for no-SSL option as mentioned in one of the comments.
| - -ec | ||
| - ETCDCTL_API=3 | ||
| - etcdctl | ||
| - --config-file /var/etcd/config/etcd.conf.yaml |
There was a problem hiding this comment.
Add = symbol. --config-file=/var/etcd/config/etcd.conf.yaml
chart/values.yaml
Outdated
|
|
||
| backup: | ||
| schedule: "*/5 * * * *" # cron standard schedule | ||
| maxBackups: 7 # Maximum number of backups to keep (may change in future) |
There was a problem hiding this comment.
Add field
# garbageCollectionPolicy mentions the policy for garbage collecting old backups. Allowed values are Exponential(default), LimtBased.
`garbageCollectionPolicy`: `Exponential`
chart/values.yaml
Outdated
| etcd-backup-restore: eu.gcr.io/gardener-project/gardener/etcdbrctl:0.3.0 | ||
|
|
||
| backup: | ||
| schedule: "*/5 * * * *" # cron standard schedule |
There was a problem hiding this comment.
Will please comments as per the standard mentioned here.
| @@ -0,0 +1,28 @@ | |||
| --- | |||
There was a problem hiding this comment.
Rename it to etcd-bootstrap-configmap.yaml.
| @@ -0,0 +1,48 @@ | |||
| --- | |||
There was a problem hiding this comment.
Rename it to Actually merge this with etcd-config-configmap.yaml.etcd-bootstrap-configmap.yaml.
| @@ -0,0 +1,186 @@ | |||
| apiVersion: v1 | |||
| kind: Service | |||
There was a problem hiding this comment.
Create separate file named etcd-client-service.yaml for service.
chart/values.yaml
Outdated
| # CN should be "etcd-<role>", and | ||
| # hosts should include "etcd-<role>-0" | ||
| # * tlsClientSecretName should have "tls.crt" and "tls.key" | ||
| tlsCaSecretName: ca-etcd |
There was a problem hiding this comment.
How about restructuring it as:
tls:
caSecret: ca-etcd
serverSecret: etcd-server-tls
clientSecret: etcd-client-tlsAlso, internally, in template files, have if-check {{ if .Value.tls }} then only embed tls related code. Otherwise go without SSL.
|
@georgekuruvillak would you want me to include this change into this PR as well: I don't really understand what this part does, can you elaborate on why this |
|
@bergerx Ignore the From helm PR's perspective, no need to change The change is being done in favour of #58 |
|
Hi @bergerx, I haven't seen update on this PR for long time. I can understand that you might be busy. Will you please take look into this and address the comments? Thanks and regards, |
bergerx
force-pushed
the
add-helm-chart
branch
from
603d81d to
69fa04c
Compare
|
Sorry for being late, but i've been stuck with an issue that i can't really understand the reason. When i provide an empty
Below you can see some logs.
|
bergerx
force-pushed
the
add-helm-chart
branch
from
69fa04c to
a0eb883
Compare
No issues.
Ahh. You probably facing this issue with backup sidecar image version 0.3.x. which had one of the unhandled race condition. Will you please change image version to 0.4.0 and try? Looks like you updated the PR. I'll review it soon. |
|
Cool, let me try with updated vetsion first, i'll do it shortly and let you
know.
…On Tue 11 Dec 2018, 01:26 Swapnil Mhamane, ***@***.***> wrote:
Sorry for being late
No issue
i've been stuck with an issue that i can't really understand the reason.
Ahh. You probably facing this issue with backup sidecar image version
0.3.x. which had one of the unhandled race condition. Will you please
change image version to 0.4.0 and try?
Looks like you updated the PR. I'll review it soon.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#59 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACx2zeem_zM4rNrUYILkwMN32gGT5aBks5u33pagaJpZM4XZu7g>
.
|
bergerx
force-pushed
the
add-helm-chart
branch
2 times, most recently
from
5c4811f to
70c0f41
Compare
|
@swapnilgm thanks for the tip, seems like working fine now PTAL |
| name: etcd-{{.Values.role}} | ||
|
|
||
| # Path to the data directory. | ||
| data-dir: /var/etcd/data |
There was a problem hiding this comment.
Will you please change this to /var/etcd/data/new.etcd. Otherwise restore will fail.
chart/values.yaml
Outdated
| etcd: quay.io/coreos/etcd:v3.3.10 | ||
|
|
||
| # etcd-backup-restore image to use | ||
| etcd-backup-restore: eu.gcr.io/gardener-project/gardener/etcdbrctl:0.3.1 |
There was a problem hiding this comment.
Please update tag to: 0.4.0
chart/values.yaml
Outdated
| # whole tls section if you dont want to use tls for the etcd. | ||
| tls: | ||
|
|
||
| # caSecret should point a pre-crearted Opaque secret that includes a |
There was a problem hiding this comment.
type: s/crearted/created/g
chart/values.yaml
Outdated
| # secretKeyRef: | ||
| # name: etcd-backup | ||
| # key: "secretAccessKey" | ||
| # - name: "AWS_ACCESS_KEY_ID |
There was a problem hiding this comment.
Please append ending quote ".
| - --max-backups={{ .Values.backup.maxBackups }} | ||
| - --garbage-collection-policy={{ .Values.gabageCollectionPolicy }} | ||
| {{- end }} | ||
| - --data-dir=/var/etcd/data |
There was a problem hiding this comment.
Please change this to /var/etcd/data/new.etcd.
There was a problem hiding this comment.
There was a problem hiding this comment.
No. Mount path should be /var/etcd/data. Its correct. Reason here is, as per recent change restore operation happens at <etcd-data-dir-path>.part i.e in this case /var/etcd/data/new.etcd.part. Once restoration there is succeded, we rename it to actual <etcd-data-dir> i.e. /var/etcd/data/new.etcd. So, if we change mount point to /var/etcd/data/new.etcd the rename operation will fail because of permission.
There was a problem hiding this comment.
Thanks for the confirmation.
swapnilgm
added
status/in-progress
bergerx
force-pushed
the
add-helm-chart
branch
2 times, most recently
from
1e707fb to
ff6811d
Compare
|
Addressed the comments, squashed, and rebased. |
|
Oh please hold, i just found something i missed about the |
Copied and modified teh helm chart available in Gardener repo.
bergerx
force-pushed
the
add-helm-chart
branch
from
ff6811d to
805b0ab
Compare
|
Apparently, I closed the backup secret condition at the wrong place, and also forcing the backup secret not to be used if Seems like working properly with or without TLS now, already planning to use this it in https://github.com/gardener/gardener/pull/606/files#diff-c0fdeb77a0802ee74c1c55237b2d4bfeR193 This is now ready to be reviewed and merged. |
swapnilgm
added
reviewed/ok-to-test
gardener-robot-ci-1
added
needs/ok-to-test
What this PR does / why we need it:
The only helm chart available for etcd-backup-restore was in gardener, this one adds a helm chart to the upstream.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Historical discussion in the slack channel can be found in: https://kubernetes.slack.com/archives/CB57N0BFG/p1539340130000100
Release note: