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

Documentation for Indexed completion mode #26958

Merged
merged 1 commit into from Mar 17, 2021

Conversation

alculquicondor
Copy link
Member

@alculquicondor alculquicondor commented Mar 8, 2021

@k8s-ci-robot k8s-ci-robot added this to the 1.21 milestone Mar 8, 2021
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. labels Mar 8, 2021
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Mar 8, 2021

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

Building with commit 552566f

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/60511b23a4aae00007b38942

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language labels Mar 8, 2021
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Mar 8, 2021
@annajung
Copy link
Contributor

annajung commented Mar 8, 2021

cc @reylejano

@reylejano
Copy link
Member

/assign

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 10, 2021
@alculquicondor alculquicondor changed the title WIP Documentation for Indexed completion mode Documentation for Indexed completion mode Mar 10, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2021
@alculquicondor
Copy link
Member Author

/assign @erictune

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks for this.

I have reviewed inline.

I wonder if there's a way to use a ready-made and reasonably trusted) public container image for the work task, something that can take an index in argv. If so, a bunch of my concerns disappear.

content/en/examples/application/job/indexed-job-args.yaml Outdated Show resolved Hide resolved
content/en/examples/application/job/indexed-job-env.yaml Outdated Show resolved Hide resolved
content/en/examples/application/job/indexed-job.yaml Outdated Show resolved Hide resolved
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.

I wonder if there's a way to use a ready-made and reasonably trusted) public container image for the work task, something that can take an index in argv. If so, a bunch of my concerns disappear.

I also don't quite like providing a program and dockerfile. I just followed the way of the existing job patterns tasks. I could just use busybox echo. WDYT?

@reylejano reylejano removed their assignment Mar 11, 2021
@reylejano
Copy link
Member

/assign @PI-Victor

@reylejano
Copy link
Member

@kubernetes/sig-apps-pr-reviews Can you provide a tech review?

@sftim
Copy link
Contributor

sftim commented Mar 11, 2021

I could just use busybox echo.

How about:

  • init container 1: write task input to path in volume based on index
  • init container 2: sleep
  • app container: do work (rev from Busybox, perhaps)

Another approach is also OK. Could run a script in a Linux OS-type container for example, providing the code via the command line.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2021
@@ -185,6 +184,33 @@ parallelism, for a variety of reasons:
- The Job controller may throttle new Pod creation due to excessive previous pod failures in the same Job.
- When a Pod is gracefully shut down, it takes time to stop.

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

BTW, this would work better as a level 3 heading so that it's part of in-page navigation.

Suggested change
#### Completion Mode
### Completion mode

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


<!-- steps -->

## Create a container image
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Create a container image
## Choose an approach

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

Comment on lines 69 to 78
This program is available in the [busybox container image](https://hub.docker.com/_/busybox):

{{< tabs name="busybox container image" >}}
{{{< tab name="Docker Hub" >}}
docker.io/library/busybox
{{< /tab >}}
{{< tab name="Google Container Registry" >}}
mirror.gcr.io/library/busybox
{{< /tab >}}}
{{< /tabs >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're not pushing images, my earlier feedback doesn't apply and this can simplify to:

Suggested change
This program is available in the [busybox container image](https://hub.docker.com/_/busybox):
{{< tabs name="busybox container image" >}}
{{{< tab name="Docker Hub" >}}
docker.io/library/busybox
{{< /tab >}}
{{< tab name="Google Container Registry" >}}
mirror.gcr.io/library/busybox
{{< /tab >}}}
{{< /tabs >}}
For this example, you'll use the `rev` tool from the [`busybox`](https://hub.docker.com/_/busybox) container image.

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

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks, this is a good docs improvement @alculquicondor !

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

LGTM label has been added.

Git tree hash: c61bd02b8ed9b91d7c8341203947011f96bab663

@reylejano
Copy link
Member

/approve

@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 16, 2021
@reylejano
Copy link
Member

reylejano commented Mar 16, 2021

/approve cancel
Tim's question came in a minute before my initial approve

BTW is batch.alpha.kubernetes.io still correct?

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2021
Signed-off-by: Aldo Culquicondor <acondor@google.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2021
@alculquicondor
Copy link
Member Author

/hold cancel

@reylejano
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano, 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2021
@sftim
Copy link
Contributor

sftim commented Mar 17, 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 Mar 17, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 8b70b23feb526919ee8b5a009b01e38bf66a0d82

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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet