Skip to content

Bump containerd client #36684

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

Merged
merged 2 commits into from
Apr 19, 2018
Merged

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Mar 23, 2018

  • Bumps containerd client
  • Uses containerd client's new Reconnect() API to reconnect cached clients instead of replacing them (leaving stale clients in other cached objects).

@@ -114,6 +114,13 @@ type client struct {
containers map[string]*container
}

func (c *client) reconnect() error {
Copy link
Member Author

Choose a reason for hiding this comment

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

We probably need to catch connection failures in normal requests and trigger a reconnect, but we aren't handling these cases at all (or ever AFAIK).

Copy link
Contributor

Choose a reason for hiding this comment

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

Those requests would just fail until the connection monitor reconnects normally.

@thaJeztah
Copy link
Member

Will this change be part of the upcoming containerd 1.1.0 release? Wondering if we can get back to released versions of the dependency (or at least something on a release branch)

@cpuguy83
Copy link
Member Author

I think so, a release branch has not be cut yet.

@cpuguy83
Copy link
Member Author

The new client API is not really "new" anymore since it's been a couple of weeks, btw.

@thaJeztah
Copy link
Member

Alright, so probably safe to assume it’ll be in 1.1.0, and once that’s released, we could switch to a tagged version, thanks!

@codecov
Copy link

codecov bot commented Mar 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@b6a7d02). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #36684   +/-   ##
=========================================
  Coverage          ?   35.33%           
=========================================
  Files             ?      614           
  Lines             ?    46345           
  Branches          ?        0           
=========================================
  Hits              ?    16375           
  Misses            ?    27797           
  Partials          ?     2173

@cpuguy83 cpuguy83 force-pushed the bump_containerd_client branch from 5554c31 to f7a5424 Compare March 27, 2018 20:50
This does not bump the containerd binary.
Picks last commit before go1.10 switch, which is not currently supported
in moby.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This fixes an issue where the containerd client is cached in a container
object in libcontainerd and becomes stale after containerd is restarted.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@crosbymichael
Copy link
Contributor

LGTM

Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

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.

6 participants