-
Notifications
You must be signed in to change notification settings - Fork 234
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
Update backup-restic to etcd 3.5.11 and fix addon documentation #2981
Conversation
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
I agree with going forward with the latter option. The former option would be a breaking change for sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
@@ -52,24 +52,14 @@ spec: | |||
path: /etc/kubernetes/pki | |||
initContainers: | |||
- name: snapshoter | |||
image: {{ Registry "gcr.io" }}/etcd-development/etcd:v3.5.6 | |||
image: {{ Registry "gcr.io" }}/etcd-development/etcd:v3.5.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to use k8s.gcr.io
instead in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should, KKP is also still using this image.
LGTM label has been added. Git tree hash: f9ae99bda4ff23244cc119ce9224a57a2e042a40
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xmudrii 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 |
/cherrypick release/v1.7 |
@xmudrii: once the present PR merges, I will cherry-pick it on top of release/v1.7 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherrypick release/v1.6 |
@xmudrii: once the present PR merges, I will cherry-pick it on top of release/v1.6 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest guess that is a flake. |
@xmudrii: new pull request created: #2982 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@xmudrii: new pull request created: #2983 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
This updates the
backup-restic
addon to use etcd 3.5.11, the latest available 3.5.x image. Because the images are now using distroless, I have moved the parts that copy the certificates into the backup into therestic
container before uploading the backup. Given that the container will have access to these files anyway (since its uploading them to s3), I did not see any gain from having another initContainer doing it.In addition, while trying out the addon, I ran into a documentation issue - The
s3Bucket
param does not do what it's documented to do. If you just put a bucket name as is documented, the backups will be written to a local folder (becauseRESTIC_REPOSITORY
is set to the contents ofs3Bucket
verbatim).Now I had two choices, either fix the way
RESTIC_REPOSITORY
is set froms3Bucket
or update documentation for the field. I have chosen to go with the latter, because anyone using the addon and validating their backups (which I hope everyone does ... right?) would have figured this out already. I'm afraid that breaking the existing way is going to create more issues (break it for everyone who already sets thes3:/
part).I'm open to change this in a follow-up PR, but it wasn't the focus of this one so I just wanted to write down the as-is situation correctly.
Which issue(s) this PR fixes:
Fixes #2980
What type of PR is this?
/kind feature
Special notes for your reviewer:
Does this PR introduce a user-facing change? Then add your Release Note here:
Documentation: