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

Create CRDs V1 #2184

Merged
merged 16 commits into from
Dec 9, 2019
Merged

Create CRDs V1 #2184

merged 16 commits into from
Dec 9, 2019

Conversation

sebgl
Copy link
Contributor

@sebgl sebgl commented Nov 28, 2019

Bump the current v1beta1 CRD to a separate v1 CRD with the exact same content. As such, it's fine to rely on the OpenAPI validation schema of the v1 version to validate the v1beta1 resources.

I tried to make sure every single reference we had to the v1beta1 API is updated to use the v1 API now. Along with that change I tried to homogenize the imports so we always import esv1, kbv1, etc. instead of using a mix of estype, esv1beta1, elasticsearchv1beta1, v1beta1, etc.

Commit messages show high-level changes.

I made sure the webhook for v1 works fine with the all-in-one version, but I have some doubts on:

  • the v1beta1 webhook (alongside the v1 webhook)
  • webhooks for the global + namespace operators
    I'll investigate and maybe fix them in a follow-up PR.

Fixes #2117.

thbkrkr
thbkrkr previously approved these changes Nov 28, 2019
Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

I went relatively quickly on all the files, played a little with grep -r, generated and applied all-crds.yml and everything seems ok.

I've the same concerns about the webhook but let's merge this PR and fix them if needed after to avoid too many "conflicts fix".

LGTM!

@sebgl
Copy link
Contributor Author

sebgl commented Nov 28, 2019

API docs generation does not produce a consistent order, which messes up with CI:

Error: dirty local changes
12:37:05   M docs/api-docs.asciidoc

I'll see what I can do here.

@charith-elastic
Copy link
Contributor

API docs generation does not produce a consistent order

That's odd. It should produce output sorted by the API name and version. Let me know if you need any help.

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

I did the following:

  • Apply all-in-one with v1beta1
  • Deploy an Elasticsearch instance
  • Apply all-in-one with v1

I have the following error:

"error":"Object default/elasticsearch-sample-es-http-certs-internal is already owned by another Elasticsearch controller elasticsearch-sample"

Is it related to the one you got yesterday ?

config/operator/global/webhook.template.yaml Show resolved Hide resolved
@sebgl
Copy link
Contributor Author

sebgl commented Nov 28, 2019

elastic/gen-crd-api-reference-docs#1 should fix the inconsistent api docs rendering.

@sebgl
Copy link
Contributor Author

sebgl commented Nov 28, 2019

@barkbay I get the same error you get, probably because the operator with CRD v1 is trying to add elasticsearch/v1 as ownerReference to the secret that already has elasticsearch/v1beta1 as ownerReference 😕

The corresponding code in controller-runtime does a strict comparison between both resource versions:

for i, r := range existingRefs {
		if referSameObject(ref, r) {
			fi = i
		} else if r.Controller != nil && *r.Controller {
			return newAlreadyOwnedError(object, r)
		}
	}
func referSameObject(a, b metav1.OwnerReference) bool {
	aGV, err := schema.ParseGroupVersion(a.APIVersion)
	if err != nil {
		return false
	}

	bGV, err := schema.ParseGroupVersion(b.APIVersion)
	if err != nil {
		return false
	}

	return aGV == bGV && a.Kind == b.Kind && a.Name == b.Name
}

I created an issue in the controller-runtime repository: kubernetes-sigs/controller-runtime#699.
And an issue in our own: #2191

@sebgl
Copy link
Contributor Author

sebgl commented Nov 29, 2019

I found another blocker for this: #2192

@sebgl
Copy link
Contributor Author

sebgl commented Dec 4, 2019

#2211 should fix the owner reference problem.

@sebgl sebgl changed the title Create CRDs V1 (not ready to merge) Create CRDs V1 Dec 4, 2019
@anyasabo
Copy link
Contributor

anyasabo commented Dec 4, 2019

I tried to homogenize the imports so we always import esv1, kbv1, etc. instead of using a mix of estype, esv1beta1, elasticsearchv1beta1, v1beta1, etc.

👏👏👏 should make refactors easier in the future

@sebgl
Copy link
Contributor Author

sebgl commented Dec 5, 2019

#2217 should fix the StatefulSet volumeClaimTemplates problem.

