Skip to content
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 a new configurable field fullSnapshotLeaseUpdateInterval in spec.backup section of Etcd CR #763

Closed
anveshreddy18 opened this issue Feb 8, 2024 · 3 comments
Assignees
Labels
area/usability Usability related kind/enhancement Enhancement, improvement, extension status/closed Issue is closed (either delivered or triaged)

Comments

@anveshreddy18
Copy link
Contributor

anveshreddy18 commented Feb 8, 2024

Feature (What you would like to be added):

Add fullSnapshotLeaseUpdateInterval field to the spec.backup section of Etcd yaml.

Motivation (Why is this needed?):

  • The backup-restore PR#711 introduces a new flag full-snapshot-lease-update-interval to configure the retry interval for updating the full snapshot lease. Adding this new fullSnapshotLeaseUpdateInterval field to Etcd CR allows user to control the behaviour of retrying to update full snapshot lease

Note: It will be an optional field, and when not set, backup-restore takes care of setting a default value to it.

Approach/Hint to the implement solution (optional):

Add the fullSnapshotLeaseUpdateInterval field to the spec.backup section of Etcd yaml, and define the FullSnapshotLeaseUpdateInterval field in the BackupSpec struct of Etcd CRD

@shreyas-s-rao
Copy link
Contributor

Thanks for creating the issue @anveshreddy18 . I would recommend calling the field FullSnapshotLeaseUpdateInterval.

  1. Snapshot is a fully qualified word and needs to be capitalised. It doesn't help much to use its short form snap
  2. The word retry means that if a lease update failed, then the operation is retried after a certain interval. In the updation logic in Full snapshot lease update retry on failure etcd-backup-restore#711, you are anyway trying an update of the full snapshot lease every X minutes (defined by the interval), irrespective of whether the previous updation succeeded or failed. So this isn't really a "retry", but simply a periodic "try"

Please also consider making these changes in the etcdbr PR as well, since the same points apply to that too. Thanks!

@anveshreddy18 anveshreddy18 changed the title Add a new configurable field fullsnapLeaseUpdateRetryInterval in spec.backup section of Etcd CR Add a new configurable field fullSnapshotLeaseUpdateInterval in spec.backup section of Etcd CR Feb 9, 2024
@anveshreddy18
Copy link
Contributor Author

Thanks for creating the issue @anveshreddy18 . I would recommend calling the field FullSnapshotLeaseUpdateInterval.

  1. Snapshot is a fully qualified word and needs to be capitalised. It doesn't help much to use its short form snap
  2. The word retry means that if a lease update failed, then the operation is retried after a certain interval. In the updation logic in Full snapshot lease update retry on failure etcd-backup-restore#711, you are anyway trying an update of the full snapshot lease every X minutes (defined by the interval), irrespective of whether the previous updation succeeded or failed. So this isn't really a "retry", but simply a periodic "try"

Please also consider making these changes in the etcdbr PR as well, since the same points apply to that too. Thanks!

Thanks for the suggestion @shreyas-s-rao, updated the issue, druid PR and etcdbr PR as well.

@shreyas-s-rao
Copy link
Contributor

/close

@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/usability Usability related kind/enhancement Enhancement, improvement, extension status/closed Issue is closed (either delivered or triaged)
Projects
None yet
3 participants