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

Server-Side Apply status wiping #99661

Merged
merged 2 commits into from Mar 10, 2021
Merged

Server-Side Apply status wiping #99661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d66477d
Make conditions list on CRDStatus non-atomic
kevindelgado Mar 9, 2021
a1fac8c
Server-Side Apply: Status Wiping/Reset Fields
kevindelgado Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions api/api-rules/violation_exceptions.list
View file
Open in desktop
Expand Up @@ -270,7 +270,6 @@ API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/ap
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,CustomResourceDefinitionNames,Categories
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,CustomResourceDefinitionNames,ShortNames
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,CustomResourceDefinitionSpec,Versions
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,CustomResourceDefinitionStatus,Conditions
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,CustomResourceDefinitionStatus,StoredVersions
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,CustomResourceDefinitionVersion,AdditionalPrinterColumns
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1,JSON,Raw
Expand All @@ -291,7 +290,6 @@ API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/ap
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,CustomResourceDefinitionNames,ShortNames
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,CustomResourceDefinitionSpec,AdditionalPrinterColumns
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,CustomResourceDefinitionSpec,Versions
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,CustomResourceDefinitionStatus,Conditions
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,CustomResourceDefinitionStatus,StoredVersions
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,CustomResourceDefinitionVersion,AdditionalPrinterColumns
API rule violation: list_type_missing,k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1,JSON,Raw
Expand Down
12 changes: 10 additions & 2 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Expand Up @@ -137,6 +137,7 @@ require (
k8s.io/sample-apiserver v0.0.0
k8s.io/system-validators v1.4.0
k8s.io/utils v0.0.0-20201110183641-67b214c5f920
sigs.k8s.io/structured-merge-diff/v4 v4.0.3
sigs.k8s.io/yaml v1.2.0
)

