Stop using deprecated SockRequest by sbko · Pull Request #33534 · moby/moby

sbko · 2017-06-06T02:19:37Z

Signed-off-by: Stanislav Bondarenko [email protected]

Use client/ package instead of request deprecated SockRequest. If not viable to use client/ replace with new Do, Post, Get, ...

- Description for the changelog
integration-cli: Use client/ package instead of deprecated SockRequest

aaronlehmann · 2017-06-06T07:36:04Z

There are gofmt issues with a few files.

sbko · 2017-06-06T11:08:28Z

@aaronlehmann should be fixed now

sbko · 2017-06-06T12:48:26Z

looks like windowsRS1 run out of space.

not sure what to do with:
FAIL: docker_cli_run_test.go:4189: DockerSuite.TestRunStoppedLoggingDriverNoLeak
.... leaked goroutines: expected less than or equal to 1560, got: 1690" ....
using /client increased number of goroutines?

thaJeztah · 2017-06-09T00:04:28Z

Looks like a couple of tests are failing

sbko · 2017-06-09T00:32:15Z

@thaJeztah failing tests are counting leaked goroutines. I'm not sure what caused that, the only change around those tests is that getGoroutineNumber() now uses client.Info() instead of SockRequest call to "/info"

https://github.com/moby/moby/pull/33534/files/971817824022fadaa6e46ffa6853c789e8878254#diff-eb56f180020ddc0ad74fde7b5b8b55fbR404

cpuguy83 · 2017-06-09T01:16:50Z

@sbko Seems like there is something wrong with the client usage.
800+ goroutines is insanely high.
Maybe we aren't closing out connections somewhere?
At least between the two calls to get the goroutines it's increased.

sbko · 2017-06-09T01:26:30Z

@cpuguy83 looks like it. I just reverted change to getGoroutineNumber() and tests passed. I tried requests.Get("/info") instead of SockRequests("GET", "/info,..) and it passes tests as well. I will take a look at client library.

sbko · 2017-06-09T02:42:59Z

@cpuguy83 Looks like it's client itself. defer cli.Close() solves the leak.

sbko · 2017-06-09T20:39:34Z

@thaJeztah tests are passing now

sbko · 2017-06-15T03:28:58Z

any changes to api?

value *errors.errorString = &errors.errorString{s:"\"secret create\" requires API version 1.25, but the Docker daemon API version is "} ("\"secret create\" requires API version 1.25, but the Docker daemon API version is ")

thaJeztah · 2017-06-15T13:23:02Z

I think I know what's happening; version negotiation is currently in the cli, not in the API-client, hence it doesn't know what version of the daemon it's talking to.

There's a pull request to update that; #32779

thaJeztah · 2017-06-15T13:24:59Z

But, hm.. if no version was found, it should probably assume "latest" and have the API server return the appropriate error if needed. This is the code that generates the error;

moby/client/errors.go

Lines 228 to 235 in 69c35da

    
           // NewVersionError returns an error if the APIVersion required 
        
           // if less than the current supported version 
        
           func (cli *Client) NewVersionError(APIrequired, feature string) error { 
        
           	if versions.LessThan(cli.version, APIrequired) { 
        
           		return fmt.Errorf("%q requires API version %s, but the Docker daemon API version is %s", feature, APIrequired, cli.version) 
        
           	} 
        
           	return nil 
        
           }

sbko · 2017-06-16T01:37:32Z

@thaJeztah do we need to open an issue for that?

thaJeztah · 2017-06-27T04:23:39Z

@sbko I see that #32779 was merged, perhaps it resolves the issue we're seeing here; this does need a rebase though; can you try rebasing, and see if the issue is resolved?

sbko · 2017-06-27T10:24:05Z

@thaJeztah sure. Thanks!

sbko · 2017-07-31T19:35:49Z

@vdemeester PTAL. do I need to keep rebasing or wait for reviews first?

vdemeester

SGTM 🐸
@sbko You can rebase 😉

sbko · 2017-08-01T17:18:29Z

@vdemeester done! :)

sbko · 2017-08-09T19:12:08Z

@thaJeztah what should be my next step? just wait?

thaJeztah

LGTM (sorry for the delay)

ping @dnephin can you have another look as well?

I see it needs a minor rebase

dnephin

This PR is quite large. I did another quick scan, and it generally seems fine, but I didn't really look at every change.

In the future doing multiple smaller PRs will make it much easier to review, and should reduce the time it takes to get it merged.

Couple minor comments, but nothing blocking a merge.

dnephin · 2017-08-10T15:29:54Z

minor: this if is unnecessary, just return client.NewClient(...)

@dnephin done

I still see the if here

dnephin · 2017-08-10T15:37:05Z

It might be easier to just store a client on the daemon struct here, instead of recreating it each time.

I guess that's an improvement we can make in the future.

sbko · 2017-08-15T18:14:08Z

@thaJeztah I rebased the code, added requested change and tests failed with looks like unrelated errors. I see that other PRs experience similar errors. is that a known issue? thanks!

thaJeztah · 2017-08-15T19:21:17Z

I think there was an issue with Docker Hub search causing these failures; restarting CI

sbko · 2017-08-15T20:08:42Z

@thaJeztah looks like still failing:

19:46:48 FAIL: docker_api_containers_test.go:1584: DockerSuite.TestContainerAPIDeleteWithEmptyName
19:46:48 docker_api_containers_test.go:1590:
19:46:48 c.Assert(err.Error(), checker.Contains, "No container name or ID supplied")
19:46:48 ... obtained string = "Error response from daemon: page not found"
19:46:48 ... substring string = "No container name or ID supplied"

