Skip to content
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

fix: cache Docker client instead of creating a new one all the time, fixes #5861 #5878

Merged

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Feb 20, 2024

The Issue

How This PR Solves The Issue

Do not create a lot of requests for new Docker client.
Close client connection.

Manual Testing Instructions

I run TestComposerVersion several times on tb-macos-arm64-7, all green:

GOTEST_SHORT=true make testpkg TESTARGS='-run TestComposerVersion --count=1'

Automated Testing Overview

Waiting for Colima tests.

Related Issue Link(s)

Release/Deployment Notes

Copy link

github-actions bot commented Feb 20, 2024

@rfay
Copy link
Member

rfay commented Feb 20, 2024

Let's enable Rancher and macOS amd64 on this PR.

@rfay
Copy link
Member

rfay commented Feb 21, 2024

Only TestConfigValidate failed, https://github.com/ddev/ddev/actions/runs/7979112678/job/21785753458?pr=5878

Restarting. This kind of failure is quite common.

@rfay
Copy link
Member

rfay commented Feb 21, 2024

Failed again on TestConfigValidate, same thing.

Retry

@rfay
Copy link
Member

rfay commented Feb 21, 2024

We may have to use tmate to try TestConfigValidate, or see how it works on tb-macos-arm64-6

@rfay
Copy link
Member

rfay commented Feb 21, 2024

Oh look at TestConfigValidate:

func TestConfigValidate(t *testing.T) {
	if nodeps.IsAppleSilicon() {
		t.Skip("Skipping on mac M1 to ignore problems with 'connection reset by peer'")
	}

If I remove the "skip on Apple Silicon" then I get the same failures on my machine using colima:

All project containers are now ready.
    config_test.go:628: 
        	Error Trace:	/Users/rfay/workspace/ddev/pkg/ddevapp/config_test.go:628
        	Error:      	Received unexpected error:
        	            	Get "http://127.0.0.1//readme.html": dial tcp 127.0.0.1:80: connect: connection refused
        	Test:       	TestConfigValidate
    config_test.go:630: 
        	Error Trace:	/Users/rfay/workspace/ddev/pkg/ddevapp/config_test.go:630
        	Error:      	Received unexpected error:
        	            	Get "http://127.0.0.1//readme.html": dial tcp 127.0.0.1:80: connect: connection refused
        	Test:       	TestConfigValidate

These are the lines that are failing:

		_, _, err = testcommon.GetLocalHTTPResponse(t, "http://x.ddev.site/"+staticURI)
		assert.NoError(err)
		_, _, err = testcommon.GetLocalHTTPResponse(t, "http://somethjingrandom.any.ddev.site/"+staticURI)
		assert.NoError(err)

I do not get those failures on Orbstack or Docker Desktop.

I'm pretty sure the exclusion for Apple Silicon was put in there in the early unstable days of Docker Desktop on arm64. Not that it's all that stable now. But it wasn't intended to exclude Colima on arm64.

TestConfigValidate also passes on HEAD with Docker Desktop.
TestConfigValidate FAILS on HEAD with Colima, but it's a bit different, same lines, but it's the EOF error
TestConfigValidate FAILS sometimes but not always on Colima at ca7e2ab (right before API change) in the same way as above (connection refused)

It's possible that this has been a flaky test for colima for a really long time and we might be able to ignore it. It does not appear to be new with this patch.

@rfay
Copy link
Member

rfay commented Feb 21, 2024

I'm in favor of adding if IsColima() t.Skip() on TestConfigValidate. I think it's OK.

@rfay
Copy link
Member

rfay commented Feb 21, 2024

Yay, it looks like we did finally get a clean colima on 538c5ca

Congratulations. I think TestConfigValidate is just too flaky on Colima so we'll be fine to be rid of it.

@stasadev stasadev force-pushed the 20240220_stasadev_docker_client_global_vars branch from a755943 to 369dfc8 Compare February 21, 2024 17:25
@stasadev stasadev marked this pull request as ready for review February 21, 2024 17:32
@stasadev stasadev requested a review from a team as a code owner February 21, 2024 17:32
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks for the amazing work on this. I think we may have almost dug out of that hole :)

@rfay rfay changed the title fix: use global variables for Docker client, fixes #5861 fix: cache Docker client instead of creating a new one all the time, fixes #5861 Feb 21, 2024
@rfay
Copy link
Member

rfay commented Feb 21, 2024

Yay, success!

@rfay rfay merged commit 18842b6 into ddev:master Feb 21, 2024
24 checks passed
@GuySartorelli
Copy link
Collaborator

Amazing work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants