[WIP]: Migrate channel reconciler to use messaging.* v1 resources #3789

nlopezgi · 2020-08-05T19:14:11Z

Part of #3584

Proposed Changes

Migrate sequence reconciler to use messaging.* v1 resources

fyi @matzew
This is a WIP PR as tests are currently failing and I have not been able to figure out how to fix them.

knative-prow-robot · 2020-08-05T19:14:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nlopezgi
To complete the pull request process, please assign tcnghia
You can assign the PR to them by writing /assign @tcnghia in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

nlopezgi · 2020-08-05T19:19:58Z

If anyone has any pointers to help here. I'm stuck in the migration for channel (this PR) and subscription with the same error.
The error I get is:

--- FAIL: TestReconcile/Updating_v1alpha1_channelable_subscribers_statuses (0.00s)
panic: Unrecognized type *v1alpha1.Channelable [recovered]
	panic: Unrecognized type *v1alpha1.Channelable

goroutine 478 [running]:
testing.tRunner.func1.1(0x1ad31c0, 0xc000940cd0)
	/usr/lib/google-golang/src/testing/testing.go:1057 +0x30d
testing.tRunner.func1(0xc000a7ad80)
	/usr/lib/google-golang/src/testing/testing.go:1060 +0x41a
panic(0x1ad31c0, 0xc000940cd0)
	/usr/lib/google-golang/src/runtime/panic.go:969 +0x175
knative.dev/pkg/reconciler/testing.(*ObjectSorter).AddObjects(0xc0004ee958, 0xc0006ad380, 0x2, 0x2)
	/usr/local/google/home/ngiraldo/go/src/knative.dev/eventing/vendor/knative.dev/pkg/reconciler/testing/sorter.go:50 +0x1ff
knative.dev/eventing/pkg/reconciler/testing/v1.NewListers(0xc0006ad380, 0x2, 0x2, 0x19094fa)
	/usr/local/google/home/ngiraldo/go/src/knative.dev/eventing/pkg/reconciler/testing/v1/listers.go:91 +0xc5
knative.dev/eventing/pkg/reconciler/testing/v1.MakeFactory.func1(0xc000a7ad80, 0xc00027b080, 0x1d8cc80, 0xc000ca84e0, 0xc000c84360, 0x3, 0x3, 0x1)
	/usr/local/google/home/ngiraldo/go/src/knative.dev/eventing/pkg/reconciler/testing/v1/factory.go:58 +0x8e
knative.dev/pkg/reconciler/testing.(*TableRow).Test(0xc00027b080, 0xc000a7ad80, 0xc0006ad480)
	/usr/local/google/home/ngiraldo/go/src/knative.dev/eventing/vendor/knative.dev/pkg/reconciler/testing/table.go:141 +0x82
knative.dev/pkg/reconciler/testing.TableTest.Test.func1(0xc000a7ad80)
	/usr/local/google/home/ngiraldo/go/src/knative.dev/eventing/vendor/knative.dev/pkg/reconciler/testing/table.go:401 +0x70
testing.tRunner(0xc000a7ad80, 0xc0009bf9e0)
	/usr/lib/google-golang/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
	/usr/lib/google-golang/src/testing/testing.go:1159 +0x386
exit status 2

This error only occurs for the test case that includes dealing with a v1alpha1 Channelable resource. I'm not even sure if this test case is relevant.

I tried to make this test work by creating a v1.Channelable, however, this also fails as lots of the conditions are not met - seems the behavior for this test depends on it using a v1alpha1 Channelable.

The underlying issue I have, I think, is that I'm not sure how to properly migrate testing/v1beta1/channelable.go. This file seems to create a v1alpha1 Channelable which depends on using v1beta1 resources (Addressable and SubscriberStatus), as the v1alpha1 Channelable has v1beta1 resources.
Any advice is appreciated!

matzew · 2020-08-06T12:00:44Z

This error only occurs for the test case that includes dealing with a v1alpha1 Channelable resource. I'm not even sure if this test case is relevant.

wanna skip this testcase, @nlopezgi ?

nlopezgi · 2020-08-06T20:10:30Z

So I looked a bit more at the types code to try to understand what is happening. I think the core issue is related to the ChannelableCombined. This type is supposed to represent "a skeleton type wrapping Subscribable and Addressable of both v1alpha1 and v1beta1 duck types". This leads me to think that as part of the types migration we need to add something else there for the Subscribable and Addressable for v1, otherwise we might end up breaking in some form backward compatibility (but again, I know little of how these things evolve in practice, so I might be missing something)

thoughts @matzew ?

aliok · 2020-08-07T07:18:58Z

@nlopezgi

