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 #764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anveshreddy18
Copy link
Contributor

@anveshreddy18 anveshreddy18 commented Feb 8, 2024

How to categorize this PR?

/area usability
/kind enhancement

What this PR does / why we need it:

This PR adds a new field fullSnapshotLeaseUpdateInterval in the spec.backup section of Etcd yaml and makes necessary changes, which allows to configure full-snapshot-lease-update-interval parameter used to configure the interval to retry updating full snapshot lease.

  • 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.

Which issue(s) this PR fixes:
Fixes #763

Special notes for your reviewer:

Release note:

Enabling the configurability of `full-snapshot-lease-update-interval` flag through the etcd resource spec `.spec.backup.fullSnapshotLeaseUpdateInterval`.

@anveshreddy18 anveshreddy18 requested a review from a team as a code owner February 8, 2024 11:51
@gardener-robot gardener-robot added area/usability Usability related kind/enhancement Enhancement, improvement, extension needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Feb 8, 2024
@anveshreddy18 anveshreddy18 self-assigned this Feb 8, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 8, 2024
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @anveshreddy18 !

Couple of comments from my side:

  1. PTAL at the points I have mentioned in Add a new configurable field fullSnapshotLeaseUpdateInterval in spec.backup section of Etcd CR #763 (comment)
  2. Please use a PR-built image of etcdbr in this PR (at charts/images.yaml) so that this PR can be easily tested. You can obtain the PR-built image from the concourse publish step from Full snapshot lease update retry on failure etcd-backup-restore#711

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Feb 8, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 9, 2024
@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
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@shreyas-s-rao
Copy link
Contributor

/hold until PR for #728 gets merged, since that will bring changes in the component model for resources deployed by druid.

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Mar 21, 2024
@anveshreddy18 anveshreddy18 force-pushed the configure/fullsnapshot-lease-update-retry-interval branch from 7c4f934 to fefae40 Compare June 24, 2024 11:52
@gardener-robot gardener-robot added size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jun 24, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 24, 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 needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new configurable field fullSnapshotLeaseUpdateInterval in spec.backup section of Etcd CR
7 participants