Bump CRD versions to v1beta1 #1782

thbkrkr · 2019-09-24T09:44:29Z

This commit bumps the version of the CRDS from version v1alpha1 to version v1betav1:

Copy pkg/apis/*/v1alpha1 in pkg/apis/*/v1beta1. Only differences exists for the ElasticsearchStatus (some fields have been removed since the 0.9)
all occurrences of v1alpha1 are replaced by v1betav1 in pkg/, config/, test/ and docs/*.asciidoc
Add JSON struct tag fields to make the controller-gen happy
Add kubebuilder annotations
Restore the controller-gen target to generate the CRDs (with trivialVersions=true for now, CRD multi-versions support with schema changes #1823)
Add a patch-crd go script to remove the podTemplate schema (CRD podTemplate schema forces to declare an empty spec.containers #1822)
Add the apm shortname for the APM Server

Resolves #1140.

charith-elastic · 2019-09-24T10:31:43Z

If we don't retain the v1alpha1 definitions, would it be possible to deploy the new version on top of an existing cluster without breaking anything? I can't seem to be able to apply the new CRDs without deleting the old ones first (The CustomResourceDefinition "kibanas.kibana.k8s.elastic.co" is invalid: spec.version: Invalid value: "v1beta1": must match the first version in spec.versions). Unless I have made a silly mistake, I assume that this means that people can't upgrade without some down time?

Also, should we consider implementing conversion to support a smoother upgrade flow?

pebrc · 2019-09-24T11:46:09Z

I think conversion would require a webhook server which is currently removed in master due to #1723 But it is definitely something we would like to do once it is restored.

As to having multiple versions of the API:
I think we are still using the deprecated version attribute and we should be using versions instead which lists the different versions and indicates via served whether users can submit new objects using a particular version.

Can we try that and see if keeping v1alpha1 in the versions (but served: false) smoothens the upgrade process?

thbkrkr · 2019-09-24T12:38:14Z

If we don't retain the v1alpha1 definitions, would it be possible to deploy the new version on top of an existing cluster without breaking anything?

No.

I assume that this means that people can't upgrade without some down time?

Yes.

In the current state it will be a breaking change that implies a downtime.

Can we try that and see if keeping v1alpha1 in the versions (but served: false) smoothens the upgrade process?

Yes, I'm going to try this to avoid the breaking.

thbkrkr · 2019-09-25T13:31:02Z

As the schema has changed between v1alpha1 and v1beta1, I don't think we can have both versions referenced in the latest CRDs without implementing conversion.

I tried with:

  versions:
  - name: v1beta1
    served: true
    storage: true
  - name: v1alpha1
    served: false
    storage: false

And this workflow:

deploy the operator 0.9
deploy a test ES resource in v1alpha1
apply the new CRDs with versions v1beta1 and v1alpha1
update the operator to the latest version

Then the operator is failing with the following errors:

2019-09-25T14:50:26.314+0200 ERROR controller-runtime.eventhandler.EnqueueRequestForOwner Could not retrieve rest mapping {"ver": "0.10.0-SNAPSHOT-263b5431", "kind": "Elasticsearch.elasticsearch.k8s.elastic.co", "error": "no matches for kind "Elasticsearch" in version "elasticsearch.k8s.elastic.co/v1alpha1""}

2019-09-25T14:50:26.562+0200 ERROR controller-runtime.controller Reconciler error {"ver": "0.10.0-SNAPSHOT-263b5431", "controller": "elasticsearch-controller", "request": "default/test", "error": "Elasticsearch.elasticsearch.k8s.elastic.co "test" is invalid: []: Invalid value: map[string]interface {}{...}: validation failure list:\nspec.nodes.name in
body should match '[a-zA-Z0-9-]+'"}

Even if it's not our goal for now, I tried to update the test ES resource with the new schema (adding the name) and got:

2019-09-25T15:29:08.949+0200 ERROR controller-runtime.controller Reconciler error {"ver": "0.10.0-SNAPSHOT-263b5431", "controller": "elasticsearch-controller", "request": "default/test", "error": "Object default/test-es-http-certs-internal is already owned by another Elasticsearch controller test", "errorCauses": [{"error": "Object default/test-es-http-certs-internal is already owned by another Elasticsearch controller test"}]}

pebrc · 2019-09-25T13:48:51Z

Would it help if instead of renaming we would keep the alpha versions around?

pebrc · 2019-09-25T19:20:03Z

Ok I played around with this a bit here pebrc@76fddfc

and it looks like if we keep the original code around and fix the CRD generation (which was not running and broken) we can have both versions.

But:

if served: true old resources will be reconciled by the new controller
if served: false then controller-runtime.eventhandler.EnqueueRequestForOwner Could not retrieve rest mapping

Also:

kubectl apply -f crds.yaml will fail miserably on update because the last-applied-configuration annoation created by kubectl will exceed the max annotation length
our controller level version check should work once we update to the correct minVersion in the code

thbkrkr · 2019-09-25T21:23:18Z

Well done!

kubectl apply -f crds.yaml will fail miserably on update because the last-applied-configuration annoation created by kubectl will exceed the max annotation length

The description fields can be truncated or removed using crd:maxDescLen=[-1|0|n] (kubernetes-sigs/controller-tools#299).

-       $(CONTROLLER_GEN) crd paths="./pkg/apis/..." output:crd:artifacts:config=config/crds
+       $(CONTROLLER_GEN) crd:maxDescLen=100 paths="./pkg/apis/..." output:crd:artifacts:config=config/crds

pebrc · 2019-09-25T22:20:48Z

The description fields can be truncated

I used kubectl replace instead which does not write the annotation

pkg/apis/apm/v1beta1/apmserver_types.go

anyasabo · 2019-10-01T15:12:32Z

pkg/apis/kibana/v1alpha1/kibana_types.go

@@ -102,8 +102,7 @@ func (k *Kibana) SetAssociationConf(assocConf *commonv1alpha1.AssociationConf) {
 	k.assocConf = assocConf
 }

-// +genclient
-// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +kubebuilder:object:root=true


anyasabo · 2019-10-01T15:12:45Z

pkg/apis/kibana/v1alpha1/kibana_types.go

-	assocConf *commonv1alpha1.AssociationConf
+	Spec      KibanaSpec                      `json:"spec,omitempty"`
+	Status    KibanaStatus                    `json:"status,omitempty"`
+	assocConf *commonv1alpha1.AssociationConf `json:"-"` //nolint:govet


I think we prefer to keep this hidden (that was the behaviour in 0.9).

anyasabo · 2019-10-01T15:21:48Z

Just to be clear since the v1beta1 types are essentially just a copy and paste of the v1alpha1 types, and they need to be that way to be compatible between versions: if we make any type changes in the future we need to make sure we change both the v1alpha1 and v1beta1 types, correct? Until we have a conversion webhook that is.

thbkrkr · 2019-10-01T15:29:56Z

if we make any type changes in the future we need to make sure we change both the v1alpha1 and v1beta1 types, correct?

No, if we make any type changes in the future, we need to create a new version (e.g. v1beta2 or v1).

thbkrkr · 2019-10-01T15:35:22Z

Also, we use our controller-version to prevent the latest operator (1.0.0-beta1) to handle previous custom resources (0.9), to overcome the fact that we have no conversion webhook.

anyasabo · 2019-10-01T15:41:53Z

pkg/controller/common/annotation/controller_version.go

-const ControllerVersionAnnotation = "common.k8s.elastic.co/controller-version"
+const (
+	ControllerVersionAnnotation       = "common.k8s.elastic.co/controller-version"
+	MinCompatibleControllerVersion    = "0.10.0-SNAPSHOT"


These names are confusing to me at least, just going by the names it's not clear why we would need both the last incompatible version and the minimum compatible version. Going by how they're used I think it's because we're using "last incompatible" to mean the version before we started adding the annotation, but it might be worth finding more descriptive names and/or adding explanatory comments on why these exist

I think I'm fine with MinCompatibleControllerVersion .

How about we change LastIncompatibleControllerVersion to:

UnknownControllerVersion = "UNKNOWN" // may match resources created with ECK-0.8.0

Will that work? I think it needs to be a parseable version for later versions of the controller to know they shouldn't do anything with it when it does a version comparison, but definitely possible I'm misunderstanding

oh, indeed! Then maybe:

UnknownControllerVersion = "0.0.0-UNKNOWN" // may match resources created with ECK-0.8.0

pkg/controller/common/annotation/controller_version.go

hack/patch-crd/main.go

sebgl

LGTM
I tested the alpha -> beta migration locally, everything seems to behave as expected.

anyasabo

Nice work!

A note to other reviewers: make sure you are on controller-gen 0.2.1, 0.2.0 has some surprising behavior

anyasabo · 2019-10-01T19:52:24Z

This should also close out #1770

thbkrkr · 2019-10-01T19:53:41Z

Glad we are finally here. Thank you for your feedbacks.

ximenzaoshi · 2019-10-12T07:44:17Z

Object default/test-es-http-certs-internal is already owned by another Elasticsearch controller test
I still don't know how to solve this error. The old resource's owner ref belongs to v1alpha1, but the new operator use v1beta1. I think version conversion cannot fix this. Is there any way to use new controllers to manage old version resources?

Bump version v1betav1

263b543

thbkrkr added >enhancement Enhancement of existing functionality v1.0.0-beta1 >breaking labels Sep 24, 2019

thbkrkr added 3 commits September 25, 2019 16:09

Merge remote-tracking branch 'upstream/master' into bump-version-v1beta1

37678bc

Keep v1alpha1 apis

110d8c5

Missing refactoring

d624fa4

thbkrkr added 11 commits September 25, 2019 21:40

Add command to generate CRDs

2ddf7f2

Update kubebuilder annotations

1f3bd01

Fix JSON struct field tags

b629d1a

Regenerate CRDs

f7db6b6

make format

715a1b2

Merge remote-tracking branch 'upstream/master' into bump-version-v1beta1

9b4c77f

Regenerate CRDs

414ff96

Fix kibana annotation

a05eedb

Limit the description length for fields in OpenAPI Schema

50f5d51

Regenerate CRDs

cf5fb33

Missing merge

421ecc4

pebrc mentioned this pull request Sep 26, 2019

Move CRDs to v1beta1 for 1.0.0-beta1 #1140

Closed

thbkrkr added 3 commits September 26, 2019 14:48

Update last compatible controller version

bdf482c

Merge remote-tracking branch 'upstream/master' into bump-version-v1beta1

087c278

Regenerate CRDs with crd:maxDescLen=100

e2f01f3

thbkrkr added 2 commits October 1, 2019 17:00

Sync patch-crd module name

86c1ad5

Review's input

3ea3e64

sebgl reviewed Oct 1, 2019

View reviewed changes

pkg/apis/apm/v1beta1/apmserver_types.go Outdated Show resolved Hide resolved

anyasabo reviewed Oct 1, 2019

View reviewed changes

sebgl mentioned this pull request Oct 1, 2019

Change current version to 1.0.0-beta1 #1833

Closed

anyasabo reviewed Oct 1, 2019

View reviewed changes

sebgl reviewed Oct 1, 2019

View reviewed changes

pkg/controller/common/annotation/controller_version.go Outdated Show resolved Hide resolved

thbkrkr added 4 commits October 1, 2019 17:45

Comment why hack/patch-crd/main.go

1e67eae

Fix categories kubebuilder annotation

5fb32b9

Improve naming and add comments

a0b17d7

Set UnknownControllerVersion to 0.0.0-UNKNOWN

bfe7b16

sebgl reviewed Oct 1, 2019

View reviewed changes

hack/patch-crd/main.go Outdated Show resolved Hide resolved

sebgl approved these changes Oct 1, 2019

View reviewed changes

thbkrkr added 2 commits October 1, 2019 20:55

go fmt and fix typo

443759c

Update common/annotation/controller_version_test.go

8afc88d

pebrc approved these changes Oct 1, 2019

View reviewed changes

anyasabo approved these changes Oct 1, 2019

View reviewed changes

thbkrkr merged commit 25caaf1 into elastic:master Oct 1, 2019

thbkrkr deleted the bump-version-v1beta1 branch October 4, 2019 20:46

pebrc changed the title ~~Bump version v1betav1~~ Bump CRD versions to v1betav1 Oct 9, 2019

pebrc removed the >enhancement Enhancement of existing functionality label Oct 9, 2019

thbkrkr changed the title ~~Bump CRD versions to v1betav1~~ Bump CRD versions to v1beta1 Oct 11, 2019

thbkrkr mentioned this pull request Oct 11, 2019

Document secure settings breaking change #1983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump CRD versions to v1beta1 #1782

Bump CRD versions to v1beta1 #1782

thbkrkr commented Sep 24, 2019 •

edited

charith-elastic commented Sep 24, 2019

pebrc commented Sep 24, 2019

thbkrkr commented Sep 24, 2019

thbkrkr commented Sep 25, 2019 •

edited

pebrc commented Sep 25, 2019

pebrc commented Sep 25, 2019 •

edited

thbkrkr commented Sep 25, 2019

pebrc commented Sep 25, 2019 •

edited

anyasabo Oct 1, 2019

anyasabo Oct 1, 2019

thbkrkr Oct 1, 2019 •

edited

anyasabo commented Oct 1, 2019

thbkrkr commented Oct 1, 2019

thbkrkr commented Oct 1, 2019

anyasabo Oct 1, 2019

sebgl Oct 1, 2019

anyasabo Oct 1, 2019

sebgl Oct 1, 2019

sebgl left a comment

anyasabo left a comment

anyasabo commented Oct 1, 2019

thbkrkr commented Oct 1, 2019

ximenzaoshi commented Oct 12, 2019 •

edited

Bump CRD versions to v1beta1 #1782

Bump CRD versions to v1beta1 #1782

Conversation

thbkrkr commented Sep 24, 2019 • edited

charith-elastic commented Sep 24, 2019

pebrc commented Sep 24, 2019

thbkrkr commented Sep 24, 2019

thbkrkr commented Sep 25, 2019 • edited

pebrc commented Sep 25, 2019

pebrc commented Sep 25, 2019 • edited

thbkrkr commented Sep 25, 2019

pebrc commented Sep 25, 2019 • edited

anyasabo Oct 1, 2019

Choose a reason for hiding this comment

anyasabo Oct 1, 2019

Choose a reason for hiding this comment

thbkrkr Oct 1, 2019 • edited

Choose a reason for hiding this comment

anyasabo commented Oct 1, 2019

thbkrkr commented Oct 1, 2019

thbkrkr commented Oct 1, 2019

anyasabo Oct 1, 2019

Choose a reason for hiding this comment

sebgl Oct 1, 2019

Choose a reason for hiding this comment

anyasabo Oct 1, 2019

Choose a reason for hiding this comment

sebgl Oct 1, 2019

Choose a reason for hiding this comment

sebgl left a comment

Choose a reason for hiding this comment

anyasabo left a comment

Choose a reason for hiding this comment

anyasabo commented Oct 1, 2019

thbkrkr commented Oct 1, 2019

ximenzaoshi commented Oct 12, 2019 • edited

thbkrkr commented Sep 24, 2019 •

edited

thbkrkr commented Sep 25, 2019 •

edited

pebrc commented Sep 25, 2019 •

edited

pebrc commented Sep 25, 2019 •

edited

thbkrkr Oct 1, 2019 •

edited

ximenzaoshi commented Oct 12, 2019 •

edited