CronJob controller cleanups #110838

soltysh · 2022-06-28T15:12:04Z

What type of PR is this?

/kind cleanup
/sig apps
/priority backlog

What this PR does / why we need it:

There are 2 major changes introduced in this PR:

Minor controller cleanups to eliminate code duplication (byJobStartTimeStar sorter) and moving helpers into utils.go file.
Cleaning up two very close methods nextScheduledTimeDuration and getNextScheduledTime (especially after we landed Fix requeueing of cronjobs with every-style schedule #109250).

Special notes for your reviewer:

/assign @atiratree
/cc @d-honeybadger

Does this PR introduce a user-facing change?

Improve cronjob controller performance

k8s-ci-robot · 2022-06-28T15:12:08Z

@soltysh: GitHub didn't allow me to request PR reviews from the following users: d-honeybadger.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind cleanup
/sig apps
/priority backlog

What this PR does / why we need it:

There are 2 major changes introduced in this PR:

Minor controller cleanups to eliminate code duplication (byJobStartTimeStar sorter) and moving helpers into utils.go file.

Cleaning up two very close methods nextScheduledTimeDuration and getNextScheduledTime (especially after we landed Fix requeueing of cronjobs with every-style schedule #109250).

Special notes for your reviewer:

/assign @atiratree
/cc @d-honeybadger

Does this PR introduce a user-facing change?
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

soltysh · 2022-06-28T15:12:50Z

/triage accepted

k8s-ci-robot · 2022-06-28T15:13:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/cronjob/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

d-honeybadger

Thanks for letting me know!
Had to think for a bit how StartingDeadlineSeconds was unnecessary in the mostRecentTime() calculations, but pretty convinced now that that's correct.
I took the libery of leaving a couple nit comments.

d-honeybadger · 2022-06-28T16:50:21Z

pkg/controller/cronjob/utils.go

@@ -33,6 +33,7 @@ import (

 // Utilities for dealing with Jobs and CronJobs and time.

+// inActiveListByName take a job and checks if activeJobs has a job with the same UID.


Looks like the comment got copied here by mistake

Updated comment to make it more explanatory.

d-honeybadger · 2022-06-28T16:54:24Z

pkg/controller/cronjob/utils.go

-	)
+// mostRecentScheduleTime returns:
+// - the last schedule time or CronJob's creation time,
+// - the most recent time a Job should be created or nil, if that's before now,


or nil, if that's before now shoud be or nil, if that's after now I believe

Yes, you're right, thx!

pkg/controller/cronjob/utils.go

atiratree · 2022-06-29T17:52:35Z

pkg/controller/cronjob/utils.go

@@ -42,6 +43,17 @@ func inActiveList(cj batchv1.CronJob, uid types.UID) bool {
 	return false
 }

+// inActiveListByName take a job and checks if activeJobs has a job with the same
+// name and namespace.
+func inActiveListByName(job *batchv1.Job, activeJobs []corev1.ObjectReference) bool {


this could be more similar to inActiveList

we could switch flip the arguments: (what, where) -> (where, what)

pass CronJob instead of []corev1.ObjectReference

Good suggestion, updated.

atiratree · 2022-06-29T18:24:52Z

pkg/controller/cronjob/utils.go

+// - the last schedule time or CronJob's creation time,
+// - the most recent time a Job should be created or nil, if that's before now,
+// - number of missed schedules
+// - error in an edge case where the schedule specification is gramatically correct,


Suggested change

// - error in an edge case where the schedule specification is gramatically correct,

// - error in an edge case where the schedule specification is grammatically correct,

;-)

atiratree · 2022-06-29T19:19:30Z

pkg/controller/cronjob/utils_test.go

 	}{
 		{
 			name:     "both have nil start times",
-			input:    []batchv1.Job{bNil, aNil},
-			expected: []batchv1.Job{aNil, bNil},
+			input:    []*batchv1.Job{&bNil, &aNil},


might be simpler to create a pointer when declaring the variables

Good point - fixed.

atiratree · 2022-06-29T19:35:26Z

pkg/controller/cronjob/utils_test.go

 			},
-			expectedTime: nil,
+			now:                topOfTheHour().Add(time.Second * 30),
+			expectedRecentTime: nil,


this would not throw an error if we got non nil result

atiratree · 2022-06-29T19:36:46Z

pkg/controller/cronjob/utils_test.go

+				t.Error("mostRecentScheduleTime() got error when none expected")
+			}
+			if gotEarliestTime.IsZero() && tt.expectedEarliestTime != nil {
+				t.Errorf("mostRecentScheduleTime() got 0, want %v", tt.expectedEarliestTime)


can we describe in all of these, what variable name are we expecting?

pkg/controller/cronjob/utils_test.go

pkg/controller/cronjob/utils.go

pkg/controller/cronjob/utils_test.go

k8s-triage-robot · 2022-10-25T13:39:23Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-11-24T14:12:03Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

dims · 2022-12-12T15:33:08Z

If you still need this PR then please rebase, if not, please close the PR

soltysh · 2023-01-03T14:25:36Z

Unrelated failure on CustomResourcePublishOpenAPI test
/test pull-kubernetes-e2e-gce

k8s-ci-robot · 2023-01-03T15:29:12Z

@soltysh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-ubuntu-containerd	8190a73ccad0641a86b7df6fa83c0aa8e3c43cc9	link	true	`/test pull-kubernetes-e2e-gce-ubuntu-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

soltysh · 2023-01-04T12:03:48Z

Unrelated failure on CustomResourcePublishOpenAPI test
/test pull-kubernetes-e2e-gce

atiratree

the PR looks in a good shape

pkg/controller/cronjob/utils_test.go

atiratree · 2023-01-04T21:10:23Z

pkg/controller/cronjob/utils_test.go

 			}
-			if gotTime != nil && tt.expectedTime != nil && !gotTime.Equal(*tt.expectedTime) {
-				t.Errorf("getMostRecentScheduleTime() got = %v, want %v", gotTime, tt.expectedTime)
+			if gotRecentTime != nil && tt.expectedRecentTime != nil && !gotRecentTime.Equal(*tt.expectedRecentTime) {


might be more readable to use reflect.DeepEqual here

1. Squash two identical sorters byTime 2. Move helper for searching active jobs into utils to exist next to its counterpart

…Duration The two methods nextScheduledTimeDuration and getNextScheduleTime have a lot of similarities, so this commit squashes the common parts together along with getMostRecentScheduleTime to avoid code duplication.

atiratree · 2023-01-18T15:55:34Z

looks good, thx
/lgtm

k8s-ci-robot · 2023-01-18T15:55:40Z

LGTM label has been added.

Git tree hash: fbb7921b5768228d7e1dd01db83443a2257182fd

…838-#121327-upstream-release-1.26 Automated cherry pick of #110838: Cleanups in controller utils #121327: Fix next schedule time duration

k8s-ci-robot assigned atiratree Jun 28, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 28, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2022

d-honeybadger reviewed Jun 29, 2022

View reviewed changes

atiratree reviewed Jun 29, 2022

View reviewed changes

soltysh force-pushed the cronjob_improvements branch from fcad11c to 8190a73 Compare July 25, 2022 14:40

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2022

atiratree reviewed Jul 27, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 24, 2022

soltysh force-pushed the cronjob_improvements branch from 8190a73 to 949c953 Compare December 22, 2022 14:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022

soltysh force-pushed the cronjob_improvements branch from 949c953 to ed99d82 Compare January 3, 2023 09:45

atiratree reviewed Jan 4, 2023

View reviewed changes

Cleanups in controller utils

cb491a8

1. Squash two identical sorters byTime 2. Move helper for searching active jobs into utils to exist next to its counterpart

soltysh force-pushed the cronjob_improvements branch 2 times, most recently from 20c1527 to 76218ff Compare January 18, 2023 15:40

soltysh force-pushed the cronjob_improvements branch from 76218ff to be44d67 Compare January 18, 2023 15:52

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2023

k8s-ci-robot merged commit c63434a into kubernetes:master Jan 18, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 18, 2023

soltysh deleted the cronjob_improvements branch January 18, 2023 18:49

sxllwx mentioned this pull request Oct 20, 2023

kube-controller-manager v1.26 high cpu usage #118706

Closed

soltysh mentioned this pull request Oct 26, 2023

Automated cherry pick of #110838: Cleanups in controller utils #121327: Fix next schedule time duration #121540

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CronJob controller cleanups #110838

CronJob controller cleanups #110838

soltysh commented Jun 28, 2022 •

edited

k8s-ci-robot commented Jun 28, 2022

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

soltysh commented Jun 28, 2022

k8s-ci-robot commented Jun 28, 2022

d-honeybadger left a comment

d-honeybadger Jun 28, 2022

soltysh Jul 25, 2022

d-honeybadger Jun 28, 2022

soltysh Jul 25, 2022

atiratree Jun 29, 2022

soltysh Jul 25, 2022

atiratree Jun 29, 2022

atiratree Jun 29, 2022

soltysh Jul 25, 2022

atiratree Jun 29, 2022

atiratree Jun 29, 2022

soltysh Jul 25, 2022

k8s-triage-robot commented Oct 25, 2022

k8s-triage-robot commented Nov 24, 2022

dims commented Dec 12, 2022

soltysh commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023 •

edited

soltysh commented Jan 4, 2023

atiratree left a comment

atiratree Jan 4, 2023

atiratree commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

		@@ -33,6 +33,7 @@ import (

		// Utilities for dealing with Jobs and CronJobs and time.

		// inActiveListByName take a job and checks if activeJobs has a job with the same UID.

	// - error in an edge case where the schedule specification is gramatically correct,
	// - error in an edge case where the schedule specification is grammatically correct,

CronJob controller cleanups #110838

CronJob controller cleanups #110838

Conversation

soltysh commented Jun 28, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jun 28, 2022

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

soltysh commented Jun 28, 2022

k8s-ci-robot commented Jun 28, 2022

d-honeybadger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Oct 25, 2022

k8s-triage-robot commented Nov 24, 2022

dims commented Dec 12, 2022

soltysh commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023 • edited

soltysh commented Jan 4, 2023

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

soltysh commented Jun 28, 2022 •

edited

k8s-ci-robot commented Jan 3, 2023 •

edited