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

update docker client method #2714

Merged
merged 1 commit into from
Nov 9, 2020
Merged

Conversation

JornShen
Copy link
Contributor

@JornShen JornShen commented Nov 8, 2020

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

dockerclient.NewClient has been deprecated. And change to recommended public methods dockerclient.NewClientWithOpts.

using WithAPIVersionNegotiation() makes the Docker client negotiate down to a lower version if Docker's current API version is newer than the server version.

ref: kubelet: pod metrics not visible with Docker 18 (19.03 works)

After kubernetes/kubernetes#89687 merged(docker client dependency update to newer), We find that kubelet failed to register cadvisor docker factory when we use k8s v1.19 with docker 18.X. And info as fellow:

[factory.go:161] Registration of the docker container factory failed: failed to validate Docker info: failed to detect Docker info: Error response from daemon: client version 1.40 is too new. Maximum supported API version is 1.39

The error Registration of the docker container factory is from

for name, plugin := range plugins {
watcher, err := plugin.Register(factory, fsInfo, includedMetrics)
if err != nil {
klog.V(5).Infof("Registration of the %s container factory failed: %v", name, err)

the error failed to validate is from

dockerInfo, err := client.Info(defaultContext())
if err != nil {
return nil, fmt.Errorf("failed to detect Docker info: %v", err)
}

we find in advisor, docker client is created by Client(). And it seems no negotiating to old server version if we use deprecated NewClient with higher docker client version.

dockerClient, dockerClientErr = dclient.NewClient(*ArgDockerEndpoint,
"",
client,
nil)

what does this PR do:
update dockerclient.NewClient to dockerclient.NewClientWithOpts
and enable client negotiate down to a lower version server version.

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 8, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Nov 8, 2020
@k8s-ci-robot
Copy link
Collaborator

Hi @JornShen. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@JornShen
Copy link
Contributor Author

JornShen commented Nov 8, 2020

cc @dims

Copy link
Collaborator

@dims dims left a comment

Choose a reason for hiding this comment

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

cc @bobbypage

LGTM!

@JornShen
Copy link
Contributor Author

JornShen commented Nov 9, 2020

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 9, 2020
@bobbypage
Copy link
Collaborator

Thanks for the fix, LGTM!

@bobbypage
Copy link
Collaborator

/ok-to-test

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

Successfully merging this pull request may close these issues.

4 participants