Skip to content

Conversation

@creydr
Copy link
Contributor

@creydr creydr commented Jan 28, 2026

The upgrade tests were failing with generic "invalid topic" errors that didn't indicate which validation check failed, making diagnosis impossible.
This PR addresses it and enhances the InvalidOrNotPresentTopic error to include specific failure reason.

The upgrade tests were failing with generic "invalid topic" errors that
didn't indicate which validation check failed, making diagnosis impossible.

Problem:
KafkaSink controller's topic validation checked multiple conditions but
returned the same error for all failures:
- Topic not found in DescribeTopics response
- Topic has zero partitions
- Topic name mismatch
- Topic marked as internal

Solution:
Enhanced InvalidOrNotPresentTopic error to include specific failure reason:
- Show which topics Kafka returned when topic not found
- Report exact validation failure (no partitions, name mismatch, etc.)
- Added debug logging in reconciler for validation attempts

Example improved errors:
- "invalid topic foo: topic not found in metadata response, got topics: [bar, baz]"
- "invalid topic foo: validation failed: no partitions"
- "invalid topic foo: validation failed: name mismatch (expected: foo, got: foo-bar)"

Impact:
Operators can now distinguish configuration errors from timing issues
like metadata propagation delays. Future test failures will provide
actionable diagnostics instead of generic error messages.
@knative-prow knative-prow bot added area/control-plane size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 28, 2026
@knative-prow knative-prow bot requested review from Cali0707 and matzew January 28, 2026 10:31
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2026
@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 48.38710% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.17%. Comparing base (88b31aa) to head (81df5e8).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
control-plane/pkg/kafka/topic.go 54.16% 8 Missing and 3 partials ⚠️
...rol-plane/pkg/reconciler/testing/objects_broker.go 0.00% 2 Missing ⚠️
...rol-plane/pkg/reconciler/testing/objects_common.go 0.00% 2 Missing ⚠️
control-plane/pkg/reconciler/sink/kafka_sink.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4641      +/-   ##
============================================
+ Coverage     28.81%   36.17%   +7.36%     
- Complexity        0      901     +901     
============================================
  Files           294      403     +109     
  Lines         16149    20002    +3853     
  Branches          0      342     +342     
============================================
+ Hits           4653     7236    +2583     
- Misses        11044    12127    +1083     
- Partials        452      639     +187     
Flag Coverage Δ
java-unittests 67.15% <ø> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The StatusTopicNotPresentOrInvalid helper function now accepts the list
of available topics returned by Kafka, allowing tests to verify the new
detailed error messages that show exactly which topics were found when
the requested topic is missing.

Changes:
- Updated StatusTopicNotPresentOrInvalid signature to accept availableTopics
- Updated StatusExternalBrokerTopicNotPresentOrInvalid wrapper
- Updated test expectation in external_topic_not_present_or_invalid test
- Error message now shows: "topic not found in metadata response, got topics: [test-topic]"

This makes the test expectations flexible and avoids hardcoding topic
names in the common helper function.
@creydr
Copy link
Contributor Author

creydr commented Jan 28, 2026

/retest-required

@creydr
Copy link
Contributor Author

creydr commented Feb 11, 2026

/cc @Cali0707 @matzew

Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks @creydr - this should really simplify debugging common issues!

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2026
@knative-prow
Copy link

knative-prow bot commented Feb 11, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, creydr

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@Cali0707
Copy link
Member

@creydr do you want to backport this so that we also get the error logs on the past release?

@creydr
Copy link
Contributor Author

creydr commented Feb 11, 2026

/cherry-pick release-1.21

@knative-prow-robot
Copy link
Contributor

@creydr: once the present PR merges, I will cherry-pick it on top of release-1.21 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.21

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-sigs/prow repository.

@creydr
Copy link
Contributor Author

creydr commented Feb 11, 2026

/test channel-reconciler-tests-ssl

@knative-prow
Copy link

knative-prow bot commented Feb 11, 2026

@creydr: 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
upgrade-tests_eventing-kafka-broker_main 81df5e8 link unknown /test upgrade-tests
reconciler-tests_eventing-kafka-broker_main 81df5e8 link unknown /test reconciler-tests

Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants