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.

Sign up for GitHub

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

Jump to bottom

Replace almost all uses of 'syscall' with 'unix' pkg. #10158

Merged
merged 1 commit into from Mar 2, 2020
Merged

Replace almost all uses of 'syscall' with 'unix' pkg. #10158

merged 1 commit into from Mar 2, 2020

Conversation

Ropes
Copy link
Contributor

@Ropes Ropes commented Feb 12, 2020 •
edited by tgraf

golang.org/x/sys/unix package is replacement for syscall which has
been deprecated since Go 1.4.

Deprecated: this package is locked down. Callers should use the corresponding package in the golang.org/x/sys repository instead. That is also where updates required by new systems or versions should be applied. See https://golang.org/s/go1.4-syscall for more information.

While the changes do touch a lot of files, all of the modifications are
simply replacing imports of the syscall package, with x/sys/unix
implementations. No functionality changes, so no new tests added.

There is one case of syscall remaining in the cilium dnsproxy. There
is currently no good replacement listening for UDP traffic.

Fixes: #10116

Signed-off-by: Joshua Roppo joshroppo@gmail.com

This change is Reviewable

@Ropes Ropes requested a review from a team February 12, 2020 07:59
@Ropes Ropes requested a review from a team as a code owner February 12, 2020 07:59
@Ropes Ropes requested a review from a team February 12, 2020 07:59
@Ropes Ropes requested review from a team as code owners February 12, 2020 07:59
@Ropes Ropes requested a review from a team February 12, 2020 07:59
@Ropes Ropes requested review from a team as code owners February 12, 2020 07:59
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 12, 2020
houndci-bot
houndci-bot reviewed Feb 12, 2020
View reviewed changes
pkg/bpf/map_linux.go Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Feb 12, 2020
View reviewed changes
pkg/bpf/map_linux.go Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Feb 12, 2020
View reviewed changes
pkg/bpf/map_linux.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 12, 2020 •
edited

Coverage Status

Coverage decreased (-0.02%) to 45.541% when pulling 8477c78 on Ropes:kind/cleanup/ropes/replace/syscall/w/unix/pkgs into ae35f0d on cilium:master.

@aanm aanm added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 12, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@aanm
Copy link
Member

aanm commented Feb 12, 2020

test-me-please

@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Feb 12, 2020
@Ropes
Copy link
Contributor Author

Ropes commented Feb 13, 2020

Alright, I've been able to verify the test failure locally. Also checked the prior commit to make sure that there wasn't some upstream instability which sneaked through.

Very curious what has actually been uncovered here. I'll start investigating syscall vs unix package constants, perhaps there is a difference between values with the same name? Seems very peculiar if that were the case, but the change surface is rather small.

@joestringer
Copy link
Member

@Ropes As far as I can tell, there's only one failure which looks like the known CI flake #9869 :

https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/17085/testReport/junit/Suite-k8s-1/17/K8sPolicyTest_Basic_Test_TLS_policy/

So if the rest of the build (including the full testsuite with the k8s-1.11+net-next+kubeproxy-free) passes then this should be good enough from a testing perspective. Just need reviews from the codeowners.

brb
brb approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@brb brb left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, thanks!

There will be a potential conflict once #10117 gets merged.

rolinh
rolinh approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

LGTM. And there might be yet another conflict with #10058 once it's merged.

@Ropes
Copy link
Contributor Author

Ropes commented Feb 16, 2020

Since there aren't really any logic changes in this PR, it'll be easier to refactor post #10058 & #10117 get merged. When they merge I'll have a second pass and get it ready again. #10117 is similar to what this changeset.. just didn't pick up using unix package.

@aanm aanm added the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Feb 24, 2020
@Ropes Ropes force-pushed the kind/cleanup/ropes/replace/syscall/w/unix/pkgs branch from 0b79807 to a26f8b4 Compare February 24, 2020 23:50
@Ropes
Copy link
Contributor Author

Ropes commented Feb 24, 2020

Just force pushed a fix for a code conflict with master.

