Track ownership of scale subresource #98377

nodo · 2021-01-25T17:56:57Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Track ownership of scale subresource for all scalable resources i.e. Deployment, ReplicaSet, StatefulSet, ReplicationController, and Custom Resources.

Which issue(s) this PR fixes:
Fixes #82046
Fixes #94585

Special notes for your reviewer:
I opened #95921 as an initial attempt to solve the problem. As this PR follows a different approach, I decided to open a new PR for clarity.

Does this PR introduce a user-facing change?:

Track ownership of scale subresource for all scalable resources i.e. Deployment, ReplicaSet, StatefulSet, ReplicationController, and Custom Resources.

k8s-ci-robot · 2021-01-25T17:57:06Z

Hi @nodo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nodo · 2021-01-25T17:58:20Z

/cc @apelisse @kwiesmueller

apelisse · 2021-01-25T18:01:40Z

/assign
/triage accepted

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource_test.go

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource.go

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource_test.go

kwiesmueller · 2021-01-25T18:22:37Z

/wg api-expression

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource.go

pkg/registry/apps/deployment/storage/storage.go

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource.go

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource_test.go

pkg/registry/apps/deployment/storage/storage.go

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/subresource.go

apelisse · 2021-01-25T18:55:13Z

It's very encouraging that we're finally on the right track for this bug! I think there are a lot of improvements we can make to the code so that we don't fall into various traps (and most, if not all, my comments are related to that), but it's great that we're going the right direction!

pkg/registry/apps/deployment/storage/storage.go

apelisse · 2021-04-19T17:53:56Z

I think the only question left is the conversion! Thank you :-)

lavalamp · 2021-04-19T23:56:16Z

I don't think registry should be importing the client, if you have to can you at least change the package name (add "_test") so that the import graph isn't weird?

lgtm other than that.

nodo · 2021-04-20T08:23:16Z

/retest

apelisse · 2021-04-20T17:54:09Z

pkg/registry/apps/deployment/storage/storage.go

@@ -58,7 +58,7 @@ type DeploymentStorage struct {
 }

 // maps a group version to the replicas path in a deployment object
-var replicasPathInDeployment = fieldmanager.ResourcePathMappings{
+var ReplicasPathInDeployment = fieldmanager.ResourcePathMappings{


Making it public is fine, but it should be returned by a function so that people can't modify it. And let's move the test to integration, thanks!

Makes sense, will amend the commit

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go

nodo · 2021-04-21T16:49:07Z

The last commit is still WIP, I will squash it with a previous one.

- Test all versions to make sure each resource version is in the mappings - Fail when request info contains an unrecognized version. We have tests that guarantee that all known versions are in the mappings. If we get a version in request info that is not there we should fail fast to prevent inconsistent behaviour (e.g. for some reason the mappings is not up to date). Ensure all known versions are in mappings

This happens when a request changes the .status.replicas but not .spec.replicas

This is aligned to the behaviour of server-side apply on main resources.

This is to prevent the ScaleHandler to drop the entry. In this way entries just get ignored.

nodo · 2021-04-21T18:30:11Z

Squashed the commit and rebased.

liggitt

conversion change lgtm

liggitt · 2021-04-21T21:54:52Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go

@@ -165,6 +165,13 @@ func (c *crConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVe
 // TODO: should this be a typed error?
 return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target)
 }
+ // Special-case typed scale conversion if this custom resource supports a scale endpoint


apelisse · 2021-04-21T21:58:48Z

OK, Thanks @nodo!

/lgtm

Who wants to approve, @lavalamp @liggitt ?

liggitt · 2021-04-21T22:02:26Z

Who wants to approve, @lavalamp @liggitt ?

Daniel's approval still holds

apelisse · 2021-04-21T22:03:37Z

Ah, I missed it, thank you :-)

fejta-bot · 2021-04-22T01:18:16Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

apelisse · 2021-04-22T03:34:08Z

/retest

nodo · 2021-04-22T06:11:00Z

/retest

nodo · 2021-04-22T07:55:22Z

/retest

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver labels Jan 25, 2021

k8s-ci-robot requested review from aojea and hongchaodeng January 25, 2021 17:57

k8s-ci-robot added 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 do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 25, 2021

k8s-ci-robot requested review from apelisse and kwiesmueller January 25, 2021 17:58

k8s-ci-robot assigned apelisse Jan 25, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 25, 2021