Expand Down
9 changes: 8 additions & 1 deletion hack/make-rules/test-cmd.sh
View file
Open in desktop
Expand Up @@ -130,6 +130,10 @@ EOF
# Creates a node object with name 127.0.0.1. This is required because we do not
# run kubelet.
#
# An arbitrary annotation is needed to ensure field managers are saved on the
# object. Without it, we would be creating an empty object and because status
# and name get wiped, there were be no field managers tracking any fields.
#
# Exports:
# SUPPORTED_RESOURCES(Array of all resources supported by the apiserver).
function create_node() {
Expand All @@ -138,7 +142,10 @@ function create_node() {
"kind": "Node",
"apiVersion": "v1",
"metadata": {
"name": "127.0.0.1"
"name": "127.0.0.1",
"annotations": {
"save-managers": "true"
}
},
"status": {
"capacity": {
Expand Down
16 changes: 12 additions & 4 deletions pkg/registry/apiserverinternal/storageversion/storage/storage.go
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ import (
printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
printerstorage "k8s.io/kubernetes/pkg/printers/storage"
strategy "k8s.io/kubernetes/pkg/registry/apiserverinternal/storageversion"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// REST implements a RESTStorage for storage version against etcd
Expand All @@ -46,17 +47,19 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {
},
DefaultQualifiedResource: apiserverinternal.Resource("storageversions"),

CreateStrategy: strategy.Strategy,
UpdateStrategy: strategy.Strategy,
DeleteStrategy: strategy.Strategy,
TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
CreateStrategy: strategy.Strategy,
UpdateStrategy: strategy.Strategy,
DeleteStrategy: strategy.Strategy,
ResetFieldsStrategy: strategy.Strategy,
TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
}
options := &generic.StoreOptions{RESTOptions: optsGetter}
if err := store.CompleteWithOptions(options); err != nil {
return nil, nil, err
}
statusStore := *store
statusStore.UpdateStrategy = strategy.StatusStrategy
statusStore.ResetFieldsStrategy = strategy.StatusStrategy
return &REST{store}, &StatusREST{store: &statusStore}, nil
}

Expand All @@ -81,3 +84,8 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat
// subresources should never allow create on update.
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, false, options)
}

// GetResetFields implements rest.ResetFieldsStrategy
Copy link
Member

Choose a reason for hiding this comment

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

This function name is not very clear. "Reset" is a verb at least as often as a noun which makes it especially confusing.

What is the difference between "reset" and "disabled" fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case "Reset" is an adjective. A "reset field" is a field that should be cleared from the field managers set of fields that it owns.

We've been looking for a better name for over a year and haven't found one I guess https://github.com/kubernetes/enhancements/pull/1123/files#r356309605

A disabled field is a field on an object used by a feature flag that is disabled.

Copy link
Member

Choose a reason for hiding this comment

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

Also not happy with the naming, I remember we talked about this in the past (like a year ago) but didn't find something better. In SMD it's called ignoredFields right now as it suits the specific context more, but ignoredFields does not really fit here I think.

func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return r.store.GetResetFields()
}
26 changes: 26 additions & 0 deletions pkg/registry/apiserverinternal/storageversion/strategy.go
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ import (
"k8s.io/kubernetes/pkg/api/legacyscheme"
"k8s.io/kubernetes/pkg/apis/apiserverinternal"
"k8s.io/kubernetes/pkg/apis/apiserverinternal/validation"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// storageVersionStrategy implements verification logic for StorageVersion.
Expand All @@ -42,6 +43,18 @@ func (storageVersionStrategy) NamespaceScoped() bool {
return false
}

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
func (storageVersionStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
fields := map[fieldpath.APIVersion]*fieldpath.Set{
"internal.apiserver.k8s.io/v1alpha1": fieldpath.NewSet(
fieldpath.MakePathOrDie("status"),
),
}

return fields
}

// PrepareForCreate clears the status of an StorageVersion before creation.
func (storageVersionStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
sv := obj.(*apiserverinternal.StorageVersion)
Expand Down Expand Up @@ -90,6 +103,19 @@ type storageVersionStatusStrategy struct {
// StatusStrategy is the default logic invoked when updating object status.
var StatusStrategy = storageVersionStatusStrategy{Strategy}

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
func (storageVersionStatusStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
fields := map[fieldpath.APIVersion]*fieldpath.Set{
"internal.apiserver.k8s.io/v1alpha1": fieldpath.NewSet(
fieldpath.MakePathOrDie("metadata"),
fieldpath.MakePathOrDie("spec"),
),
}

return fields
}

func (storageVersionStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
newSV := obj.(*apiserverinternal.StorageVersion)
oldSV := old.(*apiserverinternal.StorageVersion)
Expand Down
14 changes: 11 additions & 3 deletions pkg/registry/apps/daemonset/storage/storage.go
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ import (
printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
printerstorage "k8s.io/kubernetes/pkg/printers/storage"
"k8s.io/kubernetes/pkg/registry/apps/daemonset"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// REST implements a RESTStorage for DaemonSets
Expand All @@ -44,9 +45,10 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {
NewListFunc: func() runtime.Object { return &apps.DaemonSetList{} },
DefaultQualifiedResource: apps.Resource("daemonsets"),

CreateStrategy: daemonset.Strategy,
UpdateStrategy: daemonset.Strategy,
DeleteStrategy: daemonset.Strategy,
CreateStrategy: daemonset.Strategy,
UpdateStrategy: daemonset.Strategy,
DeleteStrategy: daemonset.Strategy,
ResetFieldsStrategy: daemonset.Strategy,

TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
}
Expand All @@ -57,6 +59,7 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {

statusStore := *store
statusStore.UpdateStrategy = daemonset.StatusStrategy
statusStore.ResetFieldsStrategy = daemonset.StatusStrategy

return &REST{store, []string{"all"}}, &StatusREST{store: &statusStore}, nil
}
Expand Down Expand Up @@ -103,3 +106,8 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat
// subresources should never allow create on update.
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, false, options)
}

// GetResetFields implements rest.ResetFieldsStrategy
func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return r.store.GetResetFields()
}
23 changes: 23 additions & 0 deletions pkg/registry/apps/daemonset/strategy.go
View file
Open in desktop
Expand Up @@ -36,6 +36,7 @@ import (
"k8s.io/kubernetes/pkg/apis/apps"
"k8s.io/kubernetes/pkg/apis/apps/validation"
"k8s.io/kubernetes/pkg/features"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// daemonSetStrategy implements verification logic for daemon sets.
Expand Down Expand Up @@ -68,6 +69,18 @@ func (daemonSetStrategy) NamespaceScoped() bool {
return true
}

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
func (daemonSetStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
fields := map[fieldpath.APIVersion]*fieldpath.Set{
Copy link
Member

Choose a reason for hiding this comment

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

How many times are these functions called? It would be a shame to construct this map for every request.

Copy link
Member

Choose a reason for hiding this comment

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

They're called at initialization time, once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For builtin types it gets called by the endpoints installer so it shouldn't be constructed for every request, right?

https://github.com/kubernetes/kubernetes/pull/99661/files#diff-a8da7b9bda33b6055a1f5deb7d0c33058c5d706ec5b44c665191a359a43160c6R265

And for CRDs it's built by the scope which is called in getOrCreateServingInfoFor, so again I don't think it's on every request.

https://github.com/kubernetes/kubernetes/pull/99661/files#diff-01e7a2059fe62e95ec32e2589c01b7218100de327647c641c334d38f2c5aa06eR967

Copy link
Member

Choose a reason for hiding this comment

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

Actually, good point Kevin: "getOrCreateServingInfoFor" is (surprisingly) called for every request!

"apps/v1": fieldpath.NewSet(
fieldpath.MakePathOrDie("status"),
),
}

return fields
}

// PrepareForCreate clears the status of a daemon set before creation.
func (daemonSetStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
daemonSet := obj.(*apps.DaemonSet)
Expand Down Expand Up @@ -202,6 +215,16 @@ type daemonSetStatusStrategy struct {
// StatusStrategy is the default logic invoked when updating object status.
var StatusStrategy = daemonSetStatusStrategy{Strategy}

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
func (daemonSetStatusStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return map[fieldpath.APIVersion]*fieldpath.Set{
"apps/v1": fieldpath.NewSet(
fieldpath.MakePathOrDie("spec"),
),
}
}

func (daemonSetStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
newDaemonSet := obj.(*apps.DaemonSet)
oldDaemonSet := old.(*apps.DaemonSet)
Expand Down
14 changes: 11 additions & 3 deletions pkg/registry/apps/deployment/storage/storage.go
View file
Open in desktop
Expand Up @@ -43,6 +43,7 @@ import (
printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
printerstorage "k8s.io/kubernetes/pkg/printers/storage"
"k8s.io/kubernetes/pkg/registry/apps/deployment"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// DeploymentStorage includes dummy storage for Deployments and for Scale subresource.
Expand Down Expand Up @@ -81,9 +82,10 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, *Rollbac
NewListFunc: func() runtime.Object { return &apps.DeploymentList{} },
DefaultQualifiedResource: apps.Resource("deployments"),

CreateStrategy: deployment.Strategy,
UpdateStrategy: deployment.Strategy,
DeleteStrategy: deployment.Strategy,
CreateStrategy: deployment.Strategy,
UpdateStrategy: deployment.Strategy,
DeleteStrategy: deployment.Strategy,
ResetFieldsStrategy: deployment.Strategy,

TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
}
Expand All @@ -94,6 +96,7 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, *Rollbac

statusStore := *store
statusStore.UpdateStrategy = deployment.StatusStrategy
statusStore.ResetFieldsStrategy = deployment.StatusStrategy
return &REST{store, []string{"all"}}, &StatusREST{store: &statusStore}, &RollbackREST{store: store}, nil
}

Expand Down Expand Up @@ -141,6 +144,11 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, false, options)
}

// GetResetFields implements rest.ResetFieldsStrategy
func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return r.store.GetResetFields()
}

// RollbackREST implements the REST endpoint for initiating the rollback of a deployment
type RollbackREST struct {
store *genericregistry.Store
Expand Down
24 changes: 24 additions & 0 deletions pkg/registry/apps/deployment/strategy.go
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ import (
"k8s.io/kubernetes/pkg/api/pod"
"k8s.io/kubernetes/pkg/apis/apps"
"k8s.io/kubernetes/pkg/apis/apps/validation"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// deploymentStrategy implements behavior for Deployments.
Expand Down Expand Up @@ -67,6 +68,18 @@ func (deploymentStrategy) NamespaceScoped() bool {
return true
}

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
func (deploymentStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
fields := map[fieldpath.APIVersion]*fieldpath.Set{
"apps/v1": fieldpath.NewSet(
fieldpath.MakePathOrDie("status"),
),
}

return fields
}

// PrepareForCreate clears fields that are not allowed to be set by end users on creation.
func (deploymentStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
deployment := obj.(*apps.Deployment)
Expand Down Expand Up @@ -147,6 +160,17 @@ type deploymentStatusStrategy struct {
// StatusStrategy is the default logic invoked when updating object status.
var StatusStrategy = deploymentStatusStrategy{Strategy}

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
func (deploymentStatusStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return map[fieldpath.APIVersion]*fieldpath.Set{
"apps/v1": fieldpath.NewSet(
fieldpath.MakePathOrDie("spec"),
fieldpath.MakePathOrDie("metadata", "labels"),
),
}
}

// PrepareForUpdate clears fields that are not allowed to be set by end users on update of status
func (deploymentStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
newDeployment := obj.(*apps.Deployment)
Expand Down
14 changes: 11 additions & 3 deletions pkg/registry/apps/replicaset/storage/storage.go
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ import (
printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
printerstorage "k8s.io/kubernetes/pkg/printers/storage"
"k8s.io/kubernetes/pkg/registry/apps/replicaset"
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
)

// ReplicaSetStorage includes dummy storage for ReplicaSets and for Scale subresource.
Expand Down Expand Up @@ -77,9 +78,10 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {
PredicateFunc: replicaset.MatchReplicaSet,
DefaultQualifiedResource: apps.Resource("replicasets"),

CreateStrategy: replicaset.Strategy,
UpdateStrategy: replicaset.Strategy,
DeleteStrategy: replicaset.Strategy,
CreateStrategy: replicaset.Strategy,
UpdateStrategy: replicaset.Strategy,
DeleteStrategy: replicaset.Strategy,
ResetFieldsStrategy: replicaset.Strategy,

TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
}
Expand All @@ -90,6 +92,7 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, error) {

statusStore := *store
statusStore.UpdateStrategy = replicaset.StatusStrategy
statusStore.ResetFieldsStrategy = replicaset.StatusStrategy

return &REST{store, []string{"all"}}, &StatusREST{store: &statusStore}, nil
}
Expand Down Expand Up @@ -138,6 +141,11 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat
return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, false, options)
}

// GetResetFields implements rest.ResetFieldsStrategy
func (r *StatusREST) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set {
return r.store.GetResetFields()
}

// ScaleREST implements a Scale for ReplicaSet.
type ScaleREST struct {
store *genericregistry.Store
Expand Down