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

runtime: TestLockOSThreadExit failing on arm64 builder #29366

Closed
ALTree opened this issue Dec 20, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@ALTree
Copy link
Member

commented Dec 20, 2018

The TestLockOSThreadExit is currently failing on the linux/arm64 builder:

--- FAIL: TestLockOSThreadExit (0.03s)
    crash_test.go:95: testprog LockOSThreadAvoidsStatePropagation exit status: exit status 1
    proc_test.go:892: want OK
        , got failed to unshare fs: operation not permitted
        
FAIL
FAIL	runtime	40.468s

https://build.golang.org/log/ba97c98ea93fe2e4b4619e38fda345eb43216f97

The failures started at d0f8a75 (runtime: don't clear lockedExt on locked M when G exits), which slightly changed the test.

cc @mknyszek

@ALTree ALTree added this to the Go1.12 milestone Dec 20, 2018

@ALTree ALTree changed the title runtime: TestLockOSThreadExit failing on arm64 runtime: TestLockOSThreadExit failing on arm64 builder Dec 20, 2018

@mknyszek

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

It looks like on linux/arm64 for whatever reason unshare(CLONE_FS) is either unsupported, or just not allowed to be executed by the user running the tests.

If I were to take a wild guess, it might have to do with the way the arm64 builders are containerized. If there's a security issue with enabling unshare, then I'm happy to just disable the test on arm64. It's an easy fix.

cc @bradfitz

@bradfitz

This comment has been minimized.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 20, 2018

Change https://golang.org/cl/155437 mentions this issue: runtime: skip TestLockOSThreadAvoidsStatePropagation on linux/arm64

@mknyszek

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

In that case, it's somewhat unclear to me what the correct fix is.

Thinking about this more, I'm not sure I like disabling the test for arm64, since it's not a property of linux/arm64 that the test fails. If the test is running in any environment where certain syscalls are unavailable then there's not much one can do.

Ideally I'd like to just have the test fail gracefully if it doesn't have permissions to execute a certain syscall. However, this is more dangerous and confusing since if all the builders stop letting the tests call unshare, then we wouldn't notice, and the test could be passing even if the functionality is broken.

So, I decided to put up a change to just disable the test. Let me know if this seems reasonable. I poked around and it seems like some of the other tests are turned off for different platforms for builder reasons, so I figured it was OK.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

I think it would be good that we skip the test on platforms that doesn't support, maybe by testing kernel configurations or trying a syscall. So users won't see this kind of failures if they have such configurations.

For the concern of actually testing the functionality, I guess we could require the tests not to be skipped on certain builders? I.e., something like

if unsupported {
  if onCertainBuilder {
    t.Fatalf(...) // the syscall should be supported on this builder
  } else {
    t.Skip(...)
  }
}
@gopherbot

This comment has been minimized.

Copy link

commented Mar 14, 2019

Change https://golang.org/cl/167707 mentions this issue: [release-branch.go1.11] runtime: skip TestLockOSThreadAvoidsStatePropagation if one can't unshare

gopherbot pushed a commit that referenced this issue Mar 14, 2019

[release-branch.go1.11] runtime: skip TestLockOSThreadAvoidsStateProp…
…agation if one can't unshare

This change splits a testprog out of TestLockOSThreadExit and makes it
its own test. Then, this change makes the testprog exit prematurely with
a special message if unshare fails with EPERM because not all of the
builders allow the user to call the unshare syscall.

Also, do some minor cleanup on the TestLockOSThread* tests.

Fixes #29366.

Change-Id: Id8a9f6c4b16e26af92ed2916b90b0249ba226dbe
Reviewed-on: https://go-review.googlesource.com/c/155437
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 429bae7)
Reviewed-on: https://go-review.googlesource.com/c/go/+/167707
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.