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

Document labels, annotations and taints for JobSet #47383

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Adarsh-verma-14
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 7, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2024
@Adarsh-verma-14
Copy link
Contributor Author

Hi @sftim ,could you review on these changes if anythings need to change let me know .

Copy link

netlify bot commented Aug 7, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit fc4c0c7
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6703725bf718730008f25363
😎 Deploy Preview https://deploy-preview-47383--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kannon92
Copy link
Contributor

kannon92 commented Aug 8, 2024

cc @ahg-g @danielvegamyhre


Used on: Jobs, Pods

This label is used to store the name of the JobSet to which a job or pod belongs.
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
This label is used to store the name of the JobSet to which a job or pod belongs.
This label is used to store the name of the JobSet to which a Job or Pod belongs.
JobSet is an extension API that you can deploy into your Kubernetes cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done this change as suggested.

Copy link
Member

@danielvegamyhre danielvegamyhre 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 working on this @kannon92, I added a couple comments

@kannon92
Copy link
Contributor

kannon92 commented Aug 8, 2024

Thanks for working on this @kannon92, I added a couple comments

@Adarsh-verma-14 is the one who worked on this.

Comment on lines 2427 to 2433
Type: Annotation

Example: `alpha.jobset.sigs.k8s.io/no-schedule: "true"`

Used on: Jobs

This annotation prevents the job from being scheduled.
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
Type: Annotation
Example: `alpha.jobset.sigs.k8s.io/no-schedule: "true"`
Used on: Jobs
This annotation prevents the job from being scheduled.
Type: Taint
Example: `alpha.jobset.sigs.k8s.io/no-schedule: NoSchedule`
Used on: Nodes
The JobSet controller uses this taint to support its node labeling exclusive placement strategy.
JobSet is an extension API that you can deploy into your Kubernetes cluster.

(aside: I'd rename this to jobset.sigs.k8s.io/exclusive-use or jobset.kubernetes.io/exclusive-use)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done this changes but is there need to change name (jobset.sigs.k8s.io/exclusive-use or jobset.kubernetes.io/exclusive-use) let me know.

@sftim
Copy link
Contributor

sftim commented Aug 8, 2024

There is also one taint, so:

/retitle Document labels, annotations and taints for JobSet

@k8s-ci-robot k8s-ci-robot changed the title Adding labels and annotation for JobSet Document labels, annotations and taints for JobSet Aug 8, 2024
@sftim
Copy link
Contributor

sftim commented Aug 8, 2024

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Aug 8, 2024
@danielvegamyhre
Copy link
Member

Thanks for working on this @kannon92, I added a couple comments

@Adarsh-verma-14 is the one who worked on this.

Woops, sorry about that @Adarsh-verma-14! Thanks for the contribution :)

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.

Very nearly LGTM for docs


Used on: JobSets

This annotation acts as a flag. When set, the JobSet controller injects nodeSelectors for the `JobSetNameKey` label (e.g., `jobset.sigs.k8s.io/jobset-name`) to ensure exclusive job placement per topology.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could tidy the explanation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This annotation acts as a flag. When it's set, the JobSet controller adds nodeSelectors for the JobSetNameKey label (like jobset.sigs.k8s.io/jobset-name), so jobs are placed according to a specific topology.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sftim it would be better or not ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What does the JobSet controller mutate based on this annotation?

  • JobSets
  • Jobs
  • Pods
    ?

It's not clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it your point @sftim , I will update this accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have update this accordingly. Now, i think is clear. PTAL!!

@tengqm
Copy link
Contributor

tengqm commented Sep 22, 2024

Sorry, I'm against this change because

  • JobSet is not something we can get from vanilla Kubernetes. Yes. The mentioned labels or annotations all carry the k8s.io domain name, which is reserved. But this doesn't mean that they are "official".
  • We could add notes (or warnings) to all these labels or annotations -- you can use them only when you deployed the jobset related manifests. They are not available by default on your cluster.
  • Or, better yet, document these labels/annotations on a different page. The current page is already lengthy (>2400 lines). Without differentiating builtin labels/annotations from those from other projects, we will end up with a huge file pretty soon.

@drackpack drackpack mentioned this pull request Sep 22, 2024
@sftim
Copy link
Contributor

sftim commented Sep 22, 2024

Thanks for the comment @tengqm, but JobSet is part of the Kubernetes project. I don't think we should force SIG Apps to reorganize a long page just to register their labels and annotations, even if that work is a good idea for the long term.

Copy link
Member

@danielvegamyhre danielvegamyhre left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple more small changes


Used on: Jobs, Pods

This label/annotation is used to store the name of the JobSet to which a Job or Pod belongs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This label/annotation is used to store the name of the JobSet to which a Job or Pod belongs.
This label/annotation is used to store the name of the JobSet that a Job or Pod belongs to.

@danielvegamyhre
Copy link
Member

/lgtm for JobSet

@sftim this is ready for SIG docs review

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.

There's no API kind called ReplicatedJob, right?

If that's true, try these changes. We main use the UpperCamelCase notation for actual API kinds.

/lgtm cancel


Used on: Jobs, Pods

