Skip to content

Commit

Permalink
Update API design
Browse files Browse the repository at this point in the history
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
  • Loading branch information
tenzen-y committed Jun 8, 2023
1 parent 0fa476f commit 87db3c1
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 38 deletions.
103 changes: 66 additions & 37 deletions keps/sig-apps/3998-job-success-completion-policy/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ tags, and then generate with `hack/update-toc.sh`.
- [Story 3](#story-3)
- [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional)
- [Risks and Mitigations](#risks-and-mitigations)
- [The Job object too big](#the-job-object-too-big)
- [The Job object too big](#the-job-object-too-big)
- [Design Details](#design-details)
- [Test Plan](#test-plan)
- [Prerequisite testing updates](#prerequisite-testing-updates)
Expand Down Expand Up @@ -250,8 +250,8 @@ bogged down.

#### Story 1

As a machine-learning researcher, I run a indexed job which drivers (leaders) are launched
as index=0,1,2, and workers are launched as non index=0,1,2.
As a machine-learning researcher, I run an indexed job which drivers (leaders) are launched
as index=0,1,2, and workers are launched as non index=3+.
I want to care about only the drivers when the result of job is evaluated.

```yaml
Expand All @@ -262,7 +262,7 @@ spec:
completions: 10
completionMode: Indexed
successfulPolicy:
onRequiredIndexes: [0,1,2]
requiredIndexes: "0-2"
template:
spec:
restartPolicy: Never
Expand All @@ -287,7 +287,7 @@ spec:
completions: 10
completionMode: Indexed
successfulPolicy:
onIndexesThreshold: 5
minSucceededIndexes: 5
template:
spec:
restartPolicy: Never
Expand All @@ -311,7 +311,7 @@ spec:
completions: 10
completionMode: Indexed
successfulPolicy:
onIndexesThreshold: 50%
minSucceededIndexes: 50%
template:
spec:
restartPolicy: Never
Expand Down Expand Up @@ -344,23 +344,13 @@ How will UX be reviewed, and by whom?
Consider including folks who also work outside the SIG or subproject.
-->
#### Incorrect `.status.completedIndexes`

If the job object's size reaches to limit of the etcd and
- If the job object's size reaches to limit of the etcd and
the job controller can't store a correct value in `.status.completedIndexes`,
we probably can not evaluate the SuccessfulPolicy correctly.

#### The Job object too big

If we allow users to set all indexes (`0~10^5`) to `.spec.successfulPolicy.onRequiredIndexes`
and don't limit the number of list sizes, there are cases that the size of a job object is too big.

In the worst case, allowed all indexes (`0~10^5`) are added to onRequiredIndexes.
In that case, a `onRequiredIndexes` size is `SUM[9*10^n*(n+1)]+2+6≈5.6656MiB`, where:
- `n` starts from `0` and goes up to `5`.
- `1` of `n+1` means a separator that indicates `,`.
- `2` is the sum of the index `0` and `,`.
- `6` is the size of indexes `10^5`.
- If we allow to set unlimited size of the value in `.spec.successfulPolicy.requiredIndexes`,
we have a risk similar to [KEP-3850: Backoff Limits Per Index For Indexed Jobs](https://github.com/kubernetes/enhancements/tree/76dcd4f342cc0388feb085e685d4cc018ebe1dc9/keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs#risks-and-mitigations).
So, we limit the size of `requiredIndexes` to 64Ki.

## Design Details

Expand Down Expand Up @@ -391,41 +381,50 @@ type JobSpec struct {

// SuccessfulPolicy describes how succeeded pods influence the conditions.
type SuccessfulPolicy struct {
// Specifies a set of required indexes.
// Specifies a set of required indexes when .spec.completionMode =
// "Indexed" in a text format.
// The job is declared as succeeded if a set of indexes are succeeded.
// The list of indexes must come within 0 to `.spec.completions` and
// must not contain duplicates. At least one element is required.
// The indexes are represented as decimal integers
// separated by commas. The numbers are listed in increasing order. Three or
// more consecutive numbers are compressed and represented by the first and
// last element of the series, separated by a hyphen.
// For example, if the completed indexes are 1, 3, 4, 5 and 7, they are
// represented as "1,3-5,7".
//
// +option
OnRequiredIndexes []int
RequiredIndexes string

// Specifies the required number of indexes.
// Specifies the required number of indexes when .spec.completionMode =
// "Indexed".
// Value can be an absolute number (ex: 5) or a percentage of total indexes
// when the job can be declared as succeeded (ex: 50%).
// The absolute number is calculated from the percentage by rounding up.
//
// +option
OnIndexesThreshold intstr.IntOrString
MinSucceededIndexes intstr.IntOrString
}
```

Moreover, we validate the following constraints:
- exactly one of the fields `onRequiredIndexes` and `onIndexesThreshold` is specified
- exactly one of the fields `requiredIndexes` and `minSucceededIndexes` is specified
for a requirement.
- whether the specified indexes in the `onRequiredIndexes` and
the number of indexes in the `onIndexesThreshold` don't exceed the value of `completions`.
- whether the specified indexes in the `requiredIndexes` and
the number of indexes in the `minSucceededIndexes` don't exceed the value of `completions`.
- whether `Indexed` is specified in the `completionMode` field.
- whether the size of `requiredIndexes` is under 64Ki.

### Evaluation
We evaluate the SuccessfulPolicy after all indexes have been finished.
And then if the job meets the SuccessfulPolicy, we add `Completed` condition
instead of `Failed` condition to `.status.conditions` even if other indexes fail.

If a set of indexes are specified in the `onRequiredIndexes` field,
If a set of indexes are specified in the `requiredIndexes` field,
we evaluate `.status.completedIndexes` to see if a set of indexes is there.

If the number of indexes is specified in the `onIndexesThreshold`,
we evaluate `.status.succeeded` to see if the value is `onIndexesThreshold` or more.
If the number of indexes is specified in the `minSucceededIndexes`,
we evaluate `.status.succeeded` to see if the value is `minSucceededIndexes` or more.

### Test Plan

Expand Down Expand Up @@ -474,7 +473,7 @@ extending the production code to implement this enhancement.

- Test cases:
- tests for Defaulting and Validating
- tests for rounding up absolute numbers in `.spec.successfulPolicy.onRequiredIndexesThreshold`.
- tests for rounding up absolute numbers in `.spec.successfulPolicy.minSucceededIndexes`.
- verify whether a job has condition=completed if the job meets to successfulPolicy
and some indexes fail.

Expand All @@ -501,8 +500,8 @@ https://storage.googleapis.com/k8s-triage/index.html
- Test scenarios:
- enabling, disabling and re-enabling of the `JobSuccessfulPolicy` feature gate
- handling of successfulPolicy when all indexes succeeded
- handling of the `.spec.successfulPolicy.onRequiredIndexes` when some indexes fail
- handling of the `.spec.successfulPolicy.onRequiredIndexesThreshold` when some indexes fail
- handling of the `.spec.successfulPolicy.requiredIndexes` when some indexes fail
- handling of the `.spec.successfulPolicy.minSucceededIndexes` when some indexes fail

##### e2e tests

Expand All @@ -518,8 +517,8 @@ We expect no non-infra related flakes in the last month as a GA graduation crite

- Test scenarios:
- handling of successfulPolicy when all indexes succeeded
- handling of the `.spec.successfulPolicy.onRequiredIndexes` when some indexes fail
- handling of the `.spec.successfulPolicy.onRequiredIndexesThreshold` when some indexes fail
- handling of the `.spec.successfulPolicy.requiredIndexes` when some indexes fail
- handling of the `.spec.successfulPolicy.minSucceededIndexes` when some indexes fail

### Graduation Criteria

Expand Down Expand Up @@ -951,8 +950,8 @@ Describe them, providing:
Yes, it will increase the size of existing API objects only when the `.spec.successfulPolicy` is set.

- API type(s): Job
- Estimated increase in size: `.spec.successPolicy.onRequiredIndexes` field are impacted.
In the worst case, the size of `onRequiredIndexes` can be estimated about 5.6MiB (see [The Job object too big](#the-job-object-too-big)).
- Estimated increase in size: `.spec.successPolicy.requiredIndexes` field are impacted.
In the worst case, the size of `requiredIndexes` can be estimated about 5.6MiB (see [The Job object too big](#the-job-object-too-big)).

###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs?

Expand Down Expand Up @@ -1062,6 +1061,36 @@ Currently, a job is restricted `.spec.completion!=nil`.
By relaxing the validation, the job can be declared as succeeded when some indexes succeeded,
similar to NonIndexed jobs.

### Hold indexes as []int typed in `.spec.successfulPolicy.onRequiredIndexes`

We can hold requiredIndexes as []int typed that a job can be declared as succeeded.

```golang
// SuccessfulPolicy describes how succeeded pods influence the conditions.
type SuccessfulPolicy struct {
// Specifies a set of required indexes.
// The job is declared as succeeded if a set of indexes are succeeded.
// The list of indexes must come within 0 to `.spec.completions` and
// must not contain duplicates. At least one element is required.
//
// +option
OnRequiredIndexes []int
...
}
```

However, if we allow users to set all indexes (`0-10^5`) to `.spec.successfulPolicy.onRequiredIndexes`
and don't limit the number of list sizes, there are cases that the size of a job object is too big.

In the worst case, allowed all indexes (`0-10^5`) are added to requiredIndexes, and a `onRequiredIndexes` size
is `SUM[9*10^n*(n+1)]+2+6≈5.6656MiB`, where:
- `n` starts from `0` and goes up to `5`.
- `1` of `n+1` means a separator that indicates `,`.
- `2` is the sum of the index `0` and `,`.
- `6` is the size of indexes `10^5`.

So, if we select this alternative API design, we need to limit the size of `onRequiredIndexes`.

## Infrastructure Needed (Optional)

<!--
Expand Down
2 changes: 1 addition & 1 deletion keps/sig-apps/3998-job-success-completion-policy/kep.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ latest-milestone: "v1.28"
milestone:
alpha: "v1.28"
beta: "v1.29"
stable: "v1.30"
stable:

# The following PRR answers are required at alpha release
# List the feature gate name and the components for which it must be enabled
Expand Down

0 comments on commit 87db3c1

Please sign in to comment.