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.

Sign up for GitHub

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

Jump to bottom

Adds sleep command at the end of compaction. #660

Merged
merged 5 commits into from
Sep 8, 2023
Merged

Adds sleep command at the end of compaction. #660

merged 5 commits into from
Sep 8, 2023

Conversation

abdasgupta
Copy link
Contributor

@abdasgupta abdasgupta commented Aug 23, 2023 •
edited
Loading

What this PR does / why we need it:
This PR adds a provision of sleep duration after the Compact function done with uploading snapshot. This sleep duration is needed as prometheus is required to collect necessary metrics for network activity after a fast full snapshot upload. The sleep duration is supplied as flag to etcdbrctl compact command and set to 0 by default. But the flag can be configured by any agent, who is using etcdbrctl, to set any duration value like 60s, 2m etc. 60s value of sleep duration is enough to allow sufficient time for Prometheus to scrape network related metrics after a fast full snapshot upload.

Which issue(s) this PR fixes:
Fixes #
gardener/etcd-druid#648

Special notes for your reviewer:

Release note:

Introduce flag `metrics-scrape-wait-duration` to `etcdbrctl compact` command, that specifies a wait duration at the end of a snapshot compaction, to allow Prometheus to scrape metrics related to compaction before the `etcdbrctl` process exits.

@abdasgupta abdasgupta requested a review from a team as a code owner August 23, 2023 05:14
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Aug 23, 2023
@abdasgupta
Copy link
Contributor Author

cc @istvanballok

@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 23, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added 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 Aug 23, 2023
@abdasgupta abdasgupta mentioned this pull request Aug 23, 2023
Merged
istvanballok
istvanballok reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@istvanballok istvanballok left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Aug 23, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 23, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 28, 2023
istvanballok
istvanballok reviewed Aug 28, 2023
View reviewed changes
Copy link
Member

@istvanballok istvanballok left a comment

Choose a reason for hiding this comment

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

lgtm

@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 Aug 29, 2023
shreyas-s-rao
shreyas-s-rao requested changes Aug 29, 2023
View reviewed changes
Copy link
Collaborator

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

@abdasgupta thanks for the PR. I have a few suggestions/comments. PTAL, thanks!

pkg/compactor/compactor_test.go Outdated Show resolved Hide resolved
pkg/types/compactor.go Outdated Show resolved Hide resolved
pkg/types/compactor.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added needs/changes Needs (more) changes and removed reviewed/lgtm Has approval for merging labels Aug 29, 2023
@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 Aug 29, 2023
@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) 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 Aug 30, 2023
@shreyas-s-rao shreyas-s-rao self-assigned this Sep 4, 2023
@gardener-robot gardener-robot removed the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Sep 8, 2023
@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) 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 Sep 8, 2023
shreyas-s-rao
shreyas-s-rao requested changes Sep 8, 2023
View reviewed changes
Copy link
Collaborator

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

@abdasgupta thanks for making the changes. I have a few comments regarding the changes made. PTAL, thanks

pkg/compactor/compactor_test.go Outdated Show resolved Hide resolved
pkg/types/compactor.go Outdated Show resolved Hide resolved
pkg/types/compactor.go Outdated Show resolved Hide resolved
@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) 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 Sep 8, 2023
@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 Sep 8, 2023
@shreyas-s-rao
Copy link
Collaborator

@abdasgupta thanks for making the changes. Please adapt the PR description and release note to reflect the new flag name and behavior.

Also, I would ask you to not force-push to PRs in the future, since it becomes very difficult to review the new changes that were made. Instead, you may choose to add a squash-merge label to the PR, so that whoever is merging the PR will squash-merge the PR and leave out the addressed review comments commits.

@shreyas-s-rao
Copy link
Collaborator

/label merge/squash

@gardener-robot gardener-robot added the merge/squash Should be merged via 'Squash and merge' label Sep 8, 2023
@shreyas-s-rao
Copy link
Collaborator

The release note needs to be more precise and correctly formatted. See below example:

Introduce flag `metrics-scrape-wait-duration` to `etcdbrctl compact` command, that specifies a wait duration at the end of a snapshot compaction, to allow Prometheus to scrape metrics related to compaction before the `etcdbrctl` process exits.

shreyas-s-rao
shreyas-s-rao approved these changes Sep 8, 2023
View reviewed changes
Copy link
Collaborator

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes labels Sep 8, 2023
@shreyas-s-rao shreyas-s-rao merged commit 77bb0f9 into gardener:master Sep 8, 2023
1 check passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 8, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 8, 2023
@seshachalam-yv seshachalam-yv added this to the v0.26.0 milestone Sep 29, 2023
@abdasgupta abdasgupta mentioned this pull request Sep 29, 2023
Merged
abdasgupta added a commit to abdasgupta/etcd-backup-restore that referenced this pull request Oct 4, 2023
@abdasgupta
Adds sleep command at the end of compaction. (gardener#660)
87db556
@abdasgupta abdasgupta mentioned this pull request Oct 4, 2023
Merged
seshachalam-yv pushed a commit that referenced this pull request Oct 4, 2023
@abdasgupta
Adds sleep command at the end of compaction. (#660) (#667)
582d985
@abdasgupta abdasgupta mentioned this pull request Oct 5, 2023
Merged
@timuthy timuthy mentioned this pull request Nov 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@istvanballok istvanballok istvanballok left review comments

@unmarshall unmarshall unmarshall left review comments

@shreyas-s-rao shreyas-s-rao shreyas-s-rao approved these changes

Assignees

@abdasgupta abdasgupta

Labels
merge/squash Should be merged via 'Squash and merge' needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
v0.26.0
Development

Successfully merging this pull request may close these issues.
9 participants
@abdasgupta @shreyas-s-rao @unmarshall @istvanballok @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot @seshachalam-yv