tklauser
tklauser requested changes Feb 25, 2020
View reviewed changes
api/v1/health/server/server.go Outdated Show resolved Hide resolved
cilium-health/responder/main.go Outdated Show resolved Hide resolved
cilium/cmd/cleanup.go Outdated Show resolved Hide resolved
daemon/cleanup.go Show resolved Hide resolved
operator/main.go Outdated Show resolved Hide resolved
pkg/bpf/bpffs_linux.go Show resolved Hide resolved
pkg/cgroups/cgroups.go Show resolved Hide resolved
pkg/datapath/prefilter/prefilter.go Show resolved Hide resolved
pkg/monitor/agent/listener/listener.go Show resolved Hide resolved
pkg/probe/probe.go Show resolved Hide resolved
@aanm aanm removed the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Feb 25, 2020
@Ropes Ropes mentioned this pull request Feb 25, 2020
Merged
@Ropes Ropes force-pushed the kind/cleanup/ropes/replace/syscall/w/unix/pkgs branch 2 times, most recently from 21568df to 2a1406d Compare February 26, 2020 01:46
@aanm aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 27, 2020
@Ropes Ropes force-pushed the kind/cleanup/ropes/replace/syscall/w/unix/pkgs branch from 2a1406d to cfb06cd Compare February 29, 2020 00:22
joestringer
joestringer requested changes Feb 29, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Just a few remaining unexpected changes, otherwise LGTM.

pkg/envoy/accesslog_server.go Outdated Show resolved Hide resolved
pkg/maps/policymap/policymap_privileged_test.go Outdated Show resolved Hide resolved
proxylib/test/accesslog_server.go Outdated Show resolved Hide resolved
@Ropes
Replaced uses of 'syscall' with 'unix' pkg.
8477c78 
'golang.org/x/sys/unix' package is replacement for 'syscall' which has
been deprecated since Go 1.4.

While the changes do touch a lot of files, all of the modifications are
simply replacing imports of the `syscall` package, with `x/sys/unix`
implementations.

The only functionality which technically changed is in
bpf_linux.go; where unix.BytePtrFromString() now returns an error value.
It is a minor logic change, syscall.StringBytePtr() does not return
error, just bad data value which would have errored later in function.

Swagger API server.gotmpl was updated, and udpated API code committed.

There is one case of `syscall` remaining in the cilium dnsproxy. There
is currently [no good
replacement](cilium#10116 (comment))
listening for UDP traffic. So left as is.

Fixes: cilium#10116

Signed-off-by: Joshua Roppo <joshroppo@gmail.com>
@Ropes Ropes force-pushed the kind/cleanup/ropes/replace/syscall/w/unix/pkgs branch from cfb06cd to 8477c78 Compare February 29, 2020 00:44
@joestringer
Copy link
Member

test-me-please

@aanm aanm requested a review from tklauser March 2, 2020 09:20
tklauser
tklauser approved these changes Mar 2, 2020
View reviewed changes
Copy link
Member

@tklauser tklauser 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.

@maintainer-s-little-helper
Copy link

Commit fc0aedf2642238dc1a6cbfc44d335c3ba6d0a33f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 2, 2020
@aanm aanm force-pushed the kind/cleanup/ropes/replace/syscall/w/unix/pkgs branch from fc0aedf to 8477c78 Compare March 2, 2020 09:30
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 2, 2020
@aanm aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 2, 2020
@aanm aanm merged commit f7547a0 into cilium:master Mar 2, 2020
1.8.0 automation moved this from In progress to Merged Mar 2, 2020
tklauser added a commit that referenced this pull request Apr 8, 2020
@tklauser
api: regenerate health serverAPI code to replace syscall with x/sys/unix
54e311c 
This was missed in commit f7547a0 ("Replaced uses of 'syscall' with
'unix' pkg."), PR #10158.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser mentioned this pull request Apr 8, 2020
Merged
tgraf pushed a commit that referenced this pull request Apr 8, 2020
@tklauser @tgraf
api: regenerate health serverAPI code to replace syscall with x/sys/unix
8311399 
This was missed in commit f7547a0 ("Replaced uses of 'syscall' with
'unix' pkg."), PR #10158.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@jrajahalme jrajahalme jrajahalme left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@brb brb brb approved these changes

@tklauser tklauser tklauser approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
1.8
Development

Successfully merging this pull request may close these issues.

Remove references to syscall package
9 participants
@Ropes @coveralls @aanm @joestringer @brb @tklauser @rolinh @jrajahalme @houndci-bot