Skip to content

Identical Listeners test - #2563

Open
mgoulish wants to merge 6 commits into
skupperproject:mainfrom
mgoulish:twin_listeners
Open

Identical Listeners test#2563
mgoulish wants to merge 6 commits into
skupperproject:mainfrom
mgoulish:twin_listeners

Conversation

@mgoulish

@mgoulish mgoulish commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

This test takes a while -- every time I have run it, it has taken very close to 27 seconds.

Create two Listeners on a Site, identical except for their names.

The first will become active -- it will own the Service, and its name will show up in the Router Config.

The second one will be rejected with an error.

But then if you kill the first one, after some time the second one will get adopted.
It will own the Service, and its name will show up in the Router Config.

Summary by CodeRabbit

  • Tests
    • Added integration coverage for identical listener failover regardless of listener ordering.
    • Verified duplicate listeners are rejected while the active listener owns the shared service and router entry.
    • Confirmed the standby listener takes over after the active listener is removed and its duplicate error clears.
    • Validated service ownership, router configuration, and listener readiness after takeover.
    • Improved documentation for related listener lifecycle and stress tests.
  • Documentation
    • Corrected a certificate-related comment.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 66c7c56a-2f22-4cc5-9ab8-929c3e0a225b

📥 Commits

Reviewing files that changed from the base of the PR and between c96d4be and 04f6f68.

📒 Files selected for processing (1)
  • tests/integration/kube/controller/listener_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Adds integration coverage for identical listener conflict handling. The test dynamically identifies the active listener, deletes it, and confirms that the remaining listener becomes configured and owns the shared Service and router entry. It also corrects a certificate test comment.

Changes

Listener failover

Layer / File(s) Summary
Listener failover integration test
tests/integration/kube/controller/listener_test.go
Adds dynamic owner detection, deletion checks through Kubernetes API errors, duplicate-error clearing, and validation of Service and router ownership by the remaining listener.
Certificate test comment cleanup
internal/kube/certificates/mgr_test.go
Corrects the managedWithOwnerHosts comment typo.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🔵 Low · up to 04f6f

The test may proceed before listener takeover is fully complete, which could cause intermittent or misleading failures. The PR is otherwise mergeable, with explicit follow-up to require the completed configured state before polling returns.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: adding an integration test for identical Listeners and their failover behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

level=error msg="[linters_context] typechecking error: build constraints exclude all Go files in /tests/integration/kube/controller"


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a53b3f86-8e24-4913-b430-081c51ef6db0

📥 Commits

Reviewing files that changed from the base of the PR and between 06223ca and 4d34ba9.

📒 Files selected for processing (1)
  • tests/integration/kube/controller/listener_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Comment thread tests/integration/kube/controller/listener_test.go

// Wait until 'B' is rejected because it's a duplicate
var lb *skupperv2alpha1.Listener
waitFor(t, 30*time.Second, 250*time.Millisecond, func() (bool, error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Kubernetes does not necessarily guarantee that resources get "created" in the order submitted. While it is likely that the second listener will be rejected the test would be improved if it anticipates either listener can claim being the initial owner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I will remember this.

I think I have taken care of it now by just checking to see which listener comes out on top.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants