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

Stop publishing Pod ResourceVersion in Endpoints and EndpointSlice API #108450

Merged
merged 1 commit into from
Mar 5, 2022
Merged

Stop publishing Pod ResourceVersion in Endpoints and EndpointSlice API #108450

merged 1 commit into from
Mar 5, 2022

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Mar 2, 2022 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This is to follow up #108078 (comment).
The field is not used anywhere and its value may be stale after #50934 and #108078.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

cc @aojea @liggitt @thockin @robscott

Does this PR introduce a user-facing change?

Endpoints and EndpointSlice controllers no longer populate [resourceVersion of targetRef](https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/object-reference/#ObjectReference) in Endpoints and EndpointSlices

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 2, 2022
@k8s-ci-robot
Copy link
Contributor

@tnqn: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 2, 2022
@tnqn
Copy link
Member Author

tnqn commented Mar 2, 2022

/sig network

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 2, 2022
@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Mar 2, 2022
liggitt
liggitt reviewed Mar 2, 2022
View reviewed changes
pkg/controller/util/endpoint/controller_utils_test.go Outdated
@@ -830,7 +830,7 @@ func TestEndpointsEqualBeyondHash(t *testing.T) {
expected: false,
},
{
name: "Pod resourceVersion changed",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we keep this test case as-is, and add a new test case to exercise "removed"?

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, I removed it because I thought there is no longer Pod resourceVersion changed case in main code. But yes, the function is still supposed to return true for this case.

liggitt
liggitt reviewed Mar 2, 2022
View reviewed changes
pkg/controller/util/endpoint/controller_utils_test.go Outdated
@@ -926,10 +926,10 @@ func TestEndpointSubsetsEqualIgnoreResourceVersion(t *testing.T) {
expected: false,
},
{
name: "Pod ResourceVersion changed",
Copy link
Member

Choose a reason for hiding this comment

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

same comment here about adding a test case and keeping this one as-is

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, thanks.

@liggitt
Copy link
Member

liggitt commented Mar 2, 2022

comments on tests but the change lgtm

@tnqn
Stop publishing Pod ResourceVersion in Endpoints and EndpointSlice API
906e6d4 
The field is not used anywhere and its value may be stale as Endpoints
and EndpointSlice won't be updated if there is only Pod ResourceVersion
change..
@tnqn tnqn force-pushed the stop-setting-rv-in-endpoints branch from e0c76cf to 906e6d4 Compare March 2, 2022 14:26
tnqn
tnqn commented Mar 2, 2022
View reviewed changes
Copy link
Member Author

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

Thanks @liggitt. Addressed your comments.

pkg/controller/util/endpoint/controller_utils_test.go Outdated
@@ -830,7 +830,7 @@ func TestEndpointsEqualBeyondHash(t *testing.T) {
expected: false,
},
{
name: "Pod resourceVersion changed",
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, I removed it because I thought there is no longer Pod resourceVersion changed case in main code. But yes, the function is still supposed to return true for this case.

pkg/controller/util/endpoint/controller_utils_test.go Outdated
@@ -926,10 +926,10 @@ func TestEndpointSubsetsEqualIgnoreResourceVersion(t *testing.T) {
expected: false,
},
{
name: "Pod ResourceVersion changed",
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, thanks.

@tnqn
Copy link
Member Author

tnqn commented Mar 2, 2022

/retest

1 similar comment
@tnqn
Copy link
Member Author

tnqn commented Mar 2, 2022

/retest

@robscott
Copy link
Member

robscott commented Mar 2, 2022 •
edited

Thanks @tnqn! The code LGTM, but I've got a tiny nit on the release note. The current one made me think that this was an API level change, not a controller one. Maybe something like this would be clearer:

Endpoints and EndpointSlice controllers no longer populate Pod ResourceVersion in Endpoints and EndpointSlices

@tnqn
Copy link
Member Author

tnqn commented Mar 3, 2022

Thanks @robscott. Release note updated as suggested.

@robscott
Copy link
Member

robscott commented Mar 3, 2022

Thanks! I can only partially approve this, assigning @freehan for the rest.

/lgtm
/approve
/assign @freehan

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

freehan commented Mar 3, 2022

/lgtm
/approve

@sftim
Copy link
Contributor

sftim commented Mar 3, 2022

For the changelog note, I'd like to make it clear that ResourceVersion is actually part of the targetRef of an endpoint within the endpoints field.

Is it feasible to use a hyperlink, eg resourceVersion?

@tnqn
Copy link
Member Author

tnqn commented Mar 3, 2022

@sftim I have updated "Pod ResourceVersion" to "resourceVersion of targetRef", hope it's clearer now. The releast note is wrapped by code block syntax, hyperlink won't be displayed.

@sftim
Copy link
Contributor

sftim commented Mar 3, 2022

You can use Markdown in release notes, right?

@tnqn
Copy link
Member Author

tnqn commented Mar 3, 2022

You can use Markdown in release notes, right?

updated as you suggested.

@thockin
Copy link
Member

thockin commented Mar 5, 2022

Should we mark the field as deprecated in the API comments?
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, robscott, thockin, tnqn

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 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit da23196 into kubernetes:master Mar 5, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 5, 2022
@tnqn tnqn deleted the stop-setting-rv-in-endpoints branch March 5, 2022 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@danwinship danwinship Awaiting requested review from danwinship

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees

@robscott robscott

@freehan freehan

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tnqn @k8s-ci-robot @liggitt @robscott @freehan @sftim @thockin