diff --git a/keps/sig-api-machinery/0006-apply.md b/keps/sig-api-machinery/0006-apply.md index 774a36157c2..a20f551e2ef 100644 --- a/keps/sig-api-machinery/0006-apply.md +++ b/keps/sig-api-machinery/0006-apply.md @@ -39,12 +39,17 @@ superseded-by: - [Maps and structs](#maps-and-structs) - [Kubectl](#kubectl) - [Server-side Apply](#server-side-apply) + - [Status Wiping](#status-wiping) + - [Current Behavior](#current-behavior) + - [Proposed Change](#proposed-change) + - [Alternatives](#alternatives) + - [Implementation History](#implementation-history) - [Risks and Mitigations](#risks-and-mitigations) - [Testing Plan](#testing-plan) - [Graduation Criteria](#graduation-criteria) -- [Implementation History](#implementation-history) +- [Implementation History](#implementation-history-1) - [Drawbacks](#drawbacks) -- [Alternatives](#alternatives) +- [Alternatives](#alternatives-1) ## Summary @@ -143,11 +148,6 @@ The linked documents should be read for a more complete picture. (TODO: update this section with current design) -What are the caveats to the implementation? -What are some important details that didn't come across above. -Go in to as much detail as necessary here. -This might be a good place to talk about core concepts and how they releate. - #### API Topology Server-side apply has to understand the topology of the objects in order to make @@ -195,7 +195,9 @@ atomic. That can be specified with `// +mapType=atomic` or `// `"x-kubernetes-map-type": "atomic"`. #### Kubectl + ##### Server-side Apply + Since server-side apply is currently in the Alpha phase, it is not enabled by default on kubectl. To use server-side apply on servers with the feature, run the command @@ -212,6 +214,91 @@ Kubernetes clusters. The semantical differences between server-side apply and client-side apply will make a smooth roll-out difficult, so the best way to achieve this has not been decided yet. +#### Status Wiping + +##### Current Behavior + +Right before being persisted to etcd, resources in the apiserver undergo a preparation mechanism that is custom for every resource kind. +It takes care of things like incrementing object generation and status wiping. +This happens through [PrepareForUpdate](https://github.com/kubernetes/kubernetes/blob/bc1360ab158d524c5a7132c8dd9dc7f7e8889af1/staging/src/k8s.io/apiserver/pkg/registry/rest/update.go#L49) and [PrepareForCreate](https://github.com/kubernetes/kubernetes/blob/bc1360ab158d524c5a7132c8dd9dc7f7e8889af1/staging/src/k8s.io/apiserver/pkg/registry/rest/create_update.go#L37). + +The problem status wiping at this level creates is, that when a user applies a field that gets wiped later on, it gets owned by said user. +The apply mechanism (FieldManager) can not know which fields get wiped for which resource and therefor can not ignore those. + +Additionally ignoring status as a whole is not enough, as it should be possible to own status (and other fields) in some occasions. More conversation on this can be found in the [GitHub issue](https://github.com/kubernetes/kubernetes/issues/75564) where the problem got reported. + +##### Proposed Change + +Add an interface that resource strategies can implement, to provide field sets affected by status wiping. + +```go +# staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go +// ResetFieldsProvider is an optional interface that a strategy can implement +// to expose a set of fields that get reset before persisting the object. +type ResetFieldsProvider interface { + // ResetFieldsFor returns a set of fields for the provided version that get reset before persisting the object. + // If no fieldset is defined for a version, nil is returned. + ResetFieldsFor(version string) *fieldpath.Set +} +``` + +Additionally, this interface is implemented by `registry.Store` which forwards it to the corresponding strategy (if applicable). +If `registry.Store` can not provide a field set, it returns nil. + +An example implementation for the interface inside the pod strategy could be: + +```go +# pkg/registry/core/pod/strategy.go +// ResetFieldsFor returns a set of fields for the provided version that get reset before persisting the object. +// If no fieldset is defined for a version, nil is returned. +func (podStrategy) ResetFieldsFor(version string) *fieldpath.Set { + set, ok := resetFieldsByVersion[version] + if !ok { + return nil + } + return set +} + +var resetFieldsByVersion = map[string]*fieldpath.Set{ + "v1": fieldpath.NewSet( + fieldpath.MakePathOrDie("status"), + ), +} +``` + +When creating the handlers in [installer.go](https://github.com/kubernetes/kubernetes/blob/3ff0ed46791a821cb7053c1e25192e1ecd67a6f0/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go) the current `rest.Storage` is checked to implement the `ResetFieldsProvider` interface and the result is passed to the FieldManager. + +```go +# staging/src/k8s.io/apiserver/pkg/endpoints/installer.go +var resetFields *fieldpath.Set +if resetFieldsProvider, isResetFieldsProvider := storage.(rest.ResetFieldsProvider); isResetFieldsProvider { + resetFields = resetFieldsProvider.ResetFieldsFor(a.group.GroupVersion.Version) +} +``` + +When provided with a field set, the FieldManager strips all `resetFields` from incoming update and apply requests. +This causes the user/manager to not own those fields. + +```go +... +if f.resetFields != nil { + patchObjTyped = patchObjTyped.Remove(f.resetFields) +} +... +``` + +##### Alternatives + +We looked at a way to get the fields affected by status wiping without defining them separately. +Mainly by pulling the reset logic from the strategies `PrepareForCreate` and `PrepareForUpdate` methods into a new method `ResetFields` implementing an `ObjectResetter` interface. + +This approach did not work as expected, because the strategy works on internal types while the FieldManager handles external api types. +The conversion between the two and creating the diff was complex and would have caused a notable amount of allocations. + +##### Implementation History + +- 12/2019 [#86083](https://github.com/kubernetes/kubernetes/pull/86083) implementing a poc for the described approach + ### Risks and Mitigations We used a feature branch to ensure that no partial state of this feature would