Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
Document labels, annotations and taints for JobSet #47383
Changes from 6 commits
099e386
0d815b6
1be0ff2
eabf9c1
1fb8971
d21c85e
fc4c0c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no ReplicatedJob API, it is just part of the JobSet API: https://github.com/kubernetes-sigs/jobset/blob/b92cbdcbe77f168a82cf12c9f349bdd053d2ae5c/api/jobset/v1alpha2/jobset_types.go#L85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this needs fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. Is
0f1e93893c4cb372080804ddb9153093cb0d20cefdd37f653e739c232d363feb
definitely a valid label value? It looks longer than is allowed.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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).