Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding #132

pierDipi · 2020-10-22T08:02:31Z

CRD v1beta1 API is deprecated and will be removed in k8s 1.22.
Similar to knative/eventing#3360.
Webhook API bump in a follow-up.

Proposed Changes

Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding

Release Note

- 🎁 Add new feature
KafkaChannel CustomResourceDefinition now uses apiextensions.k8s.io/v1 APIs

/cc slinkydeveloper matzew

codecov · 2020-10-22T08:06:33Z

Codecov Report

Merging #132 (68cffec) into master (e71a1b1) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   75.99%   76.04%   +0.04%     
==========================================
  Files         112      112              
  Lines        4274     4274              
==========================================
+ Hits         3248     3250       +2     
+ Misses        831      829       -2     
  Partials      195      195

Impacted Files	Coverage Δ
...tributed/common/kafka/admin/eventhubcache/cache.go	`67.46% <0.00%> (+2.40%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e71a1b1...aea063b. Read the comment docs.

matzew · 2020-10-22T11:26:07Z

/hold

we need upgrade tests for that

I think @aliok has some reference GH issue for this, what all needs to be done

pierDipi · 2020-10-22T13:22:55Z

@matzew, would you mind helping me understand why?
To clarify, I'm changing the CRD apiVersion (apiextensions.k8s.io/v1), not adding the KafkaChannel v1.

matzew · 2020-10-27T12:44:20Z

If we tackle this - can we also tackle the source ?

Does it work when applying the V1 based version on-top of a v1beta version? and than "rolling back from V1 to beta` ?

I think when I did that for "eventing" repo, the tests there were useful

matzew · 2020-11-06T08:20:37Z

@pierDipi Getting back to this.

We we structure those CRDs, like we have them on "eventing" repo ?

Like:

have the versions after the group (see https://github.com/matzew/eventing/blob/3974f6c67372dbc02c2fb2a0c024390ea5725553/config/channels/in-memory-channel/300-in-memory-channel.yaml#L25-L56)
have names, scope at the end, before the conversion

Can we also have the same lifting for the binding, the source and the distributed channel?

matzew · 2020-11-09T09:43:09Z

Does that make sense @pierDipi ?

aliok · 2020-11-09T10:19:08Z

config/channel/consolidated/resources/kafka-channel.yaml

-        name: kafka-webhook
-        namespace: knative-eventing
+    webhook:
+      conversionReviewVersions: [ "v1", "v1beta1" ]


@pierDipi
Please only change the CRD to use Kube's v1 ApiExtensions.

Bumping the version of messaging.knative.dev to v1 is a separate work.

This refers to the conversion review version.

From: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/

# conversionReviewVersions indicates what ConversionReview versions are understood/preferred by the webhook. # The first version in the list understood by the API server is sent to the webhook. # The webhook must respond with a ConversionReview object in the same version it received.

Ok, so, not related to KafkaChannel's version (messaging.knative.dev)?

Yes, it's unrelated, see https://github.com/knative/eventing/blob/master/config/core/resources/eventtype.yaml, EventType is in v1alpha1 and v1beta1 and the conversionReviewVersions is set to [ "v1", "v1beta1" ]

Isn't that the conversion of the CRD api itself ?

Nope, they are versions of the conversion request and response, see https://github.com/kubernetes/apiextensions-apiserver/blob/1fa7e1a40da83d381ce598c1dc4095debc040c60/pkg/apis/apiextensions/v1/types.go#L434-L476 and https://github.com/kubernetes/apiextensions-apiserver/blob/1fa7e1a40da83d381ce598c1dc4095debc040c60/pkg/apis/apiextensions/v1beta1/types.go#L475-L522

v1alpha1 version has never been created, (first CRDs release branch) https://github.com/kubernetes/apiextensions-apiserver/tree/release-1.7/pkg/apis/apiextensions

pierDipi · 2020-11-09T10:27:17Z

@matzew it makes sense to me.

matzew · 2020-11-20T12:06:31Z

/retest

matzew · 2020-11-21T11:35:29Z

@pierDipi I think this needs a rebase

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

pierDipi · 2020-11-23T15:20:06Z

Flaky, I opened an issue: #218
/retest

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

matzew · 2020-11-23T15:42:31Z

VEry cool!

travis-minke-sap · 2020-11-23T16:00:57Z

Seem fine to me! I had the same questions regarding conversionReviewVersions and so learned something from the conversation above ; )

matzew · 2020-11-23T16:03:06Z

/lgtm

/assign for @aliok to approve and unhold

aliok · 2020-11-25T08:34:36Z

/lgtm
/approve

knative-prow-robot · 2020-11-25T08:34:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aliok]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pierDipi · 2020-11-25T08:36:19Z

