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

[release/1.2] bump cgroups dependency to address blkio issue #4001

Merged
merged 1 commit into from Feb 17, 2020

Conversation

lbernail
Copy link

@lbernail lbernail commented Feb 5, 2020

Issue #3412 also impacts 1.2, which we are using extensively

This PR bumps the cgroups dependency to the same commit used in 1.3 (it includes more changes but it is much easier this way).
I did some testing on our clusters and I confirm that with this change the issue preventing the metrics from working disappear.

I may be missing other impacts, so of course let me know if this is the best approach.

@lbernail
Copy link
Author

lbernail commented Feb 5, 2020

Not sure why the CI failed, it does not seem related to the change (but I may be missing something)

@jterry75
Copy link
Contributor

jterry75 commented Feb 5, 2020

You just need to sign your commit and force push

@thaJeztah
Copy link
Member

I think it fails because the commit is missing a DCO sign-off

Looking at these changes; perhaps instead we could cherry-pick the commits from master (so backporting #3357 and #3423)

@thaJeztah
Copy link
Member

assuming your "upstream" remote (this repository) is named upstream, and the remote for your fork is named origin;

git reset --hard upstream/release/1.2
git cherry-pick -s -x 341c7c144f64d2a621160ba2dc667d67668879ea a1f3ebaec888b5bb816fa5b3437eb3ff7d81b415
git push -f origin

@jterry75
Copy link
Contributor

jterry75 commented Feb 5, 2020

@thaJeztah - @lbernail just needs to sign this commit for this PR

Signed-off-by: Laurent Bernaille <laurent.bernaille@datadoghq.com>
@lbernail
Copy link
Author

lbernail commented Feb 5, 2020

I just signed-off and pushed

If you think it's preferable to cherry-pick the commits instead of updating the vendors I'll do that of course (the end result should be the same because #3423 also updates cgroups to c4b9ac5, right?)

@codecov-io
Copy link

Codecov Report

Merging #4001 into release/1.2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/1.2    #4001   +/-   ##
============================================
  Coverage        44.19%   44.19%           
============================================
  Files              100      100           
  Lines            10847    10847           
============================================
  Hits              4794     4794           
  Misses            5313     5313           
  Partials           740      740
Flag Coverage Δ
#linux 47.87% <ø> (ø) ⬆️
#windows 41% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35bd7a5...25a806c. Read the comment docs.

@jterry75
Copy link
Contributor

jterry75 commented Feb 6, 2020

I'm not sure it matters a ton. @dmcgowan - Do you have a preference that this be a cherry pick from mainline?

@thaJeztah
Copy link
Member

Whoops, didn't catch up;

If you think it's preferable to cherry-pick the commits instead of updating the vendors I'll do that of course (the end result should be the same because #3423 also updates cgroups to c4b9ac5, right?)

Yes, the end result would the same; it's "metadata" that would differ; I usually try to cherry-pick to keep references to the original commit (the -x option helps there); in some cases one may be interested if a specific commit found its way in other branches, or the original commit / PR has a discussion related to it that can be relevant.

Not a blocker probably

@AkihiroSuda AkihiroSuda changed the title bump cgroups dependency to address blkio issue [release/1.2] bump cgroups dependency to address blkio issue Feb 7, 2020
@lbernail
Copy link
Author

lbernail commented Feb 7, 2020

I completely understand how reusing the master commits helps with readability
I tried the cherry-pick but it's not straightforward because we get conflicts with unrelated vendor dependencies (go-runc and console).
Probably easily fixable but it will not be as clean as just getting the 2 commits

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

verified that this update is already in the release/1.3 branch (a1f3eba), so no need to backport for 1.3

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 5bead45 into containerd:release/1.2 Feb 17, 2020
@lbernail lbernail deleted the lbernail/upgrade-cgroups branch February 17, 2020 16:27
thaJeztah added a commit to thaJeztah/containerd that referenced this pull request Feb 18, 2020
The thirteenth patch release for `containerd` 1.2 fixes a regression introduced
in v1.2.12 that caused container/shim to hang on single core machines, fixes an
issue with blkio, and updates the Golang runtime to 1.12.17.

Notable Updates
----------------------------------

* Fix container pid race condition [containerd#4025](containerd#4025)
* Update containerd/cgroups dependency to address blkio issue [containerd#4001](containerd#4001)
* Set octet-stream content-type on PUT request [containerd#4028](containerd#4028)
* Pin to libseccomp 2.3.3 to preserve compatibility with hosts that do not have libseccomp 2.4 or higher installed [containerd#4015](containerd#4015)
* Update Golang runtime to 1.12.17, which includes a fix to the runtime [containerd#4031](containerd#4031)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants