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
Closed

runtime: TestLockOSThreadExit failing on arm64 builder #29366

ALTree opened this issue Dec 20, 2018 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@ALTree
Copy link
Member

ALTree 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 Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 20, 2018
@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
Copy link
Contributor

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
Copy link
Contributor

@gopherbot
Copy link

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

@mknyszek
Copy link
Contributor

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
Copy link
Member

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
Copy link

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
…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>
@golang golang locked and limited conversation to collaborators Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

5 participants