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

Cherrypick to v0.37 - update docker client method #2734

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

bobbypage
Copy link
Collaborator

@bobbypage bobbypage commented Nov 18, 2020

@google-cla
Copy link

google-cla bot commented Nov 18, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@bobbypage
Copy link
Collaborator Author

@JornShen it looks like there's some issues cherrypicking #2714 to v0.37:

From CI:

[/home/runner/work/cadvisor/cadvisor/container/docker/client.go:58:12: WithAPIVersionNegotiation not declared by package client -: could not load export data: no export data for \"github.com/google/cadvisor/container/docker\"]"

I think the problem is WithAPIVersionNegotiation was added in (moby/moby@b26aa97) looks like but cAdvisor v0.37 is using an older Docker version with hash be7ac8be2ae0 [1] that doesn't have that method: https://github.com/moby/moby/blob/be7ac8be2ae072032a4005e8f232be3fc57e4127/client/options.go

[1] From v0.37 go.mod:

github.com/docker/docker v0.7.3-0.20190327010347-be7ac8be2ae0

@bobbypage
Copy link
Collaborator Author

bobbypage commented Nov 18, 2020

@JornShen is there some way we can fix #2714 with the current version of docker on the v0.37 branch? I think we should avoid bumping docker version on an existing cherrypick version.

@bobbypage
Copy link
Collaborator Author

/cc @ruiwen-zhao

@JornShen
Copy link
Contributor

JornShen commented Nov 18, 2020

@JornShen is there some way we can fix #2714 with the current version of docker on the v0.37 branch? I think we should avoid bumping docker version on an existing cherrypick version.

It's related to docker client version. Is there problem for us to update the docker version here?
If have, I think we can also only update cadvisor dependency of k/k to v0.38.X in k8s inrelease-1.19 .
Is cadvisor version strong binding to k8s version? like release-1.19 must use v0.37.X?

@JornShen
Copy link
Contributor

@dims is there some method for us to use which is same effect as WithAPIVersionNegotiation in old docker version?

@dims
Copy link
Collaborator

dims commented Nov 18, 2020

@JornShen when you use WithAPIVersionNegotiation the newer client can talk to older servers.

@liggitt
Copy link
Contributor

liggitt commented Nov 18, 2020

was the late-initializing fix verified? I'm not seeing where setup() is retried. xref #2732 (comment)

Nevermind, detectDevices was working, initializeNVML was not. Sorry for the noise.

@JornShen
Copy link
Contributor

@JornShen when you use WithAPIVersionNegotiation the newer client can talk to older servers.

@dims I meam, are there other method which is same effect as WithAPIVersionNegotiation in old docker client?
@bobbypage dont want to bump the docker version.In v0.37 version, cadvisor use old docker client v0.7.3-0.20190327010347-be7ac8be2ae0 which dont have method WithAPIVersionNegotiation.

@bobbypage
Copy link
Collaborator Author

bobbypage commented Nov 19, 2020

@JornShen I looked at this again, I think it should be ok to update docker version.

My concern is that when we go to vendor the new cAdvisor patch release back into k/k we want to avoid bumping the docker version since cherrypick should ideally be as small as possible.

However, k8s 1.19 seems to be on newer docker already compared to cAdvisor v0.37:

From k8s 1.19 go.mod:

github.com/docker/docker v1.4.2-0.20200309214505-aa6a9891b09c

https://github.com/kubernetes/kubernetes/blob/release-1.19/go.mod#L39

The PR that made that update was kubernetes/kubernetes#89687. That commit there matches https://github.com/moby/moby/tree/v19.03.8

However cAdvisor v0.37 is using:

github.com/docker/docker v0.7.3-0.20190327010347-be7ac8be2ae0

https://github.com/google/cadvisor/blob/release-v0.37/go.mod#L17

Since k8s 1.19 is already using newer docker version (that also includes WithAPIVersionNegotiation) I think it should be fine to bump docker version in cAdvisor v0.37 to match that of k8s 1.19.

bobbypage and others added 2 commits November 19, 2020 04:07
go get -u github.com/docker/docker@v1.4.2-0.20200309214505-aa6a9891b09c
update dockerclient.NewClient to dockerclient.NewClientWithOpts
and enable client negotiate down to a lower version server version
@google-cla
Copy link

google-cla bot commented Nov 19, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 19, 2020
@bobbypage bobbypage changed the title v0.37 cherrypicks Cherrypick to v0.37 - update docker client method Nov 19, 2020
@bobbypage bobbypage merged commit 0d3dfd7 into google:release-v0.37 Nov 19, 2020
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

4 participants