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.

Sign up for GitHub

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

Jump to bottom

Register Annotation resource.kubernetes.io/pod-claim-name #47093

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Register Annotation resource.kubernetes.io/pod-claim-name #47093

merged 3 commits into from
Oct 2, 2024

Conversation

abhilasha2418
Copy link
Contributor

Closes #47044

Problem: The annotation resource.kubernetes.io/pod-claim-name is unregistered, but used in Kubernetes code.

Changes: Registered the annotation, by mentioning it in Well-Known Labels, Annotations and Taints.

Additional Information:
Required for kubernetes/enhancements#3063

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 4, 2024
@abhilasha2418
Copy link
Contributor Author

/assign

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 4, 2024
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jul 4, 2024
@netlify Netlify
Copy link

netlify bot commented Jul 4, 2024 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit b2bb082
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/668796e78644f20008c4ef9d
😎 Deploy Preview https://deploy-preview-47093--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.

sftim
sftim reviewed Jul 4, 2024
View reviewed changes
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.

I think these suggestions are correct, but I also recommend a tech review.

content/en/docs/reference/labels-annotations-taints/_index.md Outdated Show resolved Hide resolved
content/en/docs/reference/labels-annotations-taints/_index.md Outdated

Example: `resource.kubernetes.io/pod-claim-name: "my-pod-claim"`

Used on: Pod
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
Used on: Pod
Used on: ResourceClaim

content/en/docs/reference/labels-annotations-taints/_index.md Outdated Show resolved Hide resolved
@abhilasha2418 abhilasha2418 requested a review from sftim July 5, 2024 06:48
@abhilasha2418
Copy link
Contributor Author

@sftim I've added the suggestion, can you please review thanks!

@T-Lakshmi
Copy link
Contributor

/sig node
LGTM

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jul 8, 2024
@T-Lakshmi
Copy link
Contributor

Preview page

@T-Lakshmi
Copy link
Contributor

Hello @kubernetes/sig-node-leads,

I think these suggestions are correct, but I also recommend a tech review.

We'd like to have a technical review of this PR.
Could you please spare some time here?

@T-Lakshmi T-Lakshmi mentioned this pull request Jul 8, 2024
Closed
@abhilasha2418
Copy link
Contributor Author

Hello @kubernetes/sig-node-leads can someone review this PR that will be really helpful

bart0sh
bart0sh reviewed Jul 18, 2024
View reviewed changes
content/en/docs/reference/labels-annotations-taints/_index.md
Its value corresponds to the name of the resource claim in the `.spec` of any Pod(s) for which the ResourceClaim was created.
This annotation is an internal implementation detail of [dynamic resource allocation](/docs/concepts/scheduling-eviction/dynamic-resource-allocation/).
You should not need to read or modify the value of this annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's mentioned here that this annotation is used only inside the controller and not documented as part of the Kubernetes API.
It looks like it's intentionally not documented. @pohly is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed intentional.

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 per #47044 we don't have a way to register annotations without also documenting them.

Copy link
Contributor

Choose a reason for hiding this comment

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

(adding such a mechanism is welcome, and it's not actually a huge amount of work, but it'd be bigger than this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can document them here. It just makes less sense to also document it in the public types.go as a const string.

Copy link
Contributor

Choose a reason for hiding this comment

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

/lgtm

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

LGTM label has been added.

Git tree hash: 3ae76c08abf449a9691b42d819de0d959b1bd8d7

@bart0sh
Copy link
Contributor

bart0sh commented Sep 19, 2024

/assign @sftim
for approval

sftim
sftim reviewed Oct 2, 2024
View reviewed changes
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.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 Oct 2, 2024
@k8s-ci-robot k8s-ci-robot merged commit 6f101d3 into kubernetes:main Oct 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@bart0sh bart0sh bart0sh left review comments

@sftim sftim sftim left review comments

@natalisucks natalisucks Awaiting requested review from natalisucks

@shannonxtreme shannonxtreme Awaiting requested review from shannonxtreme

Assignees

@bart0sh bart0sh

@sftim sftim

@abhilasha2418 abhilasha2418

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/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Annotation resource.kubernetes.io/pod-claim-name not registered or documented
6 participants
@abhilasha2418 @T-Lakshmi @k8s-ci-robot @bart0sh @pohly @sftim