feat: support assigning custom StartupTimeout within the elasticsearch module

[james-async]

What does this PR do?

Adds an Option function to support configuring the timeout used for the internal WaitFor Strategies. I did my best to follow the example laid down by the existing WithPassword Option.

Because this is an internal concern that was is not exposed for assertions by the existing elasticsearch_test.go, I added the elasticsearch_internal_test. I saw no existing internal test, so went with this naming pattern. Adding this test allowed me to extend the test coverage across the scenario when the incoming request already has a WaitFor Strategy.

Why is it important?

We were seeing intermittent timeout while running integration tests in Github PR Actions. The StartupTimeout needed to be applied to the internally controlled Strategies.

Related issues

• Closes [Enhancement]: the elasticsearch configurable timeout #2982

How to test this PR

I tested the PR by configuring a testcontainer that uses the option. Not only did my Github actions pass, but also, by adding a logger, I can see the timeout is applied to the WaitFor Strategy:

...
elasticsearchContainer, err := elasticsearch.Run(
	ctx,
	"docker.elastic.co/elasticsearch/elasticsearch:8.17.2",
	testcontainers.WithLogger(logger{}), //a custom fmt.Printf logger, or the supported TestLogger
	elasticsearch.WithStartupTimeout(2*time.Minute), // adding the feature from this PR
)
closer := func(ctx context.Context) error {
	return elasticsearchContainer.Terminate(ctx)
}
...

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	c09215f
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/67b882d22342cc0008c22e8c
😎 Deploy Preview	https://deploy-preview-2983--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[stevenh]

Thanks for the PR @james-async can you confirm the issue you're seeing is that the default timeout of 1 minute is too short for the container to create the Cert?

If so do you have logs as running a test using ExampleRun_connectUsingElasticsearchClient completes in 6 seconds here.

[james-async]

correct. My situation saw external factors that slowed down the test. In isolation it is rather fast. Our environment has multiple teams sharing an Action runner pool. Within the project where I am using this there are multiple integration test suites. Two of them use elasticsearch. I am moving them away from legacy homegrown test container solution. Our environment is such that the action runners are sometimes under heavy load. One of the first runs using testcontainers and this module failed in one of those suites but not the other -- panic: generic container: start container: started hook: wait until ready: context deadline exceeded. When I re-ran the job for the one suite that timed out, it passed. The runners where busy, causing delay.

I have been running using my fork with a 2min StartupTimeout and have not seen another problem.

[mdelapenya]

suggestion: we prefer having newly added subtest names without spaces in order to be able to copy&paste them in case they fail. At some point we are going to rename the existing ones, in the meantime we encourage using this format for the new ones.

[james-async]

I think I found and changed all the tests in this module. In my experience, it takes incremental change for such changes.

[james-async]

Are there different changes you were looking for on this suggestion that I have not resolved it with the changes I made?

[mdelapenya]

@james-async no, thanks for the ping. We were thinking more in depth about this use case, and I was wondering why the passed Go context is not controlling the timeouts here 🤔 Also @stevenh suggested online if we could, as a workaround, control it wrapping the wait strategy into a wait.ForAll(...).WithDeadline(...)

We think that the WithStartupTimeout name that is present in all strategies is misleading, as it should simply be WithTimeout, and maybe this is creating the need for this PR. Could you verify what we proposed about: using the ForAll and controlling with the context timeout?

[james-async]

Absolutely, my problem would be solved. From the client contract perspective I do think it would be natural to create a context.WithTimeout to pass into Run

ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
defer cancel()
elasticsearchContainer, err := elasticsearch.Run(
	ctx,
	"docker.elastic.co/elasticsearch/elasticsearch:8.17.2",
)

I might be seeing a complexity to using wait.ForAll internally. I see a subset of wait implementations that are building a context.WithTimeout from the defaultStartupTimeout() -- which was of course overidden with the changes I implemented here. The two Strategies ForFile and ForHTTP both follow this pattern.

I see a pattern repeated, with some variation, in the implementations of WaitUntilReady(context.Context, StrategyTarget) error which builds a context with a default or the given timeout. I'm assuming all of these would need to change to include if _, ok := ctx.Deadline(); !ok {. This would maintain backwards compatibility to code already using the StartupTimeout in their own wait strategies, but allow for the timeout to be driven by the context as you propose.

	timeout := defaultStartupTimeout()
	if ws.timeout != nil {
		timeout = *ws.timeout
	}

	ctx, cancel := context.WithTimeout(ctx, timeout)
	defer cancel()

to expand upon this idea (not tested, just tossed together):

// applyTimeoutDurationIfNotInContext is a very Java-like name for a helper function 
// to go next to `defaultStartupTimeout`, but the idea is to pull out some repetition  seen 
// in the wait implements that use `defaultStartupTimeout`
func applyTimeoutDurationIfNotInContext(ctx context.Context, d *time.Duration) (context.Context, func()) {
   if _, ok := ctx.Deadline(); !ok {
   	timeout := defaultStartupTimeout()
   	if d != nil {
   		timeout = *d
   	}

   	var cancel func()
   	ctx, cancel = context.WithTimeout(ctx, timeout)
   	return ctx, cancel
   }
   return ctx, func() { /*noop*/ }
}
...
//then in each `WaitUntilReady` use the above function:
ctx, cancel := applyTimeoutDurationIfNotInContext(ctx, ws.timeout)
defer cancel()