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

Use cron schedule for defragmentation #194

Merged
merged 2 commits into from
Sep 27, 2019
Merged

Use cron schedule for defragmentation #194

merged 2 commits into from
Sep 27, 2019

Conversation

swapnilgm
Copy link
Contributor

@swapnilgm swapnilgm commented Aug 30, 2019 •
edited
Loading

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:
This PR changes the defragmentation period configuration to cron like schedule. Also, partially addresses #125
Which issue(s) this PR fixes:
Fixes partially #125

Special notes for your reviewer:

Release note:

Defragmentation schedule can be configured now in cron standards using flag `defragmentation-schedule`. :warning: Removed the flag `defragmentation-period-hours`.

:warning: Removed the flag `delta-snapshot-period-seconds`. Instead use replacement flag `delta-snapshot-period` with input value format supported by golang `time.Duration`.

:warning: Removed the flag `garbage-collection-period-seconds`. Instead use replacement flag `garbage-collection-period` with input value format supported by golang `time.Duration`.

@swapnilgm swapnilgm added exp/beginner Issue that requires only basic skills needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) platform/all kind/api-change API change with impact on API users priority/normal labels Aug 30, 2019
@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 Aug 30, 2019
@swapnilgm swapnilgm force-pushed the feature/cron-schedule-for-defrag branch from 2635efa to 341715d Compare September 11, 2019 11:04
@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 11, 2019
@swapnilgm swapnilgm added this to the 0.8.0 milestone Sep 12, 2019
@swapnilgm
Copy link
Contributor Author

@shreyas-s-rao I have rebased the PR. This PR is now ready for the review. While refactoring, i have added the changes for garbage-collection-period and delta-snapshot-period in same PR. Sorry to have it in single PR itself.
I would also prefer to have this PR in, before rest of PR just to minimise the efforts with rebasing and conflicts.

amshuman-kr
amshuman-kr suggested changes Sep 12, 2019
View reviewed changes
Copy link
Collaborator

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

LGTM except for few comments about modularity.

cmd/server.go Outdated
}
// If no storage provider is given, snapshotter will be nil, in which
// case the status is set to OK as soon as etcd probe is successful
handler = startHTTPServer(etcdInitializer, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about modularising the common code from here till here? It is also repeated between here and here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

cmd/snapshot.go Show resolved Hide resolved
cmd/server.go Outdated
return nil
})
runEtcdProbeLoopWithoutSnapshotter(tlsConfig, handler, stopCh, ackCh)
defragSchedule, err := cron.ParseStandard(defragmentationSchedule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not parse defragmentationSchedule centrally once and then send around the parsed result?

cmd/server.go Outdated

// start defragmentation without trigerring full snapshot
// after each successful data defragmentation
defragSchedule, err := cron.ParseStandard(defragmentationSchedule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not parse defragmentationSchedule centrally once and then send around the parsed result?

@swapnilgm swapnilgm force-pushed the feature/cron-schedule-for-defrag branch from 341715d to cd7ddc0 Compare September 12, 2019 11:37
@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 12, 2019
amshuman-kr
amshuman-kr approved these changes Sep 12, 2019
View reviewed changes
Copy link
Collaborator

@amshuman-kr amshuman-kr 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 change. LGTM

@swapnilgm
Copy link
Contributor Author

Thanks you for the review @amshuman-kr . I have addressed your comments. Yes indeed validation and configuration options handling is little decentralized currently. Added as and when required with new feature. I have created the issue to separately refactor related code. Don't want combine it all together. I have it partially in-porgress here

shreyas-s-rao
shreyas-s-rao requested changes Sep 25, 2019
View reviewed changes
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@swapnilgm Thanks a lot for the well-written and much needed PR! Overall looks good. Just a few changes required here and there. Please address them. Thanks.

Edit: While running the test, I see the following message for the defrag spec should defragment and reduce size of DB within time:

{"level":"warn","ts":"2019-09-25T13:53:04.466+0530","caller":"clientv3/retry_interceptor.go:61","msg":"retrying of unary invoker failed","target":"passthrough:///127.0.0.1:60091","attempt":0,"error":"rpc error: code = DeadlineExceeded desc = context deadline exceeded"}

chart/etcd-backup-restore/values.yaml Outdated Show resolved Hide resolved
pkg/etcdutil/defrag.go Show resolved Hide resolved
cmd/restore.go Show resolved Hide resolved
cmd/server.go Show resolved Hide resolved
cmd/server.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer_test.go Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer_test.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer_suite_test.go Show resolved Hide resolved
pkg/snapshot/restorer/restorer_test.go Outdated Show resolved Hide resolved
pkg/snapshot/snapshotter/snapshotter_test.go Outdated Show resolved Hide resolved
@swapnilgm swapnilgm force-pushed the feature/cron-schedule-for-defrag branch from cd7ddc0 to c0bc734 Compare September 25, 2019 11:58
@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 Sep 25, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 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 Sep 25, 2019
Change garbage-collection-period-seconds flag to
5b7bfba 
garbage-collection-period

Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the feature/cron-schedule-for-defrag branch from c0bc734 to 5b7bfba Compare September 25, 2019 13:19
@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 Sep 25, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 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 Sep 25, 2019
@swapnilgm
Copy link
Contributor Author

swapnilgm commented Sep 26, 2019 •
edited
Loading

Thank you for the review @shreyas-s-rao. PTAL. I have addressed the comments. PTAL.

Edit: While running the test, I see the following message for the defrag spec should defragment and reduce size of DB within time:
{"level":"warn","ts":"2019-09-25T13:53:04.466+0530","caller":"clientv3/retry_interceptor.go:61","msg":"retrying of unary invoker failed","target":"passthrough:///127.0.0.1:60091","attempt":0,"error":"rpc error: code = DeadlineExceeded desc = context deadline exceeded"}

This is due to vendor update. Retry_interceptor.go from etcd has log message irrespective of error. The error appears on etcd server stop call. So, nothing to worry about it.

@swapnilgm swapnilgm merged commit cbd9c77 into gardener:master Sep 27, 2019
@swapnilgm swapnilgm deleted the feature/cron-schedule-for-defrag branch September 27, 2019 04:53
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-s-rao shreyas-s-rao shreyas-s-rao requested changes

@amshuman-kr amshuman-kr amshuman-kr approved these changes

@georgekuruvillak georgekuruvillak Awaiting requested review from georgekuruvillak

Assignees
No one assigned
Labels
exp/beginner Issue that requires only basic skills kind/api-change API change with impact on API users 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 platform/all size/s Size of pull request is small (see gardener-robot robot/bots/size.py)
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@swapnilgm @amshuman-kr @shreyas-s-rao @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot