Skip to content

Bugfix: kafka not found port error#2552

Closed
wilsouza wants to merge 4 commits intotestcontainers:mainfrom
wilsouza:fix-workaround-for-kafka-not-found-port
Closed

Bugfix: kafka not found port error#2552
wilsouza wants to merge 4 commits intotestcontainers:mainfrom
wilsouza:fix-workaround-for-kafka-not-found-port

Conversation

@wilsouza
Copy link
Contributor

@wilsouza wilsouza commented May 23, 2024

What does this PR do?

This PR addresses the Kafka startup failure related to issue #2543. It implements a workaround to ensure that the container maps the ports before attempting to retrieve them.

Why is it important?

This fix resolves a problem in the Kafka module that was causing startup failures due to port mapping issues.

Related issues

How to test this PR

package main

import (
	"context"
	"log"

	"github.com/testcontainers/testcontainers-go/modules/kafka"
)

func main() {
	ctx := context.Background()

	kafkaContainer, err := kafka.RunContainer(ctx,
		kafka.WithClusterID("test-cluster"),
	)
	if err != nil {
		log.Fatalf("failed to start container: %s", err)
	}

	// Clean up the container after
	defer func() {
		if err := kafkaContainer.Terminate(ctx); err != nil {
			log.Fatalf("failed to terminate container: %s", err)
		}
	}()
}

@wilsouza wilsouza requested a review from a team as a code owner May 23, 2024 16:59
@netlify
Copy link

netlify bot commented May 23, 2024 •
edited
Loading

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 7960122
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/666cfd007d7acd0008c5d54a
😎 Deploy Preview https://deploy-preview-2552--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

AhmedNSidd
AhmedNSidd reviewed May 23, 2024
View reviewed changes
AhmedNSidd
AhmedNSidd approved these changes May 24, 2024
View reviewed changes
Copy link

@AhmedNSidd AhmedNSidd left a comment

Choose a reason for hiding this comment

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

I tried using this for my kafka port not found error cases, and this fixed the issues I was running into.

@wilsouza wilsouza force-pushed the fix-workaround-for-kafka-not-found-port branch 2 times, most recently from c5fb1e2 to 8b27632 Compare May 27, 2024 15:06
@wilsouza wilsouza mentioned this pull request May 29, 2024
Closed
@mgilbir
Copy link

mgilbir commented Jun 14, 2024

I've tried it and it fixes most of the kafka port not found issues. Thanks!
I am still hitting some port not found: creating reaper failed: failed to create container

@wilsouza wilsouza force-pushed the fix-workaround-for-kafka-not-found-port branch from 3bae926 to 7960122 Compare June 15, 2024 02:31
@mdelapenya
Copy link
Member

mdelapenya commented Jun 26, 2024

Hi @wilsouza thanks for submitting this quick fix to the original issue. Unfortunately, I think we should not add a time.Sleep here as it would not make the code deterministic.

I'm working on #2606, which adds a condition to check that all the exposed ports have been already made available by the container runtime, and this check happens before the wait strategies. I'd appreciate your feedback in that issue if possible.

So because of that, I'd close this one.

@mdelapenya mdelapenya mentioned this pull request Jun 26, 2024
Merged
@wilsouza
Copy link
Contributor Author

wilsouza commented Jul 3, 2024

Hi @wilsouza thanks for submitting this quick fix to the original issue. Unfortunately, I think we should not add a time.Sleep here as it would not make the code deterministic.

I'm working on #2606, which adds a condition to check that all the exposed ports have been already made available by the container runtime, and this check happens before the wait strategies. I'd appreciate your feedback in that issue if possible.

So because of that, I'd close this one.

No worries. I understand. My intent was to solve the issue without altering the core features of Testcontainers.
I considered the possibility of removing the cached Inspect specifically for the MappedPort API, but I was unsure of the potential impacts of this change.
If there’s anything else I can assist with, please let me know.

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

Reviewers

1 more reviewer

@AhmedNSidd AhmedNSidd AhmedNSidd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Fail to start Kafka container (KRaft) with basic setup

4 participants

@wilsouza @mgilbir @mdelapenya @AhmedNSidd