Merge pull request #113510 from alculquicondor/finalizers-stable

Graduate JobTrackingWithFinalizers to stable
This commit is contained in:
Kubernetes Prow Robot
2022-11-07 08:06:41 -08:00
committed by GitHub
15 changed files with 306 additions and 564 deletions

View File

@@ -24,10 +24,13 @@ import (
// JobTrackingFinalizer is a finalizer for Job's pods. It prevents them from
// being deleted before being accounted in the Job status.
// The apiserver and job controller use this string as a Job annotation, to
// mark Jobs that are being tracked using pod finalizers. Two releases after
// the JobTrackingWithFinalizers graduates to GA, JobTrackingFinalizer will
// no longer be used as a Job annotation.
//
// Additionally, the apiserver and job controller use this string as a Job
// annotation, to mark Jobs that are being tracked using pod finalizers.
// However, this behavior is deprecated in kubernetes 1.26. This means that, in
// 1.27+, one release after JobTrackingWithFinalizers graduates to GA, the
// apiserver and job controller will ignore this annotation and they will
// always track jobs using finalizers.
const JobTrackingFinalizer = "batch.kubernetes.io/job-tracking"
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -405,9 +408,6 @@ type JobStatus struct {
// (3) Remove the pod UID from the array while increasing the corresponding
// counter.
//
// This field is beta-level. The job controller only makes use of this field
// when the feature gate JobTrackingWithFinalizers is enabled (enabled
// by default).
// Old jobs might not be tracked using this field, in which case the field
// remains null.
// +optional

View File

@@ -53,7 +53,7 @@ type orderedIntervals []interval
// the indexes that succeeded since the last sync.
func calculateSucceededIndexes(job *batch.Job, pods []*v1.Pod) (orderedIntervals, orderedIntervals) {
var prevIntervals orderedIntervals
withFinalizers := trackingUncountedPods(job)
withFinalizers := hasJobTrackingAnnotation(job)
if withFinalizers {
prevIntervals = succeededIndexesFromJob(job)
}

View File

@@ -22,10 +22,6 @@ import (
"github.com/google/go-cmp/cmp"
batch "k8s.io/api/batch/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"k8s.io/kubernetes/pkg/features"
"k8s.io/utils/pointer"
)
@@ -219,13 +215,7 @@ func TestCalculateSucceededIndexes(t *testing.T) {
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, tc.trackingWithFinalizers)()
job := &batch.Job{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
batch.JobTrackingFinalizer: "",
},
},
Status: batch.JobStatus{
CompletedIndexes: tc.prevSucceeded,
},
@@ -233,6 +223,11 @@ func TestCalculateSucceededIndexes(t *testing.T) {
Completions: pointer.Int32Ptr(tc.completions),
},
}
if tc.trackingWithFinalizers {
job.Annotations = map[string]string{
batch.JobTrackingFinalizer: "",
}
}
pods := hollowPodsWithIndexPhase(tc.pods)
for _, p := range pods {
p.Finalizers = append(p.Finalizers, batch.JobTrackingFinalizer)

View File

@@ -714,17 +714,13 @@ func (jm *Controller) syncJob(ctx context.Context, key string) (forget bool, rEr
var expectedRmFinalizers sets.String
var uncounted *uncountedTerminatedPods
if trackingUncountedPods(&job) {
if hasJobTrackingAnnotation(&job) {
klog.V(4).InfoS("Tracking uncounted Pods with pod finalizers", "job", klog.KObj(&job))
if job.Status.UncountedTerminatedPods == nil {
job.Status.UncountedTerminatedPods = &batch.UncountedTerminatedPods{}
}
uncounted = newUncountedTerminatedPods(*job.Status.UncountedTerminatedPods)
expectedRmFinalizers = jm.finalizerExpectations.getExpectedUIDs(key)
} else if patch := removeTrackingAnnotationPatch(&job); patch != nil {
if err := jm.patchJobHandler(ctx, &job, patch); err != nil {
return false, fmt.Errorf("removing tracking finalizer from job %s: %w", key, err)
}
}
// Check the expectations of the job before counting active pods, otherwise a new pod can sneak in
@@ -1476,7 +1472,7 @@ func (jm *Controller) manageJob(ctx context.Context, job *batch.Job, activePods
if isIndexedJob(job) {
addCompletionIndexEnvVariables(podTemplate)
}
if trackingUncountedPods(job) {
if hasJobTrackingAnnotation(job) {
podTemplate.Finalizers = appendJobCompletionFinalizerIfNotFound(podTemplate.Finalizers)
}
@@ -1635,10 +1631,6 @@ func getCompletionMode(job *batch.Job) string {
return string(batch.NonIndexedCompletion)
}
func trackingUncountedPods(job *batch.Job) bool {
return feature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers) && hasJobTrackingAnnotation(job)
}
func hasJobTrackingAnnotation(job *batch.Job) bool {
if job.Annotations == nil {
return false
@@ -1669,21 +1661,6 @@ func removeTrackingFinalizerPatch(pod *v1.Pod) []byte {
return patchBytes
}
func removeTrackingAnnotationPatch(job *batch.Job) []byte {
if !hasJobTrackingAnnotation(job) {
return nil
}
patch := map[string]interface{}{
"metadata": map[string]interface{}{
"annotations": map[string]interface{}{
batch.JobTrackingFinalizer: nil,
},
},
}
patchBytes, _ := json.Marshal(patch)
return patchBytes
}
type uncountedTerminatedPods struct {
succeeded sets.String
failed sets.String

View File

@@ -135,7 +135,7 @@ func newPodList(count int, status v1.PodPhase, job *batch.Job) []*v1.Pod {
for i := 0; i < count; i++ {
newPod := newPod(fmt.Sprintf("pod-%v", rand.String(10)), job)
newPod.Status = v1.PodStatus{Phase: status}
if trackingUncountedPods(job) {
if hasJobTrackingAnnotation(job) {
newPod.Finalizers = append(newPod.Finalizers, batch.JobTrackingFinalizer)
}
pods = append(pods, newPod)
@@ -178,7 +178,7 @@ func setPodsStatusesWithIndexes(podIndexer cache.Indexer, job *batch.Job, status
}
p.Spec.Hostname = fmt.Sprintf("%s-%s", job.Name, s.Index)
}
if trackingUncountedPods(job) {
if hasJobTrackingAnnotation(job) {
p.Finalizers = append(p.Finalizers, batch.JobTrackingFinalizer)
}
podIndexer.Add(p)
@@ -343,15 +343,15 @@ func TestControllerSyncJob(t *testing.T) {
parallelism: 2,
completions: 5,
backoffLimit: 6,
podControllerError: fmt.Errorf("fake error"),
jobKeyForget: true,
activePods: 1,
succeededPods: 1,
failedPods: 1,
expectedCreations: 1,
expectedActive: 1,
expectedActive: 2,
expectedSucceeded: 1,
expectedFailed: 1,
expectedPodPatches: 2,
},
"new failed pod": {
parallelism: 2,
@@ -728,7 +728,6 @@ func TestControllerSyncJob(t *testing.T) {
t.Skipf("Test is exclusive for wFinalizers=%t", *tc.wFinalizersExclusive)
}
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobReadyPods, tc.jobReadyPodsEnabled)()
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, wFinalizers)()
// job manager setup
clientSet := clientset.NewForConfigOrDie(&restclient.Config{Host: "", ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}})
@@ -918,13 +917,12 @@ func checkIndexedJobPods(t *testing.T, control *controller.FakePodControl, wantI
}
// TestSyncJobLegacyTracking makes sure that a Job is only tracked with
// finalizers only when the feature is enabled and the job has the finalizer.
// finalizers when the job has the annotation.
func TestSyncJobLegacyTracking(t *testing.T) {
cases := map[string]struct {
job batch.Job
trackingWithFinalizersEnabled bool
wantUncounted bool
wantPatches int
job batch.Job
wantUncounted bool
wantPatches int
}{
"no annotation": {
job: batch.Job{
@@ -937,19 +935,7 @@ func TestSyncJobLegacyTracking(t *testing.T) {
},
},
},
"no annotation, feature enabled": {
job: batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "ns",
},
Spec: batch.JobSpec{
Parallelism: pointer.Int32Ptr(1),
},
},
trackingWithFinalizersEnabled: true,
},
"tracking annotation, feature disabled": {
"tracking annotation": {
job: batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
@@ -962,26 +948,9 @@ func TestSyncJobLegacyTracking(t *testing.T) {
Parallelism: pointer.Int32Ptr(1),
},
},
// Finalizer removed.
wantPatches: 1,
wantUncounted: true,
},
"tracking annotation, feature enabled": {
job: batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "ns",
Annotations: map[string]string{
batch.JobTrackingFinalizer: "",
},
},
Spec: batch.JobSpec{
Parallelism: pointer.Int32Ptr(1),
},
},
trackingWithFinalizersEnabled: true,
wantUncounted: true,
},
"different annotation, feature enabled": {
"different annotation": {
job: batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
@@ -994,13 +963,10 @@ func TestSyncJobLegacyTracking(t *testing.T) {
Parallelism: pointer.Int32Ptr(1),
},
},
trackingWithFinalizersEnabled: true,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, tc.trackingWithFinalizersEnabled)()
// Job manager setup.
clientSet := clientset.NewForConfigOrDie(&restclient.Config{Host: "", ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}})
manager, sharedInformerFactory := newControllerFromClient(clientSet, controller.NoResyncPeriodFunc)
@@ -2909,7 +2875,6 @@ func TestSyncJobWithJobPodFailurePolicy(t *testing.T) {
for _, wFinalizers := range []bool{false, true} {
for name, tc := range testCases {
t.Run(fmt.Sprintf("%s; finalizers=%t", name, wFinalizers), func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, wFinalizers)()
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobPodFailurePolicy, tc.enableJobPodFailurePolicy)()
clientset := clientset.NewForConfigOrDie(&restclient.Config{Host: "", ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}})
manager, sharedInformerFactory := newControllerFromClient(clientset, controller.NoResyncPeriodFunc)
@@ -2992,15 +2957,9 @@ func TestSyncJobUpdateRequeue(t *testing.T) {
updateErr: apierrors.NewConflict(schema.GroupResource{}, "", nil),
wantRequeue: true,
},
"conflict error, with finalizers": {
withFinalizers: true,
updateErr: apierrors.NewConflict(schema.GroupResource{}, "", nil),
wantRequeue: true,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, tc.withFinalizers)()
manager, sharedInformerFactory := newControllerFromClient(clientset, controller.NoResyncPeriodFunc)
fakePodControl := controller.FakePodControl{}
manager.podControl = &fakePodControl
@@ -4204,7 +4163,6 @@ func TestEnsureJobConditions(t *testing.T) {
}
func TestFinalizersRemovedExpectations(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, true)()
clientset := fake.NewSimpleClientset()
sharedInformers := informers.NewSharedInformerFactory(clientset, controller.NoResyncPeriodFunc())
manager := NewController(sharedInformers.Core().V1().Pods(), sharedInformers.Batch().V1().Jobs(), clientset)

View File

@@ -450,6 +450,7 @@ const (
// owner: @alculquicondor
// alpha: v1.22
// beta: v1.23
// stable: v1.26
//
// Track Job completion without relying on Pod remaining in the cluster
// indefinitely. Pod finalizers, in addition to a field in the Job status
@@ -951,7 +952,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
JobReadyPods: {Default: true, PreRelease: featuregate.Beta},
JobTrackingWithFinalizers: {Default: true, PreRelease: featuregate.Beta},
JobTrackingWithFinalizers: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.28
KubeletCredentialProviders: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.28

View File

@@ -13605,7 +13605,7 @@ func schema_k8sio_api_batch_v1_JobStatus(ref common.ReferenceCallback) common.Op
},
"uncountedTerminatedPods": {
SchemaProps: spec.SchemaProps{
Description: "UncountedTerminatedPods holds the UIDs of Pods that have terminated but the job controller hasn't yet accounted for in the status counters.\n\nThe job controller creates pods with a finalizer. When a pod terminates (succeeded or failed), the controller does three steps to account for it in the job status: (1) Add the pod UID to the arrays in this field. (2) Remove the pod finalizer. (3) Remove the pod UID from the arrays while increasing the corresponding\n counter.\n\nThis field is beta-level. The job controller only makes use of this field when the feature gate JobTrackingWithFinalizers is enabled (enabled by default). Old jobs might not be tracked using this field, in which case the field remains null.",
Description: "UncountedTerminatedPods holds the UIDs of Pods that have terminated but the job controller hasn't yet accounted for in the status counters.\n\nThe job controller creates pods with a finalizer. When a pod terminates (succeeded or failed), the controller does three steps to account for it in the job status: (1) Add the pod UID to the arrays in this field. (2) Remove the pod finalizer. (3) Remove the pod UID from the arrays while increasing the corresponding\n counter.\n\nOld jobs might not be tracked using this field, in which case the field remains null.",
Ref: ref("k8s.io/api/batch/v1.UncountedTerminatedPods"),
},
},

View File

@@ -93,13 +93,10 @@ func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
job.Generation = 1
if utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers) {
// Until this feature graduates to GA and soaks in clusters, we use an
// annotation to mark whether jobs are tracked with it.
addJobTrackingAnnotation(job)
} else {
dropJobTrackingAnnotation(job)
}
// While legacy tracking is supported, we use an annotation to mark whether
// jobs are tracked with finalizers.
addJobTrackingAnnotation(job)
if !utilfeature.DefaultFeatureGate.Enabled(features.JobPodFailurePolicy) {
job.Spec.PodFailurePolicy = nil
}
@@ -122,20 +119,12 @@ func hasJobTrackingAnnotation(job *batch.Job) bool {
return ok
}
func dropJobTrackingAnnotation(job *batch.Job) {
delete(job.Annotations, batchv1.JobTrackingFinalizer)
}
// PrepareForUpdate clears fields that are not allowed to be set by end users on update.
func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
newJob := obj.(*batch.Job)
oldJob := old.(*batch.Job)
newJob.Status = oldJob.Status
if !utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers) && !hasJobTrackingAnnotation(oldJob) {
dropJobTrackingAnnotation(newJob)
}
if !utilfeature.DefaultFeatureGate.Enabled(features.JobPodFailurePolicy) && oldJob.Spec.PodFailurePolicy == nil {
newJob.Spec.PodFailurePolicy = nil
}
@@ -147,6 +136,13 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object
if !apiequality.Semantic.DeepEqual(newJob.Spec, oldJob.Spec) {
newJob.Generation = oldJob.Generation + 1
}
// While legacy tracking is supported, we use an annotation to mark whether
// jobs are tracked with finalizers. This annotation cannot be removed by
// users.
if hasJobTrackingAnnotation(oldJob) {
addJobTrackingAnnotation(newJob)
}
}
// Validate validates a new job.
@@ -170,7 +166,7 @@ func validationOptionsForJob(newJob, oldJob *batch.Job) validation.JobValidation
}
opts := validation.JobValidationOptions{
PodValidationOptions: pod.GetValidationOptionsFromPodTemplate(newPodTemplate, oldPodTemplate),
AllowTrackingAnnotation: utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers),
AllowTrackingAnnotation: true,
}
if oldJob != nil {
// Because we don't support the tracking with finalizers for already

View File

@@ -184,6 +184,30 @@ func TestJobStrategy_PrepareForUpdate(t *testing.T) {
},
},
},
"add tracking annotation back": {
job: batch.Job{
ObjectMeta: getValidObjectMetaWithAnnotations(0, map[string]string{batchv1.JobTrackingFinalizer: ""}),
Spec: batch.JobSpec{
Selector: validSelector,
Template: validPodTemplateSpec,
PodFailurePolicy: podFailurePolicy,
},
},
updatedJob: batch.Job{
ObjectMeta: getValidObjectMeta(0),
Spec: batch.JobSpec{
Selector: validSelector,
Template: validPodTemplateSpec,
},
},
wantJob: batch.Job{
ObjectMeta: getValidObjectMetaWithAnnotations(1, map[string]string{batchv1.JobTrackingFinalizer: ""}),
Spec: batch.JobSpec{
Selector: validSelector,
Template: validPodTemplateSpec,
},
},
},
}
for name, tc := range cases {
@@ -279,24 +303,6 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) {
// TODO(#111514): refactor by spliting into dedicated test functions
func TestJobStrategy(t *testing.T) {
cases := map[string]struct {
trackingWithFinalizersEnabled bool
}{
"features disabled": {},
"new job tracking enabled": {
trackingWithFinalizersEnabled: true,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.JobTrackingWithFinalizers, tc.trackingWithFinalizersEnabled)()
testJobStrategy(t)
})
}
}
func testJobStrategy(t *testing.T) {
trackingWithFinalizersEnabled := utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers)
ctx := genericapirequest.NewDefaultContext()
if !Strategy.NamespaceScoped() {
t.Errorf("Job must be namespace scoped")
@@ -356,9 +362,9 @@ func testJobStrategy(t *testing.T) {
if job.Spec.CompletionMode == nil {
t.Errorf("Job should allow setting .spec.completionMode")
}
wantAnnotations := map[string]string{"foo": "bar"}
if trackingWithFinalizersEnabled {
wantAnnotations[batchv1.JobTrackingFinalizer] = ""
wantAnnotations := map[string]string{
"foo": "bar",
batchv1.JobTrackingFinalizer: "",
}
if diff := cmp.Diff(wantAnnotations, job.Annotations); diff != "" {
t.Errorf("Job has annotations (-want,+got):\n%s", diff)
@@ -405,9 +411,8 @@ func testJobStrategy(t *testing.T) {
if updatedJob.Generation != 2 {
t.Errorf("expected Generation=2, got %d", updatedJob.Generation)
}
wantAnnotations = make(map[string]string)
if trackingWithFinalizersEnabled {
wantAnnotations[batchv1.JobTrackingFinalizer] = ""
wantAnnotations = map[string]string{
batchv1.JobTrackingFinalizer: "",
}
if diff := cmp.Diff(wantAnnotations, updatedJob.Annotations); diff != "" {
t.Errorf("Job has annotations (-want,+got):\n%s", diff)
@@ -418,17 +423,6 @@ func testJobStrategy(t *testing.T) {
t.Errorf("Expected a validation error")
}
// Ensure going from legacy tracking Job to tracking with finalizers is
// disallowed.
job = job.DeepCopy()
job.Annotations = nil
updatedJob = job.DeepCopy()
updatedJob.Annotations = map[string]string{batch.JobTrackingFinalizer: ""}
errs = Strategy.ValidateUpdate(ctx, updatedJob, job)
if len(errs) != 1 {
t.Errorf("Expected update validation error")
}
// Test updating suspend false->true and nil-> true when the feature gate is
// disabled. We don't care about other combinations.
job.Spec.Suspend, updatedJob.Spec.Suspend = pointer.BoolPtr(false), pointer.BoolPtr(true)
@@ -475,7 +469,6 @@ func TestJobStrategyValidateUpdate(t *testing.T) {
job *batch.Job
update func(*batch.Job)
wantErrs field.ErrorList
trackingWithFinalizersEnabled bool
mutableSchedulingDirectivesEnabled bool
}{
"update parallelism": {
@@ -518,7 +511,7 @@ func TestJobStrategyValidateUpdate(t *testing.T) {
{Type: field.ErrorTypeInvalid, Field: "spec.completions"},
},
},
"adding tracking annotation disallowed, gate disabled": {
"adding tracking annotation disallowed": {
job: &batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "myjob",
@@ -540,30 +533,7 @@ func TestJobStrategyValidateUpdate(t *testing.T) {
{Type: field.ErrorTypeForbidden, Field: "metadata.annotations[batch.kubernetes.io/job-tracking]"},
},
},
"adding tracking annotation disallowed, gate enabled": {
job: &batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "myjob",
Namespace: metav1.NamespaceDefault,
ResourceVersion: "0",
Annotations: map[string]string{"foo": "bar"},
},
Spec: batch.JobSpec{
Selector: validSelector,
Template: validPodTemplateSpec,
ManualSelector: pointer.BoolPtr(true),
Parallelism: pointer.Int32Ptr(1),
},
},
update: func(job *batch.Job) {
job.Annotations[batch.JobTrackingFinalizer] = ""
},
wantErrs: field.ErrorList{
{Type: field.ErrorTypeForbidden, Field: "metadata.annotations[batch.kubernetes.io/job-tracking]"},
},
trackingWithFinalizersEnabled: true,
},
"preserving tracking annotation, feature disabled": {
"preserving tracking annotation": {
job: &batch.Job{
ObjectMeta: metav1.ObjectMeta{
Name: "myjob",
@@ -683,7 +653,6 @@ func TestJobStrategyValidateUpdate(t *testing.T) {
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.JobTrackingWithFinalizers, tc.trackingWithFinalizersEnabled)()
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.JobMutableNodeSchedulingDirectives, tc.mutableSchedulingDirectivesEnabled)()
newJob := tc.job.DeepCopy()
tc.update(newJob)