Error should be "No container name or ID supplied" but for some reason it's "Error response from daemon: page not found". Checking why is that...

thaJeztah · 2017-08-16T14:35:02Z

@sbko yes, I chatted with the Hub team yesterday, and the cause was that the search index was out of date, causing the busybox image (which is used in that test) not being returned in the first X search results.

They triggered a re-index, and it should now (really) be fixed; triggering once more 😅

vdemeester · 2017-08-22T14:43:18Z

arf @sbko needs rebasing 😓
/cc @dnephin can you help ? 👼

sbko · 2017-08-22T14:49:43Z

@vdemeester I was busy last week. I will probably rebase tonight or tomorrow.

sbko · 2017-08-23T10:24:47Z

@vdemeester @dnephin rebased

dnephin

This might be easier to get in (faster reveiws and fewer rebases) if it were split into smaller chunks. +/- 300 lines is significantly easier to review than +/-1500.

dnephin · 2017-08-23T14:30:59Z

This looks like a bad merge. These tests were removed in #34000

dnephin · 2017-08-23T14:31:36Z

I think this should be using client.APIClient (the interface), same is true in other places.

dnephin · 2017-08-23T14:31:58Z

require.NoError(t, err)

looks like "require.NoError" doesn't work with "testingT"

dnephin · 2017-08-23T14:32:37Z

I still see the if here

Signed-off-by: Stanislav Bondarenko <[email protected]>

sbko · 2017-08-23T21:13:11Z

made changes per @dnephin review and rebased again :)
@dnephin my next pull request will be smaller ;)

GordonTheTurtle added the status/0-triage label Jun 6, 2017

vdemeester self-requested a review June 6, 2017 05:26

vdemeester added status/2-code-review and removed status/0-triage labels Jun 6, 2017

sbko force-pushed the 31410-replace-deprecated-sockrequest branch 2 times, most recently from 20e6a2a to 9718178 Compare June 6, 2017 11:01

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 9, 2017

sbko force-pushed the 31410-replace-deprecated-sockrequest branch 3 times, most recently from 5cdde3b to 341fe69 Compare June 9, 2017 17:28

sbko force-pushed the 31410-replace-deprecated-sockrequest branch 3 times, most recently from 17f8658 to 99426d6 Compare June 14, 2017 22:50

GordonTheTurtle assigned aaronlehmann Jun 21, 2017

sbko force-pushed the 31410-replace-deprecated-sockrequest branch from 99426d6 to 648c01b Compare June 27, 2017 11:13

vdemeester approved these changes Aug 1, 2017

View reviewed changes

sbko force-pushed the 31410-replace-deprecated-sockrequest branch from c58caaa to d7c1f6a Compare August 1, 2017 11:09

thaJeztah approved these changes Aug 14, 2017

View reviewed changes

dnephin reviewed Aug 14, 2017

View reviewed changes

sbko force-pushed the 31410-replace-deprecated-sockrequest branch from d7c1f6a to 5824d79 Compare August 15, 2017 11:25

thaJeztah added the rebuild/* label Aug 15, 2017

thaJeztah removed the rebuild/* label Aug 15, 2017

thaJeztah added the area/testing label Aug 16, 2017

yongtang added the rebuild/windowsRS1 label Aug 16, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Aug 16, 2017

sbko force-pushed the 31410-replace-deprecated-sockrequest branch 2 times, most recently from 9f810ec to 2332f77 Compare August 23, 2017 03:06

dnephin suggested changes Aug 23, 2017

View reviewed changes

Stop using deprecated SockRequest

0fd5a65

Signed-off-by: Stanislav Bondarenko <[email protected]>

sbko force-pushed the 31410-replace-deprecated-sockrequest branch from 2332f77 to 0fd5a65 Compare August 23, 2017 21:10

dnephin approved these changes Aug 24, 2017

View reviewed changes

dnephin merged commit eb52bb2 into moby:master Aug 24, 2017

thaJeztah mentioned this pull request Sep 4, 2023

integration/system: TestInfoAPIWarnings: improve test, and t.Parallel #46395

Draft

thaJeztah mentioned this pull request Nov 3, 2023

integration/system: update TestInfoAPI to not use string-matching #46766

Merged

Conversation

sbko commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented Jun 6, 2017

Uh oh!

sbko commented Jun 6, 2017

Uh oh!

sbko commented Jun 6, 2017

Uh oh!

thaJeztah commented Jun 9, 2017

Uh oh!

sbko commented Jun 9, 2017

Uh oh!

cpuguy83 commented Jun 9, 2017

Uh oh!

sbko commented Jun 9, 2017

Uh oh!

sbko commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbko commented Jun 9, 2017

Uh oh!

sbko commented Jun 15, 2017

Uh oh!

thaJeztah commented Jun 15, 2017

Uh oh!

thaJeztah commented Jun 15, 2017

Uh oh!

sbko commented Jun 16, 2017

Uh oh!

thaJeztah commented Jun 27, 2017

Uh oh!

sbko commented Jun 27, 2017

Uh oh!

sbko commented Jul 31, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

sbko commented Aug 1, 2017

Uh oh!

sbko commented Aug 9, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbko Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbko commented Aug 15, 2017

Uh oh!

thaJeztah commented Aug 15, 2017

Uh oh!

sbko commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 16, 2017

Uh oh!

vdemeester commented Aug 22, 2017

Uh oh!

sbko commented Aug 22, 2017

Uh oh!

sbko commented Aug 23, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

sbko commented Jun 6, 2017 •

edited

Loading

sbko commented Jun 9, 2017 •

edited

Loading

sbko Aug 15, 2017 •

edited

Loading

sbko commented Aug 15, 2017 •

edited

Loading