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 backport] Pin to libseccomp 2.3.3 #4015

Merged
merged 2 commits into from Feb 17, 2020

Conversation

hakman
Copy link
Contributor

@hakman hakman commented Feb 12, 2020

Besides the fact that lib seccomp 2.4 has huge performance regressions, it also breaks support for older distros like Debian 9 and RHEL/CentOS 7, as discussed in #4008.
This change pins to 2.3.3 where that is not an issue.

Fixes #4008.

(cherry picked from commits b5f03ea and 75d0c5f)
Signed-off-by: Ciprian Hacman ciprian.hacman@sematext.com

Copy link
Member

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

LGTM

@hakman
Copy link
Contributor Author

hakman commented Feb 12, 2020

@crosbymichael @thaJeztah seems that something is missing though:

Summarizing 1 Failure:
7110
7111[Fail] [k8s.io] Container runtime should support basic operations on container [It] runtime should support listing container stats [Conformance] 
7112/home/travis/gopath/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/container.go:569
7113
7114Ran 71 of 78 Specs in 49.854 seconds
7115FAIL! -- 70 Passed | 1 Failed | 0 Pending | 7 Skipped
7116
7117
7118Ginkgo ran 1 suite in 49.906109744s
7119Test Suite Failed
7120--- FAIL: TestCRISuite (49.91s)
7121    cri_test.go:130: Failed to run tests in parallel: exit status 1
7122FAIL

@estesp
Copy link
Member

estesp commented Feb 13, 2020

I'm at a loss for why there are CRI test failures based on this PR. I don't see that the same things happened in master when libseccomp was pinned. Any ideas @Random-Liu?

@hakman hakman closed this Feb 13, 2020
@hakman hakman reopened this Feb 13, 2020
@hakman hakman closed this Feb 13, 2020
@hakman hakman reopened this Feb 13, 2020
@hakman hakman closed this Feb 13, 2020
@hakman hakman reopened this Feb 13, 2020
@hakman
Copy link
Contributor Author

hakman commented Feb 13, 2020

Looks like it's not a flake. It consistently fails on:
[Fail] [k8s.io] Container runtime should support basic operations on container [It] runtime should support listing container stats [Conformance]


#
# Builds and installs runc to /usr/local/go/bin based off
# the commit defined in vendor.conf
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like the right comment.

Copy link
Member

Choose a reason for hiding this comment

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

That's a mistake in master from the original commit. I just opened a PR to fix.

Copy link
Member

Choose a reason for hiding this comment

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

Ah on my phone didn't notice this was a backport.

Copy link
Member

Choose a reason for hiding this comment

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

Ah on my phone didn't notice this was a backport.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the comment also. Thanks :)

@hakman
Copy link
Contributor Author

hakman commented Feb 14, 2020

@estesp I see the same test failure here: #4024.

crosbymichael and others added 2 commits February 14, 2020 12:58
lib seccomp 2.4 has huge performance regressions.
This change pins to 2.3.3 where that is not an issue

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
(cherry picked from commit b5f03ea)
Signed-off-by: Ciprian Hacman <ciprian.hacman@sematext.com>
Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>
(cherry picked from commit 75d0c5f)
Signed-off-by: Ciprian Hacman <ciprian.hacman@sematext.com>
@thaJeztah
Copy link
Member

Just to confirm; this holds back the version of libseccomp at compile time, so that the binary produced doesn't use the new seccomp_api_set feature, which is needed to allow compiling a binary to be compatible with both systems having libseccomp 2.3 and systems running libseccomp 2.4, correct?

@hakman
Copy link
Contributor Author

hakman commented Feb 14, 2020

@thaJeztah that is correct.

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

thanks; I know there's been some confusion about this in the past 😅

@hakman
Copy link
Contributor Author

hakman commented Feb 14, 2020

No problem. Thanks for looking into it. Any thoughts on when a new release could be expected?

@thaJeztah
Copy link
Member

No idea yes, but likely soon, to address #4023

@hakman
Copy link
Contributor Author

hakman commented Feb 14, 2020

Sounds good. Looking forward to see these issues fixed.

@hakman hakman closed this Feb 17, 2020
@hakman hakman reopened this Feb 17, 2020
@hakman hakman closed this Feb 17, 2020
@hakman hakman reopened this Feb 17, 2020
@codecov-io
Copy link

Codecov Report

Merging #4015 into release/1.2 will increase coverage by 3.18%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/1.2    #4015      +/-   ##
===============================================
+ Coverage           41%   44.19%   +3.18%     
===============================================
  Files               70      100      +30     
  Lines             9537    10847    +1310     
===============================================
+ Hits              3911     4794     +883     
- Misses            5061     5313     +252     
- Partials           565      740     +175
Flag Coverage Δ
#linux 47.87% <ø> (?)
#windows 41% <ø> (ø) ⬆️
Impacted Files Coverage Δ
mount/temp_unix.go 0% <0%> (ø)
sys/reaper_linux.go 0% <0%> (ø)
services/server/server_linux.go 0% <0%> (ø)
sys/env.go 100% <0%> (ø)
sys/stat_unix.go 0% <0%> (ø)
runtime/v2/shim/shim_unix.go 0% <0%> (ø)
sys/reaper.go 0% <0%> (ø)
sys/fds.go 0% <0%> (ø)
sys/proc.go 0% <0%> (ø)
sys/mount_linux.go 0% <0%> (ø)
... and 26 more

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 c2383a5...a7c9b76. Read the comment docs.

@hakman
Copy link
Contributor Author

hakman commented Feb 17, 2020

All tests are passing now, after kubernetes-sigs/cri-tools#574 was merged.

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 01edb7c into containerd:release/1.2 Feb 17, 2020
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>
@hakman hakman deleted the fix-libseccomp-ver branch June 12, 2020 06:04
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

7 participants