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 setting SelfLink in kube-apiserver. #94397

Merged
merged 1 commit into from Sep 3, 2020
Merged

Stop setting SelfLink in kube-apiserver. #94397

merged 1 commit into from Sep 3, 2020

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Sep 1, 2020

Ref kubernetes/enhancements#1164

Stop propagating SelfLink (deprecated in 1.16) in kube-apiserver

/kind cleanup
/priority important-soon

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/apiserver area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 1, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2020
lavalamp
lavalamp reviewed Sep 1, 2020
View reviewed changes
Copy link
Member

@lavalamp lavalamp left a comment

Choose a reason for hiding this comment

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

only minor comments.

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go Outdated
@@ -1144,8 +1144,7 @@ func TestList(t *testing.T) {
t.Logf("%d: body: %s", i, string(body))
continue
}
// TODO: future, restore get links
if !selfLinker.called {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) == selfLinker.called {
t.Errorf("%d: never set self link", i)
Copy link
Member

Choose a reason for hiding this comment

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

this error message is no longer correct for all the ways you could get into this if statement.

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

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go Outdated
@@ -1279,8 +1278,7 @@ func TestListCompression(t *testing.T) {
t.Logf("%d: body: %s", i, string(body))
continue
}
// TODO: future, restore get links
if !selfLinker.called {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) == selfLinker.called {
t.Errorf("%d: never set self link", i)
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 (not that it matters that much)

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

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go Outdated
@@ -1596,7 +1600,7 @@ func TestExport(t *testing.T) {
t.Errorf("Expected: exported, saw: %s", itemOut.Other)
}

if !selfLinker.called {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) == selfLinker.called {
t.Errorf("Never set self link")
Copy link
Member

Choose a reason for hiding this comment

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

I'll stop commenting on these :)

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

staging/src/k8s.io/apiserver/pkg/features/kube_features.go
@@ -159,7 +159,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
StorageVersionHash: {Default: true, PreRelease: featuregate.Beta},
WatchBookmark: {Default: true, PreRelease: featuregate.GA, LockToDefault: true},
APIPriorityAndFairness: {Default: false, PreRelease: featuregate.Alpha},
RemoveSelfLink: {Default: false, PreRelease: featuregate.Alpha},
RemoveSelfLink: {Default: true, PreRelease: featuregate.Beta},
Copy link
Member

Choose a reason for hiding this comment

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

Wait, this was already here? Do we not test feature gates in both positions in tests?

Did it work before? If it didn't work before, then no one could possibly have tested it and it's not clear to me that it's fair or safe to go to beta.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently not in unit tests...

The feature gate was there and the self-linking logic itself was gated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually - it seems that only a few e2e tests are running with all Alpha features enabled:
https://github.com/kubernetes/test-infra/blob/03cbb16ef550658d61fb14912d3dd19b1cd7fa21/config/jobs/kubernetes/sig-cloud-provider/gcp/gcp-gce.yaml#L428

This is why all of these weren't caught...

@wojtek-t
Copy link
Member Author

wojtek-t commented Sep 1, 2020

/hold

"TestEmptyList" integration test seem to be a real failure I don't really understand yet. Will investigate in the next few days.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020
@k8s-ci-robot k8s-ci-robot added 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. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 1, 2020
wojtek-t
wojtek-t commented Sep 1, 2020
View reviewed changes
Copy link
Member Author

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Comments applied, failing test fixed (it was a real issue).

/hold cancel

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go Outdated
@@ -1144,8 +1144,7 @@ func TestList(t *testing.T) {
t.Logf("%d: body: %s", i, string(body))
continue
}
// TODO: future, restore get links
if !selfLinker.called {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) == selfLinker.called {
t.Errorf("%d: never set self link", i)
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

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go Outdated
@@ -1279,8 +1278,7 @@ func TestListCompression(t *testing.T) {
t.Logf("%d: body: %s", i, string(body))
continue
}
// TODO: future, restore get links
if !selfLinker.called {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) == selfLinker.called {
t.Errorf("%d: never set self link", i)
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

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go Outdated
@@ -1596,7 +1600,7 @@ func TestExport(t *testing.T) {
t.Errorf("Expected: exported, saw: %s", itemOut.Other)
}

if !selfLinker.called {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) == selfLinker.called {
t.Errorf("Never set self link")
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

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go
@@ -328,6 +328,12 @@ func checkName(obj runtime.Object, name, namespace string, namer ScopeNamer) err
// interfaces
func setObjectSelfLink(ctx context.Context, obj runtime.Object, req *http.Request, namer ScopeNamer) error {
if utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) {
// Ensure that for empty lists we don't return <nil> items.
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI - this was clearly a bug previously. Fortunately it was caught by an existing integration test.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you comment why the fix is here in setObjectSelfLink? Doesn't sound like this func's task is to set an items slice to non-nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency - before this change (i.e. before adding this branch with return) - setting that list to non-nil was happening before in this function too. I mean - it still does - see line 370.

I agree it would be better to move it elsewhere, but that's orthogonal to this PR.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020
@wojtek-t
Copy link
Member Author

wojtek-t commented Sep 2, 2020

Ref #94414

/retest

@wojtek-t
Copy link
Member Author

wojtek-t commented Sep 2, 2020

@lavalamp @deads2k - it's ready for review now

@lavalamp
Copy link
Member

lavalamp commented Sep 2, 2020

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, wojtek-t

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

@wojtek-t
Copy link
Member Author

wojtek-t commented Sep 3, 2020

@lavalamp - are you ok with applying the milestone now?

@lavalamp
Copy link
Member

lavalamp commented Sep 3, 2020 via email

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Sep 3, 2020
@k8s-ci-robot k8s-ci-robot merged commit f736be5 into kubernetes:master Sep 3, 2020
@liggitt liggitt mentioned this pull request Sep 9, 2020
Closed
@nevalla nevalla mentioned this pull request Dec 14, 2020
Closed
@knight42 knight42 mentioned this pull request Jan 14, 2021
Closed
@toneill818 toneill818 mentioned this pull request Jun 3, 2021
Closed
@AlexLast AlexLast mentioned this pull request Jul 6, 2021
Closed
This was referenced Nov 24, 2021
Closed
Merged
@wozniakjan wozniakjan mentioned this pull request Jan 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@jpbetz jpbetz Awaiting requested review from jpbetz

@sttts sttts Awaiting requested review from sttts

Assignees

@lavalamp lavalamp

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wojtek-t @lavalamp @k8s-ci-robot @sttts @deads2k