-
-
Notifications
You must be signed in to change notification settings - Fork 549
feat(kafka,redpanda): support for waiting for mapped ports without external checks #3165
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.
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
feat(kafka,redpanda): support for waiting for mapped ports without external checks #3165
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
53f804f
to
10e0579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand the issue you're experiencing now, so many thanks for all your effort on this.
If you could review the comment below to confirm we're on the same page and if so you're thoughts taking on the suggestion of how to proceed?
856accf
to
4658171
Compare
Hi,
This is done (forced pushed changes into this PR. updated repro repositories and rerun tests). Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, getting there.
Have put some suggestions for you, if you aren't aware we use conventional comments to help folks understand the intent better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed clicking request changes, see above.
Hi @stevenh, Regarding your ask for details:
I updated https://github.com/mabrarov/testcontainers-go-kafka-2748/blob/master/README.md and the last screenshot (at the bottom of file) confuses me - it looks like wrong port is checked there (33509 instead of 33508) - so I'm going to investigate it deeper. Thank you. |
Not sure I understand 33508 is the ryuk port, 33509 is the kafka ports, so its what I would expect? |
Hi @stevenh, Yes, my test was wrong (was checking wrong port from PowerShell). Updating it right now. Thank you. |
Hi @stevenh, https://github.com/mabrarov/testcontainers-go-kafka-2748/blob/master/README.md is fixed now and is ready for your review (Local Docker Engine and Remote Docker Engine sections). In the meantime I'm going to address this pull request comments. Thank you for your patience. |
4658171
to
e0d741f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close, just a few bits to fix.
So it looks like nc is getting the right results, could you also run a test with go: package main
import (
"context"
"fmt"
"net"
"os"
"time"
)
func main() {
if len(os.Args) < 2 {
fmt.Println("Usage: go run main.go <host:port>")
return
}
address := os.Args[1]
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
var dialer net.Dialer
conn, err := dialer.DialContext(ctx, "tcp", address)
if err != nil {
fmt.Printf("Dial error: %#v\n", err)
return
}
defer conn.Close()
fmt.Printf("Connected to: %q\n", address)
} |
e0d741f
to
23904bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is now an elegant solution, thanks for all your hard work on this.
The only thing missing which @mdelapenya would pick up on is it needs docs for the new methods, sorry I always forget too as go docs are generated from the code, but testcontainers-go also has its own site, which needs manually added docs unfortunately.
Hi @stevenh,
Done - updated https://github.com/mabrarov/testcontainers-go-kafka-2748/blob/master/README.md Thank you. |
c24b903
to
3aa43bd
Compare
Grr, sorry we need to unpack the error a bit: package main
import (
"context"
"errors"
"fmt"
"net"
"os"
"time"
)
func main() {
if len(os.Args) < 2 {
fmt.Println("Usage: go run main.go <host:port>")
return
}
address := os.Args[1]
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
var dialer net.Dialer
conn, err := dialer.DialContext(ctx, "tcp", address)
if err != nil {
var opErr *net.OpError
if errors.As(err, &opErr) {
var syscallErr *os.SyscallError
if errors.As(opErr.Err, &syscallErr) {
fmt.Printf("Dial error: %#v\n", syscallErr)
return
}
fmt.Printf("Dial error: %#v\n", opErr)
return
}
fmt.Printf("Dial error: %#v\n", err)
return
}
defer conn.Close()
fmt.Printf("Connected to: %q\n", address)
} |
@mabrarov can I also see the result of |
304a972
to
58cc8a8
Compare
Hi @mdelapenya, Regarding my comment about unstable tests in reuse_test.go:
I tried main branch on Rocky Linux 9.5 with Docker Engine 28.1.1 and found that sometimes test passes:
and sometimes test fails:
Note that there is no "Starting container: cf711b2a8ec9" message before the last "Container started: cf711b2a8ec9" message. Here are screenshots from debugger for another run and similar fail of the same test (in order of screenshot creation): I suspect Docker Daemon caches state of container or uses asynchronous logic when container is stopped which causes Docker client used by Testcontainers for Go to report about container running. It seems to be another issue - not related to this pull request (example of the same issue on another pull request) - which requires dedicated investigation (including Docker Engine bug-tracker review) and workaround / fix. Thank you. |
Working on this now |
d2b148b
to
80aa134
Compare
…which is not possible due to Kafka is not started at that point of time) in container post start hook which prepares configuration for Kafka (fix for testcontainers#2748). Signed-off-by: Marat Abrarov <[email protected]>
…them (which is not possible due to Redpanda is not started at that point of time) in container post start hook before preparing configuration for Redpanda which requires completion of port mapping (fix for testcontainers#2748). Signed-off-by: Marat Abrarov <[email protected]>
…ainer with Redpanda (which is Linux based) to run correctly when host OS is Windows. Signed-off-by: Marat Abrarov <[email protected]>
80aa134
to
456836b
Compare
Signed-off-by: Marat Abrarov <[email protected]>
Signed-off-by: Marat Abrarov <[email protected]>
…waiting for port mapping completion. Signed-off-by: Marat Abrarov <[email protected]>
Hi @mdelapenya, I opened #3177 for the issue described in #3165 (comment). Sorry, but I failed to find time to make a deeper investigation (like checking Docker Engine issues or checking if some sort of caching happens inside Testcontainers for Go). Thank you. |
456836b
to
f2c8549
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @mabrarov for your enormous work in this PR, addressing all the suggestions from the review and also contributing with a deep research on the port mapping issue.
In fact, this PR helped us to take decisions on the upcoming PRs, which will remove the readiness check for mapped ports, among other improvements.
Thanks for your time and dedication to make Testcontainers better 🙇
@mabrarov I've renamed the PR title so that it's even clearer what we do when reading the release notes. Please let me know what you think |
Merged, thank you so much! |
* main: (236 commits) feat(kafka,redpanda): support for waiting for mapped ports without external checks (testcontainers#3165) chore: bump ryuk to 0.12.0 (testcontainers#3195) feat!: add options when creating RawCommand (testcontainers#3168) chore(deps)!: bump github.com/docker/docker from 28.1.1+incompatible to 28.2.2+incompatible (testcontainers#3194) feat(couchbase): adding auth to couchbase initCluster functions to support container reuse (testcontainers#3048) chore(deps): bump github.com/containerd/containerd/v2 (testcontainers#3167) docs(options): refactor options layout in modules (testcontainers#3163) fix(ci): do not run sonar for Testcontainers Cloud (testcontainers#3166) chore(ci): do not fail fast in the Testcontainers Cloud run (testcontainers#3164) feat: support adding wait strategies as functional option (testcontainers#3161) fix(etcd): expose ports for the etcd nodes (testcontainers#3162) fix(wait): no port to wait for (testcontainers#3158) feat: add more functional options for customising containers (testcontainers#3156) docs(redpanda): update sasl authentication option to use scram sha 256 (testcontainers#3126) chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3137) chore(deps): bump github.com/shirou/gopsutil/v4 from 4.25.1 to 4.25.4 (testcontainers#3133) chore(deps): bump github.com/docker/docker from 28.0.1+incompatible to 28.1.1+incompatible (testcontainers#3152) feat(memcached): add memcached module (testcontainers#3132) fix(etcd): single node etcd cluster access (testcontainers#3149) feat(valkey): add TLS support for Valkey (testcontainers#3131) ...
* main: fix: workaround for moby/moby#50133 when reusing container in testcontainers.GenericContainer. (testcontainers#3197) feat(kafka,redpanda): support for waiting for mapped ports without external checks (testcontainers#3165)
Changes
What does this PR do?
HostPort
strategy, which waits for a MappedPort. This is useful for situations when you just need the port to be ready, but you don't need to perform neither an internal or external check for it. For that reason, a newSkipExternalCheck
fluent method has been added to the strategy, for users to created more advanced waits.Why is it important?
Related issues
How to test this PR
Refer to:
Tested environments / cases (along with environments / tests described in repro repo listed above):
Follow-ups
Refer to #2670 (comment).