". This leads me to think that as part of the types migration we need to add something else there for the Subscribable and Addressable for v1,

Yes, that's what I thought too. BTW, some chat around this: https://knative.slack.com/archives/C9JP909F0/p1595856750119700

nlopezgi · 2020-08-07T13:47:23Z

@nlopezgi

". This leads me to think that as part of the types migration we need to add something else there for the Subscribable and Addressable for v1,

Yes, that's what I thought too. BTW, some chat around this: https://knative.slack.com/archives/C9JP909F0/p1595856750119700

Thanks for confirming. I'll prepare a PR adding that.

matzew · 2020-08-11T13:02:46Z

@nlopezgi can you rebase your PR her e?

nlopezgi · 2020-08-11T13:06:11Z

@nlopezgi can you rebase your PR her e?

Thanks, I'm working on the PR. I rebased locally but still running into some issues, hope to make some progress later today.

lgtm-com · 2020-08-11T15:19:12Z

This pull request introduces 4 alerts when merging 7bc321e into f132f49 - view on LGTM.com

new alerts:

4 for Identical operands

lgtm-com · 2020-08-11T17:03:59Z

This pull request introduces 4 alerts when merging 506f7a9 into 951a8d0 - view on LGTM.com

new alerts:

4 for Identical operands

knative-metrics-robot · 2020-08-11T18:13:34Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/duck/v1alpha1/channelable_combined_types.go	93.3%	92.9%	-0.5

lgtm-com · 2020-08-11T18:17:33Z

This pull request introduces 4 alerts when merging 606e5a1 into 951a8d0 - view on LGTM.com

new alerts:

4 for Identical operands

lgtm-com · 2020-08-11T18:47:51Z

This pull request introduces 4 alerts when merging 857e611 into e8b5057 - view on LGTM.com

new alerts:

4 for Identical operands

lgtm-com · 2020-08-11T18:59:26Z

This pull request introduces 4 alerts when merging 8e80582 into e8b5057 - view on LGTM.com

new alerts:

4 for Identical operands

nlopezgi · 2020-08-11T20:01:35Z

@nlopezgi can you rebase your PR her e?

