Graduate indexed job to beta #101292

AliceZhang2016 · 2021-04-20T20:34:50Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Graduate indexed job from alpha to beta, contain three parts in total:

add three metrics to job controller:
a. job_sync_duration_seconds: tracks the latency of a Job sync.
b. job_sync_total: tracks the number of Job syncs.
c. job_finished_total: tracks the number of finished job.
add limitations pods create/delete operations per job sync, so that:
a. big job will not occupy controller too much time which may get other jobs stuck, due to too many pods create/delete at once.
b. gives much better predictability of metrics job_sync_duration_seconds
change the default feature gate value of "IndexedJob" from false to true, to make it enabled by default in beta.

Which issue(s) this PR fixes:

fixes #98434

Special notes for your reviewer:

This is a requirement to graduate indexed job to Beta
The PR of changing hostname and podname is #101601

Does this PR introduce a user-facing change?

Add three metrics to job controller to monitor if Job works in a healthy condition.
IndexedJob promoted to Beta

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/2214-indexed-job

k8s-ci-robot · 2021-04-20T20:35:00Z

Hi @AliceZhang2016. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

AliceZhang2016 · 2021-04-20T20:35:09Z

cc @alculquicondor

pkg/controller/job/job_controller.go

pkg/controller/job/metrics/metrics.go

alculquicondor · 2021-04-20T21:26:08Z

pkg/controller/job/metrics/metrics.go

+	"sync"
+
+	"k8s.io/component-base/metrics"
+	"k8s.io/component-base/metrics/legacyregistry"


The name of this package suggests it's deprecated. We can probably use prometheus as suggested here https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/instrumentation.md#quick-start

The code of package metrics seems only wraps the prometheus, it also uses prometheus inside. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/metrics/histogram.go#L95

In that sense, there is no need for using legacyregistry, just use prometheus directly. Note that the guide was updated in February, so it's pretty up to date.

but the example in that guide was updated 6 years ago.

kubernetes/pkg/apiserver/apiserver.go

Line 53 in cd32993

requestCounter = prometheus.NewCounterVec(

@logicalhan what is the up-to-date recommendation?

You can't use prometheus directly, we actually have CI checks which should block you from merging PRs that use prometheus. The legacyregistry is deprecated because we want to eventually move off of using globals, which should happen hopefully this upcoming release.

So the recommendation is to use the wrappers (from component base) and currently legacyregistry unless you are exposing a new metrics endpoint. I've submitted a PR to update the docs (kubernetes/community#5737), sorry for the confusion.

alculquicondor · 2021-04-21T13:03:48Z

/ok-to-test

logicalhan · 2021-04-21T16:07:45Z

/triage accepted
/assign @brancz

pkg/controller/job/job_controller.go

logicalhan

/lgtm
(from sig instrumentation) sorry about the confusion in the docs.

alculquicondor · 2021-05-05T14:47:00Z

/lgtm

pkg/apis/batch/validation/validation.go

alculquicondor

/lgtm

liggitt · 2021-05-07T18:28:56Z

validation change looks good to me. eric's approve is now effective

liggitt · 2021-05-07T18:29:07Z

/hold cancel

This PR updates the feature gate status based on upstream implementation. The chanegs are listed below with links to upstream PRs merged: - BalanceAttacedNodeVolumes kubernetes/kubernetes#102443 - CSIMigrationvSphereComplete kubernetes/kubernetes#101272 - CSIServiceAccountToken kubernetes/kubernetes#103001 - DaemonSetUpdateSurge kubernetes/kubernetes#101742 - DisableCloudProviders kubernetes/kubernetes#100136 - IndexedJob kubernetes/kubernetes#101292 - LegacyNodeRoleBehavior kubernetes/kubernetes#100776 - NamespaceDefaultLabelName kubernetes/kubernetes#101342 - NetworkPolicyEndPort kubernetes/kubernetes#102834 - NodeDisruptionExclusion kubernetes/kubernetes#100776 - PodAffinityNamespaceSelector kubernetes/kubernetes#101496 - PodDeletionCost kubernetes/kubernetes#101080 - PreferNominatedNode kubernetes/kubernetes#102201 - ServiceLoadBalancerClass kubernetes/kubernetes#103129 - ServiceNodeExclusion kubernetes/kubernetes#100776 - ServiceTopology kubernetes/kubernetes#102412 - SizeMemoryBackedVoluems kubernetes/kubernetes#101048 - StatefulSetMinReadySeconds kubernetes/kubernetes#100842 - SuspendJob kubernetes/kubernetes#102022 - WindowsHostProcessContainers kubernetes/kubernetes#99576

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 20, 2021

k8s-ci-robot requested review from mortent and soltysh April 20, 2021 20:35

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 20, 2021

alculquicondor reviewed Apr 20, 2021

View reviewed changes

pkg/controller/job/job_controller.go Outdated Show resolved Hide resolved

alculquicondor reviewed Apr 20, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 21, 2021

AliceZhang2016 force-pushed the job_controller_metrics branch from 7475e81 to 2f1341f Compare April 21, 2021 15:43

k8s-ci-robot assigned brancz Apr 21, 2021

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 Apr 21, 2021

AliceZhang2016 force-pushed the job_controller_metrics branch from 2f1341f to 9c87df8 Compare April 21, 2021 17:17

alculquicondor reviewed Apr 21, 2021

View reviewed changes

pkg/controller/job/job_controller.go Outdated Show resolved Hide resolved

pkg/controller/job/job_controller.go Outdated Show resolved Hide resolved

logicalhan reviewed Apr 22, 2021

View reviewed changes

k8s-ci-robot assigned logicalhan Apr 22, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021

AliceZhang2016 force-pushed the job_controller_metrics branch from 9f3f610 to 8dbccdc Compare May 5, 2021 14:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021

AliceZhang2016 force-pushed the job_controller_metrics branch from 8dbccdc to 07f0ceb Compare May 5, 2021 17:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021

liggitt reviewed May 6, 2021

View reviewed changes

pkg/apis/batch/validation/validation.go Show resolved Hide resolved

add restriction to indexed job name

0c99f29

AliceZhang2016 force-pushed the job_controller_metrics branch from 07f0ceb to 0c99f29 Compare May 7, 2021 13:29

alculquicondor reviewed May 7, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2021

k8s-ci-robot merged commit 548fb43 into kubernetes:master May 7, 2021

k8s-ci-robot added this to the v1.22 milestone May 7, 2021

github-actions bot mentioned this pull request May 11, 2021

Week Ending May 9, 2021 dev-obs/actus#370

Open

JamesLaverack mentioned this pull request Jun 23, 2021

Indexed Job semantics in Job API kubernetes/enhancements#2214

Closed

12 tasks

tengqm mentioned this pull request Jun 27, 2021

Update feature gates for 1.22 kubernetes/website#28644

Merged

alculquicondor mentioned this pull request Aug 30, 2021

Add alculquicondor to sig-apps-reviewers #104663

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graduate indexed job to beta #101292

Graduate indexed job to beta #101292

AliceZhang2016 commented Apr 20, 2021 •

edited

Loading

k8s-ci-robot commented Apr 20, 2021

AliceZhang2016 commented Apr 20, 2021

alculquicondor Apr 20, 2021

AliceZhang2016 Apr 21, 2021

alculquicondor Apr 21, 2021

AliceZhang2016 Apr 21, 2021

alculquicondor Apr 21, 2021

logicalhan Apr 22, 2021

logicalhan Apr 22, 2021

alculquicondor commented Apr 21, 2021

logicalhan commented Apr 21, 2021

logicalhan left a comment

alculquicondor commented May 5, 2021

alculquicondor left a comment

liggitt commented May 7, 2021

liggitt commented May 7, 2021

Graduate indexed job to beta #101292

Graduate indexed job to beta #101292

Conversation

AliceZhang2016 commented Apr 20, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Apr 20, 2021

AliceZhang2016 commented Apr 20, 2021

alculquicondor Apr 20, 2021

Choose a reason for hiding this comment

AliceZhang2016 Apr 21, 2021

Choose a reason for hiding this comment

alculquicondor Apr 21, 2021

Choose a reason for hiding this comment

AliceZhang2016 Apr 21, 2021

Choose a reason for hiding this comment

alculquicondor Apr 21, 2021

Choose a reason for hiding this comment

logicalhan Apr 22, 2021

Choose a reason for hiding this comment

logicalhan Apr 22, 2021

Choose a reason for hiding this comment

alculquicondor commented Apr 21, 2021

logicalhan commented Apr 21, 2021

logicalhan left a comment

Choose a reason for hiding this comment

alculquicondor commented May 5, 2021

alculquicondor left a comment

Choose a reason for hiding this comment

liggitt commented May 7, 2021

liggitt commented May 7, 2021

AliceZhang2016 commented Apr 20, 2021 •

edited

Loading