feat: add $RYUK_CONNECTION_TIMEOUT to configure connection timeout

[hhsnopek]

Context

This pull request adds the ability to configure ryku's connection timeout by either a flag or environment variable RYKU_CONNECTION_TIMEOUT and if both are defined the flag is preferred. The goal of this change is to provide a way to define the reaper's timeout when environments take longer than 60 seconds to initialize.

[hhsnopek]

should the FlagSet name be ryuk or moby-ryuk?

[kiview]

We use ryuk.container.timeout as the property in Java.

[hhsnopek]

This references the name of the program which called by the command line. In main it shows the following:

$ ./bin/moby-ryuk -h
Usage of ./bin/moby-ryuk:
  -p int
        Port to bind at (default 8080)

With the flagset:

$ ./bin/moby-ryuk -h
Usage of ryuk:
  -p int
        Port to bind at (default 8080)
  -timeout duration
        Connection timeout (default 1m0s)

[kiview]

Thanks @hhsnopek, in which context did you have the need to increase the Ryuk timeout?

We added the same to Testcontainers for Java some years ago, so I generally understand the need for it:

• Ryuk startup timeout is too low testcontainers-java#621
• #621 increase Ryuk's timeout and make it configurable testcontainers-java#635

In Java, we are using our usual configuration mechanism, which ENV, ~/.testcontainers.properties and classpath:testcontainers.properties, in that order. In know we also have ~/.testcontainers.properties in Go, is it already hooked up in this case (sorry for asking, I am not super familiar with the codebase, to easily see it from the diff)? And is a flag something idiomatic to our Go implementation?

[bsideup]

FTR it is "ryuk", not "ryku" 😅

Also, I am not sure we need flags here, as Ryuk isn't intended to be used as a standalone CLI tool, so I would focus on environment variables only 👍

[hhsnopek]

@kiview My current use cases are:

• debugging Colima issues
• ensuring that the ryuk doesn't timeout before the Strategy.Deadline/Timeout timeout occurs

Go processes are typically configured by flags and environment variables, like Docker's convention where you can use environment variables, flags, and local/global configuration (toml, yaml, json) files. I often see environment variables with a namespace prefix, like DOCKER_ or PG, which matches the RYUK_ prefix in the PR.

@bsideup since Ryuk runs in a docker container and could be configured by an environment variable and a flag passed in cmd, I would suggest keeping both options open. With both exposed the interface/configuration later exposed in testcontainers-go via reaper.go can also override either programmatically. The order of priority would be: default (60s), environment variable, flag, testcontainers-go definition which would match Java's configuration prioritization.

[kiview]

Thanks for clarification @hhsnopek. I was in the wrong headspace when I looked at the PR yesterday and thought it was about tc-go, so ignore my detailed comments above 😅

If it is Go idiomatic to have flag as well as ENV I am fine with it, but I would also be fine to only have the ENV variable if it helps to cut down the amount of implementation code, since the Testcontainers projects users of Ryuk can configure the container accordingly through the ENV variable.

Was the flag something that made your debugging or development work easier?

[hhsnopek]

I used the flag in both testing and debugging by updating my local docker image to have a static flag in CMD. The environment variable could have also been used. To be honest, Ryuk can just use the environment variable - this addition alone does nothing until the environment variable is forwarded to the container in testcontainers-go

I'm happy to remove the flag, I don't think Go has an idiom for this. I was mainly sharing common usage/examples 😄

[kiview]

Sounds good, then let's just minimize the PR and the implementation. WDYT @mdelapenya?

[hhsnopek]

@mdelapenya is there any pending changes required before merging? I believe all feedback has been addressed, please let me know if I have not.

[kiview]

@hhsnopek LGTM. Given the fact that @mdelapenya currently takes a bigger responsibility in the maintenance and development of Ryuk and given testcontainers/testcontainers-go#653, is it ok for you if we wait until @mdelapenya is back?

[mdelapenya]

LGTM, thanks for your patience during the review 🙏

[mdelapenya]

BTW, I've just refreshed myself with the comments, specially #49 (comment) and #49 (comment).

Let's remove the flag support and keep the env variables only, as suggested. Other than that, the code is fine: once there we are ready to merge

[mdelapenya]

I'm revisiting this PR after being back from leave. As mentioned above, we are going to focus on env vars for the connection timeout config. But then, as a consequence, we should remove the already existing flag for the port in which Ryuk is listening, to be consistent while configuring its execution. Probably in a follow-up PR.

The flag for the port already exists:

Usage of ryuk:
  -p int
        Port to bind at (default 8080)

[mdelapenya]

@hhsnopek thanks for your work here. I'm finally merging this PR, and created a follow-up issue to move all flags to env vars.