@sebgl
Copy link
Contributor Author

sebgl commented Dec 6, 2019

The PR is ready for another round of reviews.
I did some manual testing following the procedure outlined in #2218 (comment).
Everything seems alright as far as I can tell.

The only thing I'm not sure about is the webhook. I created a follow-up issue so it does not delay this PR.

@pebrc
Copy link
Collaborator

pebrc commented Dec 6, 2019

Maybe I am doing something silly here, that I am not aware of, but when I run the operator based on this PR I see errors on my existing v1beta1 resources.

2019-12-06T11:18:47.656+0100	ERROR	controller-runtime.controller	Reconciler error	{"ver": "1.0.0-beta1-620892a3", "controller": "elasticsearch-controller", "request": "default/es-apm-sample", "error": "Elasticsearch.elasticsearch.k8s.elastic.co \"es-apm-sample\" is invalid: apiVersion: Invalid value: \"elasticsearch.k8s.elastic.co/v1beta1\": must be elasticsearch.k8s.elastic.co/v1", "errorCauses": [{"error": "Elasticsearch.elasticsearch.k8s.elastic.co \"es-apm-sample\" is invalid: apiVersion: Invalid value: \"elasticsearch.k8s.elastic.co/v1beta1\": must be elasticsearch.k8s.elastic.co/v1"}]}
github.com/go-logr/zapr.(*zapLogger).Error

Update: so it looks like it is failing on the status subresource update (we are sending v1 I checked, but presumable the subresource update tries to patch only the status bit and we still have v1beta1 stored on the server)

@sebgl
Copy link
Contributor Author

sebgl commented Dec 6, 2019

@pebrc hm right I did not retest APM. Will look into it, thanks for testing!

@sebgl
Copy link
Contributor Author

sebgl commented Dec 6, 2019

We discussed the bug offline with @pebrc:

  • it happens when Status subresources are updated on a resource whose storedVersion is v1beta1
  • since the migration from v1beta1 to v1 includes a rewrite of the resource to remove finalizers, I could not reproduce it easily. But @pebrc had resources with finalizers removed already, hence could reproduce it.
  • enabling debug logs at the client-go level seems to indicate everything is fine on our side, but there's a bug on the apiserver
  • spotted an issue that seems to exactly match this: TestValidateOnlyStatus flakes with apiVersion: Invalid value kubernetes/kubernetes#72493
  • it seems resolved in k8s 1.15
  • we'll probably need to stop updating the subresource API and update the entire resource directly to avoid the error on pre-1.15 clusters

I'm extracting this to its own issue.

@sebgl
Copy link
Contributor Author

sebgl commented Dec 9, 2019

The status subresource validation error is now fixed with #2229.

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM! (I did not retest but assume we are good now)

@sebgl
Copy link
Contributor Author

sebgl commented Dec 9, 2019

Jenkins test this please.

@sebgl sebgl merged commit 459d005 into elastic:master Dec 9, 2019
@sebgl sebgl changed the title (not ready to merge) Create CRDs V1 Create CRDs V1 Dec 10, 2019
mjmbischoff pushed a commit to mjmbischoff/cloud-on-k8s that referenced this pull request Jan 13, 2020
Bump the current v1beta1 CRD to a separate v1 CRD with the exact same content. As such, it's fine to rely on the OpenAPI validation schema of the v1 version to validate the v1beta1 resources.

I tried to make sure every single reference we had to the v1beta1 API is updated to use the v1 API now. Along with that change I tried to homogenize the imports so we always import esv1, kbv1, etc. instead of using a mix of estype, esv1beta1, elasticsearchv1beta1, v1beta1, etc.

* Add v1 API codebase

* Unset storage version for v1beta1 resources

* Fix Elasticsearch webhook path

* Regenerate CRDs with v1 version

* Regenerate webhook manifests with v1 webhook

* Use v1 APIs in the entire codebase & homogenize imports

* Adapt golangci exceptions to v1 apis

* Use v1 CRD in samples

* Regenerate API docs to cover v1 crds

* Default to using crd v1 in the documentation

* Fix typo: corev1

* Regenerate api docs with the fixed version with sorting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move CRD API versions to v1 for 1.0.0
6 participants