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.

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

Update alpha field guidance #869

Merged
merged 1 commit into from
Aug 23, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 127 additions & 33 deletions contributors/devel/api_changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -788,8 +788,10 @@ For example, consider the following object:
```go
// API v6.
type Frobber struct {
Height int `json:"height"`
Param string `json:"param"`
// height ...
Height *int32 `json:"height" protobuf:"varint,1,opt,name=height"`
// param ...
Param string `json:"param" protobuf:"bytes,2,opt,name=param"`
}
```

Expand All @@ -798,59 +800,151 @@ A developer is considering adding a new `Width` parameter, like this:
```go
// API v6.
type Frobber struct {
Height int `json:"height"`
Width int `json:"height"`
Param string `json:"param"`
// height ...
Height *int32 `json:"height" protobuf:"varint,1,opt,name=height"`
// param ...
Param string `json:"param" protobuf:"bytes,2,opt,name=param"`
// width ...
Width *int32 `json:"width,omitempty" protobuf:"varint,3,opt,name=width"`
Copy link

Choose a reason for hiding this comment

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

Do you prescribe adding the new field to the end of the Go struct, with the intention of allowing existing "unkeyed" composite literals to remain valid when recompiled, or do you mandate use keyed elements so that existing code can't impose assumptions about field order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you prescribe adding the new field to the end of the Go struct, with the intention of allowing existing "unkeyed" composite literals to remain valid when recompiled

No, unkeyed literal compatibility is impossible when adding new fields.

Adding new fields at the end just makes it easier to see the incrementing protobuf tag.

Copy link

Choose a reason for hiding this comment

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

Ah, yes, a trip to the Go Playground confirmed what I had forgotten. I was thinking that that an unkeyed composite literal could get away with specifying a prefix of the fields, but then this is where that trick of using a field called "_" comes in, defending against callers using unkeyed literals.

}
```

However, the new feature is not stable enough to be used in a stable version
(`v6`). Some reasons for this might include:

- the final representation is undecided (e.g. should it be called `Width` or
`Breadth`?)
- the implementation is not stable enough for general use (e.g. the `Area()`
routine sometimes overflows.)
- the final representation is undecided (e.g. should it be called `Width` or `Breadth`?)
- the implementation is not stable enough for general use (e.g. the `Area()` routine sometimes overflows.)

The developer cannot add the new field until stability is met. However,
The developer cannot add the new field unconditionally until stability is met. However,
sometimes stability cannot be met until some users try the new feature, and some
users are only able or willing to accept a released version of Kubernetes. In
that case, the developer has a few options, both of which require staging work
over several releases.


A preferred option is to first make a release where the new value (`Width` in
this example) is specified via an annotation, like this:

```go
kind: frobber
version: v6
metadata:
name: myfrobber
annotations:
frobbing.alpha.kubernetes.io/width: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this annotation process no longer advocated? @cmluciano !!

Copy link
Member Author

@liggitt liggitt Aug 1, 2017

Choose a reason for hiding this comment

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

it is not advocated. see the links in this PR's description for the problems. the alternatives to the approach described in this PR are:

  • use annotations
    • exposes you to time-bomb data added while running an earlier version of the server
    • no way for an admin to control use
    • if the field graduates to beta/stable it requires breaking clusters using alpha fields or trying to migrate/surface the options in both an annotation and field at the same time (which doesn't work)
  • use fields with "alpha" in the name
    • if the field graduates to beta/stable it requires breaking clusters using alpha fields or trying to migrate/surface the options in two fields at the same time (which doesn't work)
  • use fields without "alpha" in the name and allow changing them in incompatible ways
    • breaks old servers and/or clients if the schema changes on the way to beta/stable

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to mention the old approach (and that it is no longer recommended/allowed for new features), since it will still exist in a number of places for some time to come.

height: 4
param: "green and blue"
```

This format allows users to specify the new field, but makes it clear that they
are using a Alpha feature when they do, since the word `alpha` is in the
annotation key.
#### Alpha field in existing API version

Previously, annotations were used for experimental alpha features, but are no longer recommended for several reasons:

* They expose the cluster to "time-bomb" data added as unstructured annotations against an earlier API server (https://issue.k8s.io/30819)
* They cannot be migrated to first-class fields in the same API version (see the issues with representing a single value in multiple places in [backward compatibility gotchas](#backward-compatibility-gotchas))

The preferred approach adds an alpha field to the existing object, and ensures it is disabled by default:

1. Add a feature gate to the API server to control enablement of the new field (and associated function):

In [k8s.io/apiserver/pkg/util/feature/feature_gate.go](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go):

```go
// owner: @you
// alpha: v1.11
//
// Add multiple dimensions to frobbers.
Frobber2D utilfeature.Feature = "Frobber2D"

var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureSpec{
...
Frobber2D: {Default: false, PreRelease: utilfeature.Alpha},
}
```

2. Add the field to the API type:

* ensure the field is [optional](api-conventions.md#optional-vs-required)
* add the `omitempty` struct tag
* add the `// +optional` comment tag
* ensure the field is entirely absent from API responses when empty (if it is a struct type, it must be a pointer)
* include details about the alpha-level in the field description

```go
// API v6.
type Frobber struct {
// height ...
Height int32 `json:"height" protobuf:"varint,1,opt,name=height"`
// param ...
Param string `json:"param" protobuf:"bytes,2,opt,name=param"`
// width indicates how wide the object is.
// This field is alpha-level and is only honored by servers that enable the Frobber2D feature.
// +optional
Width *int32 `json:"width,omitempty" protobuf:"varint,3,opt,name=width"`
}
```

3. Before persisting the object to storage, clear disabled alpha fields.
One possible place to do this is in the REST storage strategy's PrepareForCreate/PrepareForUpdate methods:

```go
func (frobberStrategy) PrepareForCreate(ctx genericapirequest.Context, obj runtime.Object) {
frobber := obj.(*api.Frobber)

if !utilfeature.DefaultFeatureGate.Enabled(features.Frobber2D) {
frobber.Width = nil
}
}

func (frobberStrategy) PrepareForUpdate(ctx genericapirequest.Context, obj, old runtime.Object) {
newFrobber := obj.(*api.Frobber)
oldFrobber := old.(*api.Frobber)

if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) {
newFrobber.Spec.ConfigSource = nil
oldFrobber.Spec.ConfigSource = nil
}
}
```

4. In validation, ensure the alpha field is not set if the feature gate is disabled:

```go
func ValidateFrobber(f *api.Frobber, fldPath *field.Path) field.ErrorList {
...
if utilfeature.DefaultFeatureGate.Enabled(features.Frobber2D) {
... normal validation of width field ...
} else if f.Width != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("width"), "disabled by feature-gate"))
}
...
}
```

In future versions:

* if the feature progresses to beta or stable status, the feature gate can simply be enabled by default.
* if the schema of the alpha field must change in an incompatible way, a new field name must be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

so, if we're working with a new alpha field Quuxer, and, while still in alpha, we want to change the value type from int to resource.Quantity, we have to abandon the name Quuxer forever? That seems to partially defeat the purpose of alpha...

Copy link
Member Author

@liggitt liggitt Aug 4, 2017

Choose a reason for hiding this comment

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

to be clear, this is talking about changing an alpha field that has already been included in a release.

until we generate servers and clients with alpha fields completely absent from the go structs, changing the schema breaks old clients and servers:

  • an old client with the inert alpha field quxxer int in its go struct would fail to decode a server sending data including the beta field "quxxer":"1m", which breaks forward client compatibility
  • an old server with the inert alpha field quxxer int in its go struct would fail to decode a client sending data including the beta field "quxxer":"1m", which breaks backward client compatibility

Copy link
Member

Choose a reason for hiding this comment

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

This is admittedly a compromise, but the alternative discussed was to always prefix alpha fields with alpha, which would always incur breakage when transitioning out of alpha, as it is not possible to support multiple representations of the same attribute in the same API version.

https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#backward-compatibility-gotchas

* if the feature is abandoned, or a different field name is selected, the field should be removed from the go struct, with a tombstone comment ensuring the field name and protobuf tag are not reused:

```go
// API v6.
type Frobber struct {
// height ...
Height int32 `json:"height" protobuf:"varint,1,opt,name=height"`
// param ...
Param string `json:"param" protobuf:"bytes,2,opt,name=param"`

// removed alpha-level field 'width'
// the field name 'width' and protobuf tag '3' may not be reused.
// Width *int32 `json:"width,omitempty" protobuf:"varint,3,opt,name=width"`
}
```

#### New alpha API version

Another option is to introduce a new type with an new `alpha` or `beta` version
designator, like this:

```
// API v6alpha2
// API v7alpha1
type Frobber struct {
Height int `json:"height"`
Width int `json:"height"`
Param string `json:"param"`
// height ...
Height *int32 `json:"height" protobuf:"varint,1,opt,name=height"`
// param ...
Param string `json:"param" protobuf:"bytes,2,opt,name=param"`
// width ...
Width *int32 `json:"width,omitempty" protobuf:"varint,3,opt,name=width"`
}
```

The latter requires that all objects in the same API group as `Frobber` to be
replicated in the new version, `v6alpha2`. This also requires user to use a new
replicated in the new version, `v7alpha1`. This also requires user to use a new
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this particular example offers a strong amount of guidance.

before it seemed to suggest taking an api 'back to alpha', chronologically, like so:

v6alpha1
v6beta1
v6
v6alpha2

and now what makes more sense to me to suggest a main version bump, again chronologically

v6alpha1
v6beta1
v6
v7alpha1

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, starting v6alpha2 after v6 exists wouldn't make sense.

Copy link
Member

Choose a reason for hiding this comment

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

It was recently pointed out to me that we don't really document this API versioning convention anywhere.

client which uses the other version. Therefore, this is not a preferred option.

A related issue is how a cluster manager can roll back from a new version
Expand Down