This label/annotation is set by the JobSet controller on child Jobs and Pods of a JobSet. The value will be the SHA256 hash of the namespaced Job name.
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
This label/annotation is set by the JobSet controller on child Jobs and Pods of a JobSet. The value will be the SHA256 hash of the namespaced Job name.
The JobSet controller sets this label (and also an annotation with the same key) on child Jobs and
Pods of a JobSet. The value is the SHA256 hash of the namespaced Job name.

@danielvegamyhre
Copy link
Member

@Adarsh-verma-14 can you address sftim@'s comments as soon as you can please? The JobSet blog post PR is blocked by this one, so we are hoping to get it merged as soon as possible :)

@Adarsh-verma-14
Copy link
Contributor Author

@Adarsh-verma-14 can you address sftim@'s comments as soon as you can please? The JobSet blog post PR is blocked by this one, so we are hoping to get it merged as soon as possible :)

sure

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sftim. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@Adarsh-verma-14
Copy link
Contributor Author

Hi , I have updated these changes as suggested by @sftim . PTAL!!

@ahg-g
Copy link
Member

ahg-g commented Oct 20, 2024

Friendly ping @sftim


Example: `jobset.sigs.k8s.io/replicatedjob-replicas: "5"`

Used on: ReplicatedJobs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there definitely an API kind named ReplicatedJob? If so, what API group would I find this in?


Example: `alpha.jobset.sigs.k8s.io/exclusive-topology: "zone"`

Used on: JobSets, ReplicatedJobs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there definitely an API kind named ReplicatedJob? If so, what API group would I find this in?

Copy link
Member

Choose a reason for hiding this comment

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

There is no API kind named ReplicatedJob, but the annotation can be used on the JobTemplate defined inside the ReplicatedJob struct

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so Used on: would be JobSet, Job I think.
JobTemplate is also not an API kind (unlike say the unusual case of PodTemplate)


Type: Label, Annotation

Example: `alpha.jobset.sigs.k8s.io/node-selector-strategy=true`
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
Example: `alpha.jobset.sigs.k8s.io/node-selector-strategy=true`
Example: `alpha.jobset.sigs.k8s.io/node-selector-strategy: "true"`


Type: Annotation, Label

Example: `alpha.jobset.sigs.k8s.io/default_myjobset-replicatedjob-0`
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a value for the label.

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
Example: `alpha.jobset.sigs.k8s.io/default_myjobset-replicatedjob-0`
Example: `alpha.jobset.sigs.k8s.io/namespaced-job: default_myjobset-replicatedjob-0`

Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside, this will run into name length issues. The JobSet and Namespace can both have names that are up to 63 characters long, so the concatenation won't always fit into a label value.

(but not really relevant to this PR).


Type: Label, Annotation

Example: `jobset.sigs.k8s.io/job-key=0f1e93893c4cb372080804ddb9153093cb0d20cefdd37f653e739c232d363feb`
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
Example: `jobset.sigs.k8s.io/job-key=0f1e93893c4cb372080804ddb9153093cb0d20cefdd37f653e739c232d363feb`
Example: `jobset.sigs.k8s.io/job-key: 0f1e93893c4cb372080804ddb9153093cb0d20cefdd37f653e739c232d363feb`

This doesn't look right. Is 0f1e93893c4cb372080804ddb9153093cb0d20cefdd37f653e739c232d363feb definitely a valid label value? It looks longer than is allowed.

Copy link
Member

Choose a reason for hiding this comment

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

It is, we set the value as the SHA256 hash of the namespaced job name here.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but a SHA256 hash is 64 UTF-8 (or ASCII) characters, and conformant label values are documented as being up to 63 characters long.

Something doesn't add up. Maybe there's an off-by-one in the API server (I hope not).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, it's SHA1 not SHA256, my mistake. So it's only 40 characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Minor docs bug then; we can merge this with or without a fix (still would need a post-merge fixup).


Used on: Jobs, Pods

This label/annotation is set by the JobSet controller on child Jobs and Pods. It contains the index of the Job replica within its parent ReplicatedJob.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there definitely an API kind named ReplicatedJob? If so, what API group would I find this in?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, this needs fixing.

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.

We mainly just want to document what's there even if it's a bit awry. This is close but not yet right, though, and I think it does need another round of corrections.

@danielvegamyhre
Copy link
Member

@Adarsh-verma-14 can you address the last few comments here? Thanks!

@ahg-g
Copy link
Member

ahg-g commented Nov 18, 2024

Hello, anything blocking this one?

@sftim
Copy link
Contributor

sftim commented Nov 18, 2024

There's a lot of pending feedback. I noticed #47383 (comment)

@ahg-g
Copy link
Member

ahg-g commented Nov 19, 2024

since @Adarsh-verma-14 is not responding, I will clone it and submit a PR tomorrow to close this

@Adarsh-verma-14
Copy link
Contributor Author

sorry for late response, due to some engagement with my other work. it will take more time meanwhile if anyone wants to work on it, can work.

@ahg-g
Copy link
Member

ahg-g commented Dec 23, 2024

I created #49217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

JobSet labels and annotations not registered / documented
7 participants