Enabled jobs instead of cronjobs to take care of running compaction. #235

abdasgupta · 2021-09-25T10:38:12Z

How to categorize this PR?

/area performance
/kind enhancement

What this PR does / why we need it:
Earlier cronjobs are scheduled through druid for backup compaction of ETCD. But that incurs huge cost as we have to predefine schedule and resources for compaction jobs. This PR enabled to run jobs instead of cronjobs to run the backup compaction. Jobs are triggered based on the number of events that accumulated as delta snapshot after last full snapshot.

cc @aaronfern
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

CronJob is no more used to schedule compaction job at regular interval. Instead, we are using Job.
Therefore, `BackupCompactionSchedule` field is removed from ETCD backup spec, as it was only necessary for scheduling CronJob.

A new controller named lease controller has been introduced. Lease controller will be responsible for creating compaction job based on the delta event lease.
For this, two new `Lease`s are introduced: One to hold the value of the latest full snapshot revision and one for the last delta revision.

gardener-robot · 2021-09-25T10:38:15Z

@abdasgupta Labels area/todo, kind/todo do not exist.

timuthy

Thanks for the PR. I have a few suggestions and questions which I added as a comment to the affected line(s).

controllers/etcd_custodian_controller.go

charts/etcd/templates/etcd-compaction-job.yaml

controllers/etcd_custodian_controller.go

aaronfern

Thanks @abdasgupta for the well written PR!
Just one query from me and some nitpicks
Thanks!

controllers/lease_controller.go

controllers/etcd_controller.go

controllers/controller_ref_manager.go

aaronfern

Hey @abdasgupta
Just adding below some changes to the job that needs to be made before this PR is merged
This is to ensure it correctly works with the etcdbr version

controllers/lease_controller.go

controllers/controller_ref_manager.go

timuthy

Just some last comments. Thanks for your effort 👍

main.go

pkg/predicate/predicate_test.go

timuthy

/lgtm
I tested and played around with it w/o facing any issues 👍

stoyanr

/lgtm

There are a few comments that still use the old name LeaseController that you may want to fix.

stoyanr · 2021-11-12T13:50:08Z

controllers/compaction_lease_controller.go

+	config      controllersconfig.CompactionLeaseConfig
+}
+
+// NewCompactionLeaseController creates a new LeaseController object


Suggested change

// NewCompactionLeaseController creates a new LeaseController object

// NewCompactionLeaseController creates a new CompactionLeaseController object

stoyanr · 2021-11-12T13:50:32Z

controllers/compaction_lease_controller.go

+	}
+}
+
+// NewCompactionLeaseControllerWithImageVector creates a new LeaseController object


Suggested change

// NewCompactionLeaseControllerWithImageVector creates a new LeaseController object

// NewCompactionLeaseControllerWithImageVector creates a new CompactionLeaseController object

stoyanr · 2021-11-12T13:50:54Z

controllers/compaction_lease_controller.go

+	return lc.InitializeControllerWithImageVector()
+}
+
+// InitializeControllerWithImageVector will use LeaseController client to initialize image vector for etcd


Suggested change

// InitializeControllerWithImageVector will use LeaseController client to initialize image vector for etcd

// InitializeControllerWithImageVector will use CompactionLeaseController client to initialize image vector for etcd

aaronfern

/lgtm

Added test case for lease controller in separate test file.

timuthy

/lgtm

Comments have been addressed. Reviewer is absent.

abdasgupta requested a review from a team as a code owner September 25, 2021 10:38

gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Sep 25, 2021

gardener-robot-ci-3 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, 2021

gardener-robot-ci-2 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 Sep 25, 2021

timuthy suggested changes Sep 28, 2021

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Sep 28, 2021

timuthy suggested changes Sep 28, 2021

View reviewed changes

timuthy mentioned this pull request Sep 30, 2021

Simplify reconciliation #208

Closed

abdasgupta force-pushed the job branch from 6565109 to 5314db6 Compare October 6, 2021 10:00

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 Oct 6, 2021

abdasgupta force-pushed the job branch from 5314db6 to df17135 Compare October 6, 2021 15:33

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 Oct 6, 2021

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 Oct 6, 2021

abdasgupta force-pushed the job branch from df17135 to 54d3671 Compare October 6, 2021 16:09

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 Oct 6, 2021

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 Oct 6, 2021

abdasgupta changed the title ~~[WIP] Enabled jobs instead of cronjobs to take care of running compaction.~~ Enabled jobs instead of cronjobs to take care of running compaction. Oct 6, 2021

aaronfern requested changes Oct 7, 2021

View reviewed changes

aaronfern reviewed Oct 7, 2021

View reviewed changes

controllers/lease_controller.go Outdated Show resolved Hide resolved

controllers/lease_controller.go Outdated Show resolved Hide resolved

controllers/lease_controller.go Outdated Show resolved Hide resolved

abdasgupta force-pushed the job branch from 54d3671 to d945509 Compare October 7, 2021 13:37

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 Oct 7, 2021

timuthy mentioned this pull request Oct 11, 2021

Added role, rolebinding, serviceaccount, and environment variables. #233

Merged

aaronfern reviewed Oct 18, 2021

View reviewed changes

controllers/lease_controller.go Outdated Show resolved Hide resolved

controllers/lease_controller.go Outdated Show resolved Hide resolved

controllers/controller_ref_manager.go Outdated Show resolved Hide resolved

abdasgupta force-pushed the job branch from d945509 to 0f95533 Compare October 19, 2021 11:27

timuthy suggested changes Nov 11, 2021

View reviewed changes

main.go Outdated Show resolved Hide resolved

pkg/predicate/predicate_test.go Outdated Show resolved Hide resolved

gardener-robot-ci-3 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 Nov 11, 2021

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 Nov 11, 2021

timuthy approved these changes Nov 12, 2021

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else labels Nov 12, 2021

timuthy requested review from aaronfern, stoyanr and shreyas-s-rao November 12, 2021 12:43

stoyanr approved these changes Nov 12, 2021

View reviewed changes

timuthy mentioned this pull request Nov 12, 2021

Several improvements for compaction feature #258

Merged

aaronfern approved these changes Nov 15, 2021

View reviewed changes

Enabled jobs instead of cronjobs to take care of running compaction.

f5a2929

Added test case for lease controller in separate test file.

abdasgupta force-pushed the job branch from efa8681 to f5a2929 Compare November 15, 2021 09:21

gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Nov 15, 2021

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 Nov 15, 2021

gardener-robot-ci-1 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 Nov 15, 2021

gardener-robot added area/performance Performance (across all domains, such as control plane, networking, storage, etc.) related kind/enhancement Enhancement, improvement, extension labels Nov 15, 2021

timuthy approved these changes Nov 15, 2021

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Nov 15, 2021

timuthy merged commit 1c2d8ab into gardener:master Nov 15, 2021

abdasgupta deleted the job branch November 15, 2021 16:56

abdasgupta mentioned this pull request Nov 16, 2021

[Feature] Divide the Druid controllers into Reconciler and Actuator #259

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled jobs instead of cronjobs to take care of running compaction. #235

Enabled jobs instead of cronjobs to take care of running compaction. #235

abdasgupta commented Sep 25, 2021 •

edited by timuthy

gardener-robot commented Sep 25, 2021

timuthy left a comment

aaronfern left a comment

aaronfern left a comment

timuthy left a comment

timuthy left a comment

stoyanr left a comment

stoyanr Nov 12, 2021

stoyanr Nov 12, 2021

stoyanr Nov 12, 2021

aaronfern left a comment

timuthy left a comment

	// NewCompactionLeaseController creates a new LeaseController object
	// NewCompactionLeaseController creates a new CompactionLeaseController object

	// NewCompactionLeaseControllerWithImageVector creates a new LeaseController object
	// NewCompactionLeaseControllerWithImageVector creates a new CompactionLeaseController object

	// InitializeControllerWithImageVector will use LeaseController client to initialize image vector for etcd
	// InitializeControllerWithImageVector will use CompactionLeaseController client to initialize image vector for etcd

Enabled jobs instead of cronjobs to take care of running compaction. #235

Enabled jobs instead of cronjobs to take care of running compaction. #235

Conversation

abdasgupta commented Sep 25, 2021 • edited by timuthy

gardener-robot commented Sep 25, 2021

timuthy left a comment

Choose a reason for hiding this comment

aaronfern left a comment

Choose a reason for hiding this comment

aaronfern left a comment

Choose a reason for hiding this comment

timuthy left a comment

Choose a reason for hiding this comment

timuthy left a comment

Choose a reason for hiding this comment

stoyanr left a comment

Choose a reason for hiding this comment

stoyanr Nov 12, 2021

Choose a reason for hiding this comment

stoyanr Nov 12, 2021

Choose a reason for hiding this comment

stoyanr Nov 12, 2021

Choose a reason for hiding this comment

aaronfern left a comment

Choose a reason for hiding this comment

timuthy left a comment

Choose a reason for hiding this comment

abdasgupta commented Sep 25, 2021 •

edited by timuthy