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.

Sign up for GitHub

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

Jump to bottom

GH actions - E2E tests for namespaced broker #2450

Closed
wants to merge 3 commits into from
Closed

GH actions - E2E tests for namespaced broker #2450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65da1ea
Enable namespaced controllers
aliok Aug 3, 2022
017055a
Make e2e test scripts be able to recognize BROKER_CLASS env var
aliok Aug 3, 2022
49e80b6
Pass new broker class to GH actions
aliok Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/kind-e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ jobs:

broker-class:
- Kafka
- KafkaNamespaced

test-suite:
- ./test/e2e
Expand All @@ -50,6 +51,20 @@ jobs:
- test-suite: ./test/e2e_channel
extra-test-flags: -channels=messaging.knative.dev/v1beta1:KafkaChannel

exclude:
- test-suite: ./test/e2e
broker-class: KafkaNamespaced
- test-suite: ./test/e2e_sink
broker-class: KafkaNamespaced
- test-suite: ./test/e2e_source
broker-class: KafkaNamespaced
- test-suite: ./test/e2e_channel
broker-class: KafkaNamespaced
- test-suite: ./test/e2e_new_channel
broker-class: KafkaNamespaced
- test-suite: ./test/experimental
broker-class: KafkaNamespaced

env:
GOPATH: ${{ github.workspace }}
KO_DOCKER_REPO: localhost:5001
Expand Down
31 changes: 15 additions & 16 deletions control-plane/cmd/kafka-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,22 +76,21 @@ func main() {
},
},

// TODO: enable later
//// 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)
// },
//},
// 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)
},
},
Comment on lines +79 to +93
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

/hold

OK we need to make a decision:

  • Release with poor CM support
  • Block the release until CM issues are completely fixed

Copy link
Member

@pierDipi pierDipi Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)


// Channel controller
injection.NamedControllerConstructor{
Expand Down
14 changes: 14 additions & 0 deletions test/e2e-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,22 @@ if [ "${EVENTING_KAFKA_BROKER_CHANNEL_AUTH_SCENARIO:-""}" != "" ]; then
success
fi

# TODO: going to be reverted once we have the Prow changes in https://github.com/knative/test-infra
export BROKER_CLASS="Kafka"
Comment on lines +30 to 31
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally always prefer explicit errors over invisible defaulting.

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

Copy link
Member

@pierDipi pierDipi Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The job is there, can we fix the TODOs?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


if [[ -z "${BROKER_CLASS}" ]]; then
fail_test "Broker class is not defined. Specify it with 'BROKER_CLASS' env var."
else
echo "BROKER_CLASS is set to '${BROKER_CLASS}'. Running tests for that broker class."
fi

if [ "${BROKER_CLASS}" == "KafkaNamespaced" ]; then
# if flag exists, only test tests that are relevant to namespaced KafkaBroker
echo "BROKER_CLASS is set to 'KafkaNamespaced'. Only running the relevant tests."
go_test_e2e -timeout=1h ./test/e2e_broker/... || fail_test "E2E suite failed (directory: ./e2e_broker/...)"
success
fi

go_test_e2e -timeout=1h ./test/e2e/... || fail_test "E2E suite failed (directory: ./e2e/...)"
go_test_e2e -timeout=1h ./test/e2e_broker/... || fail_test "E2E suite failed (directory: ./e2e_broker/...)"
go_test_e2e -timeout=1h ./test/e2e_sink/... || fail_test "E2E suite failed (directory: ./e2e_sink/...)"
Expand Down
14 changes: 14 additions & 0 deletions test/reconciler-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,22 @@ if [ "${EVENTING_KAFKA_BROKER_CHANNEL_AUTH_SCENARIO:-""}" != "" ]; then
success
fi

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

if [[ -z "${BROKER_CLASS}" ]]; then
fail_test "Broker class is not defined. Specify it with 'BROKER_CLASS' env var."
else
echo "BROKER_CLASS is set to '${BROKER_CLASS}'. Running tests for that broker class."
fi

if [ "${BROKER_CLASS}" == "KafkaNamespaced" ]; then
# if flag exists, only test tests that are relevant to namespaced KafkaBroker
echo "BROKER_CLASS is set to 'KafkaNamespaced'. Only running the relevant tests."
go_test_e2e -timeout=1h ./test/e2e_new/... || fail_test "E2E (new) suite failed"
success
fi

go_test_e2e -timeout=1h ./test/e2e_new/... || fail_test "E2E (new) suite failed"

go_test_e2e -tags=e2e,cloudevents -timeout=1h ./test/e2e_new_channel/... || fail_test "E2E (new - KafkaChannel) suite failed"
Expand Down
1 change: 1 addition & 0 deletions test/upgrade-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ set -Eeuo pipefail
TIMEOUT=${TIMEOUT:-100m}
GO_TEST_VERBOSITY="${GO_TEST_VERBOSITY:-standard-verbose}"

# TODO: we don't support upgrade tests for namespaced KafkaBroker right now
export BROKER_CLASS="Kafka"

EVENTING_KAFKA_BROKER_UPGRADE_TESTS_FINISHEDSLEEP="5m" \
Expand Down