Switch from package syscall to golang.org/x/sys/unix#49300
Switch from package syscall to golang.org/x/sys/unix#49300k8s-github-robot merged 3 commits intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @tklauser. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-github-robot
added
size/XXL
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
There was a problem hiding this comment.
May I ask what's the go version required for this?
There was a problem hiding this comment.
I have only tested this with go 1.8.3 but I'd guess this should work with every version, as the underlying type of both syscall.Signal and unix.Signal is int.
|
The federation-e2e-gce test failure seems unrelated to the PR as indicated by the following log messages in https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/49300/pull-kubernetes-federation-e2e-gce/16070/?log#log: |
|
Should these files be guarded with build tags? |
|
@cmluciano AFAICT all files touched by this PR - except |
|
/test pull-kubernetes-cross will tell us whether this works on all platforms |
The syscall package is locked down and the comment in [1] advises to switch code to use the corresponding package from golang.org/x/sys. Do so and replace usage of package syscall with package golang.org/x/sys/unix where applicable. [1] https://github.com/golang/go/blob/master/src/syscall/syscall.go#L21-L24 This will also allow to get updates and fixes for syscall wrappers without having to use a new go version. Errno, Signal and SysProcAttr aren't changed as they haven't been implemented in /x/sys/. Stat_t from syscall is used if standard library packages (e.g. os) require it. syscall.SIGTERM is used for cross-platform files.
Use unix.Eventfd() instead of C.eventfd and also use the correct corresponding unix.EFD_CLOEXEC flag. This allows to get rid of cgo.
tklauser
force-pushed
the
syscall-to-x-sys-unix
branch
from
c38054f to
5acfb16
Compare
Apparently pkg/kubectl/rollback.go is also needed for Windows, so |
|
kubemark-e2e-gce failure seems to be the same as #49320 |
|
/retest |
|
Test the cross-build again: For the pull-kubernetes-e2e-gce-etcd3 flake: |
|
This needs an associated issue and a /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thockin, tklauser Associated issue: 33399 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
@k8s-bot test this Tests are more than 96 hours old. Re-running tests. |
|
/retest |
|
Seems like the results for pull-kubernetes-cross and @k8s-ci-robot |
|
/test pull-kubernetes-cross |
|
There still seems to be some issue communicating back the status of pull-kubernetes-kubemark-e2-gce :( /retest |
|
I believe the hung status was a prow/plank bug that was fixed earlier this week /test pull-kubernetes-kubemark-e2e-gce |
|
Thank you @fejta! |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue (batch tested with PRs 51666, 49829, 51058, 51004, 50938)
Fix threshold notifier build tags
**What this PR does / why we need it**:
Cross building from darwin is currently broken on the following error:
```
# k8s.io/kubernetes/pkg/kubelet/eviction
pkg/kubelet/eviction/threshold_notifier_unsupported.go:25: NewMemCGThresholdNotifier redeclared in this block
previous declaration at pkg/kubelet/eviction/threshold_notifier_linux.go:38
```
It looks like #49300 broke the build tags introduced in #38630 and #37384. This fixes the build tag on `threshold_notifier_unsupported.go` as the cgo requirement was removed from `threshold_notifier_linux.go`.
**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #50935
**Special notes for your reviewer**:
**Release note**:
```release-note
NONE
```
9 participants
What this PR does / why we need it:
The syscall package is locked down and the comment in https://github.com/golang/go/blob/master/src/syscall/syscall.go#L21-L24 advises to switch code to use the corresponding package from golang.org/x/sys. This PR does so and replaces usage of package syscall with package golang.org/x/sys/unix where applicable. This will also allow to get updates and fixes
without having to use a new go version.
In order to get the latest functionality, golang.org/x/sys/ is re-vendored. This also allows to use Eventfd() from this package instead of calling the eventfd() C function.
Special notes for your reviewer:
This follows previous works in other Go projects, see e.g. moby/moby#33399, cilium/cilium#588
Release note: