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

Roll-forward: Beta requirements for JobTrackingWithFinalizers #105197

Merged
merged 5 commits into from Oct 5, 2021
Merged

Roll-forward: Beta requirements for JobTrackingWithFinalizers #105197

merged 5 commits into from Oct 5, 2021

Conversation

alculquicondor
Copy link
Member

@alculquicondor alculquicondor commented Sep 22, 2021 •
edited

What type of PR is this?

/kind feature
/sig apps

What this PR does / why we need it:

Satisfies beta requirements for JobTrackingWithFinalizers (but it doesn't graduate the feature gate yet):

  • Limit the number of pods tracked in a single sync to avoid starvation of small jobs.
  • Add metric job_pod_finished_total, which increments when the job controller finished tracking a pod (finalizer removed and pod accounted in status counters)

Which issue(s) this PR fixes:

Ref kubernetes/enhancements#2307

Fixes #105200

Special notes for your reviewer:

This is roll-forward of #104739, reverted in #105181. Commits 2 and 3 are a direct revert of #105181.
The remaining commits include the fixes to the flakiness. The main changes are:

  • Doing Pod updates in parallel, reduce number of pods to 50.
  • Add an in-memory cache for the pods for which we have removed a finalizer (similar to the creations/deletions expectations).

Does this PR introduce a user-facing change?

dropped release note with revert in #105181

When feature gate JobTrackingWithFinalizers is enabled:
- Limit the number of pods tracked in a single job sync to avoid starvation of small jobs.
- The metric job_pod_finished_total counts the number of finished pods tracked by the job controller

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/2307-job-tracking-without-lingering-pods
- [Usage]: https://kubernetes.io/docs/concepts/workloads/controllers/job/#job-tracking-with-finalizers

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/apps Categorizes an issue or PR as relevant to SIG Apps. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 22, 2021
@alculquicondor
Copy link
Member Author

/hold
This is still WIP (do not review yet)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2021
@k8s-ci-robot k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 22, 2021
@alculquicondor
Copy link
Member Author

/remove-sig instrumentation testing

@k8s-ci-robot k8s-ci-robot removed sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 22, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 30, 2021
@alculquicondor
Parallelize pod updates in job test
95c2a80 
To potentially reduce the number of job controller syncs.

Also reduce the maximum number of pods to sync in tests.
@alculquicondor
Copy link
Member Author

I will proceed to rerun integration tests a few times to spot flakiness

/test pull-kubernetes-integration

@alculquicondor
Copy link
Member Author

3rd run
/test pull-kubernetes-integration

@alculquicondor
Copy link
Member Author

4th run
/test pull-kubernetes-integration

@alculquicondor
Copy link
Member Author

5th run
/test pull-kubernetes-integration

@alculquicondor
Copy link
Member Author

/hold cancel
/assign @soltysh

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2021
soltysh
soltysh approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/triage accepted
/priority backlog
/lgtm
/approve

pkg/controller/job/job_controller.go Outdated
var err error
if needsFlush {
if job, err = jm.updateStatusHandler(job); err != nil {
return job, needsFlush, fmt.Errorf("adding uncounted pods to status: %w", err)
}
recordJobPodFinished(job, *oldCounters)
// Shallow copy.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: explain why it's sufficient for shallow copy rather than just stating it

Copy link
Member Author

Choose a reason for hiding this comment

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

/hold

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, 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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 4, 2021
@alculquicondor
Track expected removals of Pod finalizers
5929ccd 
Add the UIDs of Pods for which we are removing finalizers to an in-memory cache.

The controller removes UIDs from the cache as Pod updates or deletes come in.

This avoids double counting finished Pods when Pod updates arrive after Job status updates.

kubernetes#105200
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2021
alculquicondor
alculquicondor commented Oct 4, 2021
View reviewed changes
Copy link
Member Author

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2021
@alculquicondor
Copy link
Member Author

/retest

@ahg-g
Copy link
Member

ahg-g commented Oct 4, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2021
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-ci-robot k8s-ci-robot merged commit 0a29e2a into kubernetes:master Oct 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 5, 2021
@alculquicondor alculquicondor mentioned this pull request Oct 14, 2021
Merged
@alculquicondor alculquicondor mentioned this pull request Nov 9, 2021
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@krmayankk krmayankk Awaiting requested review from krmayankk

@neolit123 neolit123 Awaiting requested review from neolit123

Assignees

@soltysh soltysh

@ahg-g ahg-g

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

Tracking with finalizers might count a Pod more than once
5 participants
@alculquicondor @k8s-ci-robot @ahg-g @k8s-triage-robot @soltysh