Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add DEP-06: Immutable ETCD Backups #884
Add DEP-06: Immutable ETCD Backups #884
Changes from 1 commit
805f277
8cd21aa
b8655fb
769f24a
6e4511a
5c4ab0e
b1af993
d14c9cc
eb64433
47e806d
45dbc38
d96e0ea
e778155
859c030
c1b27e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since the DEP has changed significantly since the original draft, and the both of us have only acted as reviewers after the first draft, it is not right to have our names in the author section.
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.
If you are providing hints for 2 out of the 3 supported providers then i would say just complete the list and provide it as bullet points - one for each provider. Alternatively, you can completely remove this point since you have already provided link to a detailed documentation in etcd-backup-restore repo.
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.
Removed.
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.
It would be helpful if you provide a sample yaml. So either choose one from config/samples which illustrates what you are proposing or create a new one and put it there and then give a reference to the section/lines in that yaml.
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.
Yes, Added the sample yaml
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.
handled by
gardener/gardener
? - either it should point to the component within gardener which handles this or it should at least provide a link to the g/g repo. Just havinggardener/gardener
makes sense for us but not for anyone reading this from outside.Can you also reach out to the core colleagues and ask for a link to any documentation which describes how gardener does credential rotation specifically for backup buckets?
Since in this section which is titled
Gardener Only
you also have a line about non-gardener use case then the title of this section should rather just beCredential Rotation
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.
I've removed the bullet point since Gardener does not handle cloud provider credential rotation for the seed bucket, as stated [here](https://gardener.cloud/docs/getting-started/features/credential-rotation/#user-provided-credentials).
Credential rotation must be managed by the operator, so I've added the following note:
Note
etcd-druid
does not handle cloud provider credential rotation. It is the responsibility of the operator to manage credential rotation.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.
To ensure that there is no data loss, one must cut-off traffic to etcd before taking a full snapshot. This is then followed by hibernation of an etcd cluster. This you have also mentioned in
etcd Controller Enhancements
section below but in summarising the approach this is missed.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.
Thanks, @unmarshall, for catching this! I've now added the note.
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 also discussed that it is possible that the full snapshot taken prior to hibernation gets corrupted. Then you stand a chance to lose data for at least 24 hours. In addition the full snapshot prior to the one that you are about to take will become mutable after some time. So now you get into a situation where the final full snapshot is corrupt and the one prior to that is mutable and therefore susceptible to modifications. Should we mention this gap?
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.
Yes, I've added it as limitation as part of 5c4ab0e
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.
Just add a line saying that we have defined a new type to allow us future enhancements to the immutability specification.
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.
Done
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.
you should also mention that if a bucket is shared across several etcd clusters then this enhancement would increase the storage cost and perhaps some numbers would be nice.
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.
i did not quite understand this type in context of
ExtendFullSnapshotImmutabilityTaskConfig
- why is it that complicated? If GC needs to be done via this task as well then it should only be done for copied last full snapshot (taken prior to hibernation). I am confused why is delta snapshot retention coming here and what is exponential GC policy and how is relevant w.r.t this task.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.
Now, I've removed the garbage collection config and ExtendFullSnapshotImmutabilityTask will automatically garbage collect the snapshots it has created, ensuring storage efficiency and preventing stale backups.