GH actions - E2E tests for namespaced broker #2450

aliok · 2022-08-02T06:38:27Z

Fixes #

Proposed Changes

GH actions - E2E tests for namespaced broker
Broker class is still hardcoded to Kafka in Prow e2e tests
Gonna remove the hardcoding in a separate PR when Add new jobs for namespaced broker and adjust existing jobs knative/test-infra#3466 is merged

Release Note

Docs

knative-prow · 2022-08-02T06:38:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok

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

codecov · 2022-08-02T06:45:26Z

Codecov Report

Merging #2450 (49e80b6) into main (2412c54) will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2450      +/-   ##
============================================
+ Coverage     64.59%   64.64%   +0.05%     
- Complexity      708      709       +1     
============================================
  Files           146      146              
  Lines          9803     9803              
  Branches        223      223              
============================================
+ Hits           6332     6337       +5     
+ Misses         3042     3035       -7     
- Partials        429      431       +2

Flag	Coverage Δ
java-unittests	`81.40% <ø> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...broker/dispatcher/impl/consumer/OffsetManager.java	`95.69% <0.00%> (+1.07%)`	⬆️
...dispatcher/impl/consumer/BaseConsumerVerticle.java	`93.54% <0.00%> (+6.45%)`	⬆️
...ive/eventing/kafka/broker/core/AsyncCloseable.java	`80.00% <0.00%> (+13.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

aliok · 2022-08-03T11:02:12Z

/retest

aliok · 2022-08-03T11:40:46Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T06:47:14Z

test/e2e-tests.sh

+# TODO: going to be reverted once we have the Prow changes in https://github.com/knative/test-infra
 export BROKER_CLASS="Kafka"


Still hardcoded in Prow jobs. Gonna pass this once knative/test-infra#3466 is merged.

Suggested change

# TODO: going to be reverted once we have the Prow changes in https://github.com/knative/test-infra

export BROKER_CLASS="Kafka"

# TODO: going to be reverted once we have the Prow changes in https://github.com/knative/test-infra

export BROKER_CLASS=${BROKER_CLASS:-"Kafka"}

what about this option? that would facilitate running them locally without specifying the class since it would default to the existing/stable one.

I personally always prefer explicit errors over invisible defaulting.

However, I am ok with that if you feel strong about it

The job is there, can we fix the TODOs?

TODO: going to be reverted once we have the Prow changes in https://github.com/knative/test-infra

This is a bad comment of mine, sorry.

I want to make sure that I am not breaking any E2E tests for the regular KafkaBroker with this PR.
Similarly, in #2459, I am testing if E2E tests are passing for the namespaced broker.

So, I will remove that TODO in a separate PR when I am 100% sure that E2E tests are passing in both class.

aliok · 2022-08-04T08:17:17Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T08:38:48Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T09:21:00Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T10:00:27Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T11:14:27Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T13:35:15Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T14:06:49Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T18:10:16Z

/test upgrade-tests_eventing-kafka-broker_main

aliok · 2022-08-04T20:18:26Z

/test upgrade-tests_eventing-kafka-broker_main

Trying agin...
I guess there's a real problem, but I don't believe it is related to my changes.

aliok · 2022-08-05T06:45:04Z

/test upgrade-tests_eventing-kafka-broker_main

Trying head with #2469

pierDipi · 2022-08-05T07:14:45Z

/test upgrade-tests_eventing-kafka-broker_main

Trying agin... I guess there's a real problem, but I don't believe it is related to my changes.

This seems to be the issue: #2427

aliok · 2022-08-05T07:18:26Z

This seems to be the issue: #2427

I am gonna try 2-3 more times and then disable the upgrade tests.

pierDipi · 2022-08-05T07:21:10Z

control-plane/cmd/kafka-controller/main.go

+		// Namespaced broker controller
+		injection.NamedControllerConstructor{
+			Name: "broker-namespaced-controller",
+			ControllerConstructor: func(ctx context.Context, watcher configmap.Watcher) *controller.Impl {
+				return broker.NewNamespacedController(ctx, watcher, brokerEnv)
+			},
+		},
+
+		// Namespaced trigger controller
+		injection.NamedControllerConstructor{
+			Name: "trigger-namespaced-controller",
+			ControllerConstructor: func(ctx context.Context, watcher configmap.Watcher) *controller.Impl {
+				return trigger.NewNamespacedController(ctx, watcher, brokerEnv)
+			},
+		},


We're enabling these controllers which means that they will be released but I think we should solve the known issues first like:

Namespaced broker implementation - initial implementation with disabled controllers #2361 (comment)

or the list in Namespaced broker implementation - initial implementation with disabled controllers #2361

/hold

OK we need to make a decision:

Release with poor CM support

Block the release until CM issues are completely fixed

I'd prefer solving known issues because right now these deployments aren't debuggable at all, no tracing, no logging config or stale ConfigMaps contain buggy behavior (consumer and producer configs)

pierDipi · 2022-08-05T07:49:19Z

This seems to be the issue: #2427

I am gonna try 2-3 more times and then disable the upgrade tests.

No, please going back at debugging is worst, someone needs to take a closer look at what's happening and de-flake jobs.

aliok · 2022-08-05T08:08:12Z

No, please going back at debugging is worst, someone needs to take a closer look at what's happening and de-flake jobs.

I didn't understand. I don't want to fix an irrelevant issue part of this work.
That upgrade tests are blocking this PR from getting merged. IMO blocking every PR until that issue is solved is meaningless.

knative-prow-robot · 2022-08-21T07:24:00Z

@aliok: PR needs rebase.

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.

knative-prow · 2022-09-06T14:29:05Z

@aliok: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
integration-tests-namespaced-broker_eventing-kafka-broker_main	`49e80b6`	link	true	`/test integration-tests-namespaced-broker_eventing-kafka-broker_main`
reconciler-tests-namespaced-broker_eventing-kafka-broker_main	`49e80b6`	link	true	`/test reconciler-tests-namespaced-broker_eventing-kafka-broker_main`

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.

aliok · 2022-11-30T08:59:59Z

/close

@pierDipi FYI, contents of this PR is in upstream/main already.

knative-prow · 2022-11-30T09:00:03Z

@aliok: Closed this PR.

In response to this:

/close

@pierDipi FYI, contents of this PR is in upstream/main already.

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.

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

knative-prow bot added area/control-plane area/test approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 2, 2022

aliok force-pushed the 2022-08-02-e2e-test-namespaced-broker branch from 61f0698 to 3969bfd Compare August 2, 2022 10:30

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 2, 2022

aliok force-pushed the 2022-08-02-e2e-test-namespaced-broker branch 4 times, most recently from a01ab14 to bf20b7c Compare August 2, 2022 12:23

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2022

aliok force-pushed the 2022-08-02-e2e-test-namespaced-broker branch from 5bb12e6 to 3ba227e Compare August 3, 2022 06:28

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 3, 2022

aliok force-pushed the 2022-08-02-e2e-test-namespaced-broker branch 2 times, most recently from 1539916 to 048218f Compare August 3, 2022 09:59

aliok mentioned this pull request Aug 3, 2022

Add new jobs for namespaced broker and adjust existing jobs knative/test-infra#3466

Merged

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

aliok added 3 commits August 4, 2022 09:39

Enable namespaced controllers

65da1ea

Make e2e test scripts be able to recognize BROKER_CLASS env var

017055a

Pass new broker class to GH actions

49e80b6

aliok force-pushed the 2022-08-02-e2e-test-namespaced-broker branch from 048218f to 49e80b6 Compare August 4, 2022 06:39

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2022

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2022

aliok changed the title ~~[WIP] E2E tests for namespaced broker~~ GH actions - E2E tests for namespaced broker Aug 4, 2022

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2022

aliok commented Aug 4, 2022

View reviewed changes

pierDipi reviewed Aug 5, 2022

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2022

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

knative-prow bot closed this Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH actions - E2E tests for namespaced broker #2450

GH actions - E2E tests for namespaced broker #2450

aliok commented Aug 2, 2022 •

edited

knative-prow bot commented Aug 2, 2022

codecov bot commented Aug 2, 2022 •

edited

aliok commented Aug 3, 2022

aliok commented Aug 3, 2022

aliok Aug 4, 2022

pierDipi Aug 4, 2022

aliok Aug 4, 2022

pierDipi Aug 5, 2022 •

edited

aliok Aug 5, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 5, 2022

pierDipi commented Aug 5, 2022

aliok commented Aug 5, 2022

pierDipi Aug 5, 2022

aliok Aug 5, 2022

pierDipi Aug 5, 2022 •

edited

pierDipi commented Aug 5, 2022

aliok commented Aug 5, 2022

knative-prow-robot commented Aug 21, 2022

knative-prow bot commented Sep 6, 2022

aliok commented Nov 30, 2022

knative-prow bot commented Nov 30, 2022

		# TODO: going to be reverted once we have the Prow changes in https://github.com/knative/test-infra
		export BROKER_CLASS="Kafka"

GH actions - E2E tests for namespaced broker #2450

GH actions - E2E tests for namespaced broker #2450

Conversation

aliok commented Aug 2, 2022 • edited

Proposed Changes

knative-prow bot commented Aug 2, 2022

codecov bot commented Aug 2, 2022 • edited

Codecov Report

aliok commented Aug 3, 2022

aliok commented Aug 3, 2022

aliok Aug 4, 2022

Choose a reason for hiding this comment

pierDipi Aug 4, 2022

Choose a reason for hiding this comment

aliok Aug 4, 2022

Choose a reason for hiding this comment

pierDipi Aug 5, 2022 • edited

Choose a reason for hiding this comment

aliok Aug 5, 2022

Choose a reason for hiding this comment

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 4, 2022

aliok commented Aug 5, 2022

pierDipi commented Aug 5, 2022

aliok commented Aug 5, 2022

pierDipi Aug 5, 2022

Choose a reason for hiding this comment

aliok Aug 5, 2022

Choose a reason for hiding this comment

pierDipi Aug 5, 2022 • edited

Choose a reason for hiding this comment

pierDipi commented Aug 5, 2022

aliok commented Aug 5, 2022

knative-prow-robot commented Aug 21, 2022

knative-prow bot commented Sep 6, 2022

aliok commented Nov 30, 2022

knative-prow bot commented Nov 30, 2022

aliok commented Aug 2, 2022 •

edited

codecov bot commented Aug 2, 2022 •

edited

pierDipi Aug 5, 2022 •

edited

pierDipi Aug 5, 2022 •

edited