/unhold

…xtensions#132) * Channel upgrade tests structure and smoke test * Code reformat * Executing shell scripts to install eventing core * Eventing Core installs well * Fix add_trap function * Install latest release and HEAD of consolidated channel * Install channel from release works * Format code * Default a KafkaChannel in upgrade tests * Continual tests * Update configmap instead of patch * Removal of strict mode from e2e-common as it fails current code :-( * Reformatting * Invoke upgrade tests while testing consolidated * Proper invocation od e2e-upgrade-tests * Move strict mode after souring the library * Move strict mode lower * Reference the eventing-core wathola test images * Run post-scripts * Move strict mode lower * Remove strict mode from scripts :-/ * Raise unavailable period to report to 40 sec * Shorter name of main test method * Commenting out run_postinstall_jobs to pass mt-source job See reported issue: knative-extensions#495 * Removing temporal invocation of upgrade tests

knative-prow-robot requested review from matzew and slinkydeveloper October 22, 2020 08:02

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 22, 2020

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 22, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 22, 2020

pierDipi changed the title ~~Bump KafkaChannel CRD to v1~~ Use CRD v1 API for KafkaChannel Oct 22, 2020

aliok reviewed Nov 9, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2020

pierDipi added 4 commits November 23, 2020 15:54

Use CRD v1 API for KafkaChannel

54ad320

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Use CRD v1 API for KafkaBinding

811e463

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Use CRD v1 API for KafkaSource

762e698

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Clean up preserveUnknownFields

a7c1078

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

pierDipi force-pushed the bump-channel-crd branch from d6fe2b2 to a7c1078 Compare November 23, 2020 15:09

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 23, 2020

Lint

aea063b

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

pierDipi changed the title ~~Use CRD v1 API for KafkaChannel~~ Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding Nov 23, 2020

knative-prow-robot assigned matzew Nov 23, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020

knative-prow-robot assigned aliok Nov 25, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2020

knative-prow-robot merged commit 700349f into knative-extensions:master Nov 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding #132

Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding #132

pierDipi commented Oct 22, 2020 •

edited

codecov bot commented Oct 22, 2020 •

edited

matzew commented Oct 22, 2020

pierDipi commented Oct 22, 2020

matzew commented Oct 27, 2020

matzew commented Nov 6, 2020

matzew commented Nov 9, 2020

aliok Nov 9, 2020

pierDipi Nov 9, 2020 •

edited

pierDipi Nov 9, 2020

aliok Nov 9, 2020

pierDipi Nov 9, 2020

aliok Nov 9, 2020

matzew Nov 9, 2020

pierDipi Nov 9, 2020

pierDipi commented Nov 9, 2020

matzew commented Nov 20, 2020

matzew commented Nov 21, 2020

pierDipi commented Nov 23, 2020

matzew commented Nov 23, 2020

travis-minke-sap commented Nov 23, 2020

matzew commented Nov 23, 2020

aliok commented Nov 25, 2020

knative-prow-robot commented Nov 25, 2020

pierDipi commented Nov 25, 2020

Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding #132

Use CRD v1 API for KafkaChannel, KafkaSource and KafkaBinding #132

Conversation

pierDipi commented Oct 22, 2020 • edited

Proposed Changes

codecov bot commented Oct 22, 2020 • edited

Codecov Report

matzew commented Oct 22, 2020

pierDipi commented Oct 22, 2020

matzew commented Oct 27, 2020

matzew commented Nov 6, 2020

matzew commented Nov 9, 2020

aliok Nov 9, 2020

Choose a reason for hiding this comment

pierDipi Nov 9, 2020 • edited

Choose a reason for hiding this comment

pierDipi Nov 9, 2020

Choose a reason for hiding this comment

aliok Nov 9, 2020

Choose a reason for hiding this comment

pierDipi Nov 9, 2020

Choose a reason for hiding this comment

aliok Nov 9, 2020

Choose a reason for hiding this comment

matzew Nov 9, 2020

Choose a reason for hiding this comment

pierDipi Nov 9, 2020

Choose a reason for hiding this comment

pierDipi commented Nov 9, 2020

matzew commented Nov 20, 2020

matzew commented Nov 21, 2020

pierDipi commented Nov 23, 2020

matzew commented Nov 23, 2020

travis-minke-sap commented Nov 23, 2020

matzew commented Nov 23, 2020

aliok commented Nov 25, 2020

knative-prow-robot commented Nov 25, 2020

pierDipi commented Nov 25, 2020

pierDipi commented Oct 22, 2020 •

edited

codecov bot commented Oct 22, 2020 •

edited

pierDipi Nov 9, 2020 •

edited