I've rebased and tried everything I can think of but I'm still seeing tests fail with the same error as before (#3789 (comment)).
I have no idea how to continue debugging this error. Some details:

I could not find any threads anywhere with an error similar to panic: Unrecognized type. My only hypothesis is related to the error message: that I'm not registering a type properly, but cannot find how.
Nothing I do on the failing test case (e.g., change init calls) seems to have any impact, I even tried re-creating the pr from scratch, but just switching from v1beta1 to v1 causes the issue.
I cant even find a way to repro the failure with the existing test. If I change this on the passing test (i.e., init on master) the test sill passes.
any advice is appreciated.

aliok · 2020-08-18T13:25:57Z

I've rebased and tried everything I can think of but I'm still seeing tests fail with the same error as before (#3789 (comment)).

@nlopezgi
I gave this a try.
I was able to reproduce the problem locally.

I didn't also figure out how to fix this problem.

Reconciler uses a v1 lister
v1 lister cache doesn't contain the v1alpha1.Channelable type

Not sure if this is a valid test.

aliok · 2020-08-18T13:30:04Z

I cant even find a way to repro the failure with the existing test. If I change this on the passing test (i.e., init on master) the test sill passes.

@nlopezgi
Following works for me:

$ go test knative.dev/eventing/pkg/reconciler/channel/

...
...
--- FAIL: TestReconcile (0.77s)
    --- FAIL: TestReconcile/Updating_v1alpha1_channelable_subscribers_statuses (0.01s)
panic: Unrecognized type *v1alpha1.Channelable [recovered]
        panic: Unrecognized type *v1alpha1.Channelable

goroutine 521 [running]:
testing.tRunner.func1.1(0x1924280, 0xc00077cd90)
        /usr/lib/golang/src/testing/testing.go:940 +0x2f5
...
...

But, I fixed the compile errors first. See the latest 2 commits here: https://github.com/knative/eventing/pull/3860/commits

lgtm-com · 2020-08-18T14:58:30Z

This pull request introduces 4 alerts when merging 8672643 into 9f3fb3f - view on LGTM.com

new alerts:

4 for Identical operands

vaikas · 2020-08-18T15:23:24Z

pkg/apis/duck/v1alpha1/channelable_combined_types.go

@@ -80,9 +80,9 @@ type ChannelableCombinedStatus struct {
 	// SubscribableTypeStatus is the v1alpha1 part of the Subscribers status
 	SubscribableTypeStatus `json:",inline"`
 	// SubscribableStatus is the v1beta1 part of the Subscribers status.
-	eventingduckv1beta1.SubscribableStatus `json:",inline"`
+	SubscribableStatusv1beta1 eventingduckv1beta1.SubscribableStatus `json:",inline"`


Just to make sure I understand. We should be dropping the duck/v1alpha1, since we're dropping the support for v1alpha1 API shapes as per:
https://knative.dev/community/contributing/mechanics/release-versioning-principles/

They were not served in the .16, so we should not be using them here. Are we just leaving the pkg in v1alpha1 as is to make the changes smaller? I guess what I'm asking is, I feel like we should be dropping the duck/v1alpha1/channelable* related things, because v1alpha1 API is no longer supported, so would make more sense to only have a v1beta1 and v1 only. Or, am I missing something (still catching up after vacation :) )

that's good to hear. dropping v1alpha1 would make things easier.

thanks for commenting @vaikas. Dropping v1alpha support will make this simpler. I was not aware that was the plan. I will update this PR (and likely revert some of the code in #3803 as it should not be necessary, and ping you to review once that is ready.
Thanks!

awesome sauce, thanks much for doing this!

one extra thing to keep in mind... if we drop v1alpha1 now, I think eventing-contrib won't be able to update-deps until we move natss out of there or we fix it..

Issue to migrate NATSSChannel to v1 Channelable: knative/eventing-contrib#1485
Issue to move NATSSChannel to a separate repo: knative/eventing-contrib#1487

#3871 is now ready for review. Not sure if it needs to be put on hold until all deps are also updated. Please put on hold if needed. Will be closing this one.

knative-prow-robot · 2020-08-18T15:42:12Z

@nlopezgi: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-unit-tests	`8672643`	link	`/test pull-knative-eventing-unit-tests`
pull-knative-eventing-conformance-tests	`8672643`	link	`/test pull-knative-eventing-conformance-tests`
pull-knative-eventing-upgrade-tests	`8672643`	link	`/test pull-knative-eventing-upgrade-tests`
pull-knative-eventing-integration-tests	`8672643`	link	`/test pull-knative-eventing-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

knative-test-reporter-robot · 2020-08-18T15:42:13Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-conformance-tests		0/3
pull-knative-eventing-integration-tests		0/3
pull-knative-eventing-upgrade-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestDefaultBrokerWithManyTriggers/test_default_broker_with_many_attribute_triggers
test/e2e.TestDefaultBrokerWithManyTriggers/test_default_broker_with_many_attribute_triggers_using_v1beta1_trigger
test/e2e.TestDefaultBrokerWithManyTriggers/test_default_broker_with_many_deprecated_triggers
test/e2e.TestDefaultBrokerWithManyTriggers
test/e2e.TestTriggerDependencyAnnotation
test/e2e.TestBrokerChannelFlowTriggerV1BrokerV1/Channel-messaging.knative.dev/v1beta1
test/e2e.TestBrokerChannelFlowTriggerV1BrokerV1/Channel-messaging.knative.dev/v1
test/e2e.TestBrokerChannelFlowTriggerV1BrokerV1/InMemoryChannel-messaging.knative.dev/v1beta1

and 62 more.

matzew · 2020-08-19T17:12:32Z

lets move it out. asap

On Wed 19. Aug 2020 at 18:30, Ignacio (Nacho) Cano ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/duck/v1alpha1/channelable_combined_types.go <#3789 (comment)>: > @@ -80,9 +80,9 @@ type ChannelableCombinedStatus struct { // SubscribableTypeStatus is the v1alpha1 part of the Subscribers status SubscribableTypeStatus `json:",inline"` // SubscribableStatus is the v1beta1 part of the Subscribers status. - eventingduckv1beta1.SubscribableStatus `json:",inline"` + SubscribableStatusv1beta1 eventingduckv1beta1.SubscribableStatus `json:",inline"` one extra thing to keep in mind... if we drop v1alpha1 now, I think eventing-contrib won't be able to update-deps until we move natss out of there or we fix it.. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3789 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTSSGUANTZPJZ6HVBK3SBP4Z3ANCNFSM4PVZ64SA> .

-- Sent from Gmail Mobile

nlopezgi · 2020-08-20T13:32:13Z

Closing in favor of #3871. See comments above for more details.

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 5, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 5, 2020

knative-prow-robot requested review from lberk and matzew August 5, 2020 19:14

nlopezgi mentioned this pull request Aug 6, 2020

KafkaChannel reconciler to use KafkaChannel v1beta1 API knative/eventing-contrib#1407

Merged

nlopezgi mentioned this pull request Aug 6, 2020

Make reconcilers use messaging.* v1 resources #3584

Closed

nlopezgi mentioned this pull request Aug 7, 2020

Add missing bits to ChannelableCombined and SubscribableTypes for v1 migration #3802

Closed

nlopezgi force-pushed the issue3584-3 branch from 17ef51d to 3945634 Compare August 11, 2020 15:01

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 11, 2020

nlopezgi force-pushed the issue3584-3 branch from 3945634 to 7bc321e Compare August 11, 2020 15:05

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 11, 2020

Nicolas Lopez added 4 commits August 11, 2020 14:06

migrating channel to v1 messaging

1c2581d

channel migration draft

033dfdc

failing with panic: Unrecognized type *v1alpha1.Channelable

e3135b8

also migrate subscription

fdd86cb

update types

606e5a1

nlopezgi force-pushed the issue3584-3 branch from 506f7a9 to 606e5a1 Compare August 11, 2020 18:06

remove changed types

8e80582

nlopezgi force-pushed the issue3584-3 branch from 857e611 to 8e80582 Compare August 11, 2020 18:48

aliok mentioned this pull request Aug 18, 2020

WIP DO NOT MERGE #3860

Closed

merging changes suggested by @aliok

8672643

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Aug 18, 2020

vaikas reviewed Aug 18, 2020

View reviewed changes

nlopezgi mentioned this pull request Aug 18, 2020

Migrate channel reconciler to use messaging.* v1 resources #3871

Merged

nachocano mentioned this pull request Aug 19, 2020

Remove v1alpha1 support for our Resources google/knative-gcp#1544

Closed

aliok mentioned this pull request Aug 20, 2020

Use v1 Channelable in NATSS Channel knative/eventing-contrib#1485

Closed

nlopezgi closed this Aug 20, 2020

nlopezgi mentioned this pull request Aug 25, 2020

Migrate Subscription reconciler to use messaging.* v1 resources #3930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]: Migrate channel reconciler to use messaging.* v1 resources #3789

[WIP]: Migrate channel reconciler to use messaging.* v1 resources #3789

nlopezgi commented Aug 5, 2020

knative-prow-robot commented Aug 5, 2020

nlopezgi commented Aug 5, 2020

matzew commented Aug 6, 2020

nlopezgi commented Aug 6, 2020

aliok commented Aug 7, 2020 •

edited

Loading

nlopezgi commented Aug 7, 2020

matzew commented Aug 11, 2020

nlopezgi commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

knative-metrics-robot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

nlopezgi commented Aug 11, 2020

aliok commented Aug 18, 2020

aliok commented Aug 18, 2020

lgtm-com bot commented Aug 18, 2020

vaikas Aug 18, 2020

aliok Aug 18, 2020

nlopezgi Aug 18, 2020

vaikas Aug 18, 2020

nachocano Aug 19, 2020

aliok Aug 20, 2020

nlopezgi Aug 20, 2020

knative-prow-robot commented Aug 18, 2020

knative-test-reporter-robot commented Aug 18, 2020

matzew commented Aug 19, 2020 via email

nlopezgi commented Aug 20, 2020

[WIP]: Migrate channel reconciler to use messaging.* v1 resources #3789

[WIP]: Migrate channel reconciler to use messaging.* v1 resources #3789

Conversation

nlopezgi commented Aug 5, 2020

Proposed Changes

knative-prow-robot commented Aug 5, 2020

nlopezgi commented Aug 5, 2020

matzew commented Aug 6, 2020

nlopezgi commented Aug 6, 2020

aliok commented Aug 7, 2020 • edited Loading

nlopezgi commented Aug 7, 2020

matzew commented Aug 11, 2020

nlopezgi commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

knative-metrics-robot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

lgtm-com bot commented Aug 11, 2020

nlopezgi commented Aug 11, 2020

aliok commented Aug 18, 2020

aliok commented Aug 18, 2020

lgtm-com bot commented Aug 18, 2020

vaikas Aug 18, 2020

Choose a reason for hiding this comment

aliok Aug 18, 2020

Choose a reason for hiding this comment

nlopezgi Aug 18, 2020

Choose a reason for hiding this comment

vaikas Aug 18, 2020

Choose a reason for hiding this comment

nachocano Aug 19, 2020

Choose a reason for hiding this comment

aliok Aug 20, 2020

Choose a reason for hiding this comment

nlopezgi Aug 20, 2020

Choose a reason for hiding this comment

knative-prow-robot commented Aug 18, 2020

knative-test-reporter-robot commented Aug 18, 2020

matzew commented Aug 19, 2020 via email

nlopezgi commented Aug 20, 2020

aliok commented Aug 7, 2020 •

edited

Loading