nodo mentioned this pull request Jan 25, 2021

Track ownership for scale subresource of deployments #95921

Closed

kwiesmueller suggested changes Jan 25, 2021

View reviewed changes

k8s-ci-robot added the wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. label Jan 25, 2021

apelisse reviewed Jan 25, 2021

View reviewed changes

liggitt reviewed Jan 25, 2021

View reviewed changes

pkg/registry/apps/deployment/storage/storage.go Outdated Show resolved Hide resolved

apelisse reviewed Apr 20, 2021

View reviewed changes

liggitt reviewed Apr 20, 2021

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go Outdated Show resolved Hide resolved

nodo and others added 2 commits April 21, 2021 18:41

Track ownership of deployments scale subresource

a9ea98b

Use ScaleHandler for all scalable resources

816e802

nodo force-pushed the scale-ownership-tracking-deployment branch from f8a6ee7 to 41312b7 Compare April 21, 2021 16:42

nodo added 4 commits April 21, 2021 20:27

Do not add managed fields if a scale entry doesn't own replicas

8e4b5c8

This happens when a request changes the .status.replicas but not .spec.replicas

Drop managed fields entries with unknown fields

c10dd88

This is aligned to the behaviour of server-side apply on main resources.

Add nil path to mapping when a CR has no "scale" subresource

5b666a6

This is to prevent the ScaleHandler to drop the entry. In this way entries just get ignored.

nodo force-pushed the scale-ownership-tracking-deployment branch from 41312b7 to 5b666a6 Compare April 21, 2021 18:29

liggitt reviewed Apr 21, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2021

k8s-ci-robot merged commit f1e51ce into kubernetes:master Apr 22, 2021

nodo deleted the scale-ownership-tracking-deployment branch April 22, 2021 08:34

github-actions bot mentioned this pull request Apr 27, 2021

Week Ending April 25, 2021 dev-obs/actus#361

Open

timebertt mentioned this pull request Apr 28, 2021

Implement scale subresource for ManagedSeedSets gardener/gardener#3907

Closed

timebertt mentioned this pull request May 27, 2021

☂️-issue for Server-Side Apply in Gardener gardener/gardener#4122

Closed

pacoxu mentioned this pull request Jun 28, 2021

Server-side Apply: Ownership not tracked for sub-resources #88981

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track ownership of scale subresource #98377

Track ownership of scale subresource #98377

nodo commented Jan 25, 2021 •

edited

k8s-ci-robot commented Jan 25, 2021

nodo commented Jan 25, 2021

apelisse commented Jan 25, 2021

kwiesmueller commented Jan 25, 2021

apelisse commented Jan 25, 2021

apelisse commented Apr 19, 2021

lavalamp commented Apr 19, 2021

nodo commented Apr 20, 2021

apelisse Apr 20, 2021

nodo Apr 21, 2021

nodo commented Apr 21, 2021

nodo commented Apr 21, 2021

liggitt left a comment

liggitt Apr 21, 2021

apelisse commented Apr 21, 2021

liggitt commented Apr 21, 2021

apelisse commented Apr 21, 2021

fejta-bot commented Apr 22, 2021

apelisse commented Apr 22, 2021

nodo commented Apr 22, 2021

nodo commented Apr 22, 2021

Track ownership of scale subresource #98377

Track ownership of scale subresource #98377

Conversation

nodo commented Jan 25, 2021 • edited

k8s-ci-robot commented Jan 25, 2021

nodo commented Jan 25, 2021

apelisse commented Jan 25, 2021

kwiesmueller commented Jan 25, 2021

apelisse commented Jan 25, 2021

apelisse commented Apr 19, 2021

lavalamp commented Apr 19, 2021

nodo commented Apr 20, 2021

apelisse Apr 20, 2021

Choose a reason for hiding this comment

nodo Apr 21, 2021

Choose a reason for hiding this comment

nodo commented Apr 21, 2021

nodo commented Apr 21, 2021

liggitt left a comment

Choose a reason for hiding this comment

liggitt Apr 21, 2021

Choose a reason for hiding this comment

apelisse commented Apr 21, 2021

liggitt commented Apr 21, 2021

apelisse commented Apr 21, 2021

fejta-bot commented Apr 22, 2021

apelisse commented Apr 22, 2021

nodo commented Apr 22, 2021

nodo commented Apr 22, 2021

nodo commented Jan 25, 2021 •

edited