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.

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

Tweak task for Indexed completion Job example #27103

Merged

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Mar 17, 2021

A few changes to round off the new task added in PR #26958

/kind cleanup
/sig apps
/milestone 1.21
/priority backlog

/cc alculquicondor

@k8s-ci-robot k8s-ci-robot added this to the 1.21 milestone Mar 17, 2021
@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 09cf80c

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/605246ce40b45e000869e98b

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/apps Categorizes an issue or PR as relevant to SIG Apps. priority/backlog Higher priority than priority/awaiting-more-evidence. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Mar 17, 2021
Copy link
Member

@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.

Thanks

/lgtm

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

LGTM label has been added.

Git tree hash: 20b75d9254f3b83b0a382780c5c8e96fedb01bd4

1. **Define a Job manifest using indexed completion**.
The downward API allows you to pass the pod index annotation as an
environment variable or file to the container.
2. **Start an `Indexed` Job based on that manifest**.
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @sftim ,

Is there a reason to use bold and code formatting in the instructions?
An indexed Job is a Job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The formatting here is as per the original.

Copy link
Member

Choose a reason for hiding this comment

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

An Indexed Job is a Job with .spec.completionMode=Indexed. Bold could be omitted. I just think it makes it easier to get the gist of the steps.

@@ -66,13 +66,21 @@ program accepts a file as an argument and prints its content reversed.
rev data.txt
```

For this example, you'll use the `rev` tool from the
You'll use the `rev` tool from the
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 69 seems confusing. Could you combine that sentence with lines 61-63?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could omit this paragraph - @alculquicondor is that OK?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

[`busybox`](https://hub.docker.com/_/busybox) container image.

As this is only an example, each Pod only does a tiny piece of work (reversing a short
string). In a real workload you might, for example, create a Job that represents
the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the empty spaces in line 74

string). In a real workload you might, for example, create a Job that represents
the
task of producing 60 seconds of video based on scene data.
Each work item in the video rendering Job would be to render a particular
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: reword
Each work item in the video rendering Job renders a particular frame ...

## Define an Indexed Job

Here is a job definition. You'll need to edit the container image to match your
preferred registry.
Here is a sample Job manifest that uses `Indexed` completion mode:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: What is "Indexed" completion mode? Should this be capitalized and use code formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The page prerequisites direct the reader to read up about Job first, which (in the v1.21 docs) explains that mode. We could link directly to that part of the page to emphasize this?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary. The following paragraph is the example, which shows how the completion mode is set in the spec.

```

Wait a bit, then check on the job:
When you create this Job, the control plane creates a series of Pods, one for each index you specified. The value of `.spec.parallelism` determines how many can run at once whereas `.spec.completions` determines how many Pods the Job creates in total.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The value of ... determines how many Pods (?) can run [immediately or at the same time]?

@kbhawkey
Copy link
Contributor

Since this is for 1.21,
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit a8ebc3c into kubernetes:dev-1.21 Mar 26, 2021
@sftim sftim deleted the 20210317_tweak_indexed_job_task branch June 9, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants