Skip to content

ethtool: use ioctl wrapper from golang.org/x/sys/unix #17153

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
Merged
merged 3 commits into from
Sep 10, 2021
Merged

ethtool: use ioctl wrapper from golang.org/x/sys/unix #17153

merged 3 commits into from
Sep 10, 2021

Conversation

tklauser
Copy link
Member

Update the vendored copy of golang.org/x/sys/unix to pull inioctl wrapper fixes and switch pkg/datapath/ethtool to use one of these wrappers with the correctly padded ifreqData struct.

See individual commits for details.

@tklauser tklauser added the release-note/misc This PR makes changes that have no direct user impact. label Aug 12, 2021
@tklauser tklauser requested a review from a team August 12, 2021 15:22
@tklauser tklauser requested a review from a team as a code owner August 12, 2021 15:22
@tklauser tklauser requested review from jrfastab and rolinh August 12, 2021 15:22
@tklauser tklauser force-pushed the ethtool-x-sys-unix branch from 71e6fe9 to 785b340 Compare August 12, 2021 15:25
@tklauser tklauser requested a review from a team as a code owner August 12, 2021 15:25
@tklauser
Copy link
Member Author

test-me-please

ti-mo
ti-mo reviewed Aug 12, 2021
View reviewed changes
@tklauser tklauser force-pushed the ethtool-x-sys-unix branch from 785b340 to e402210 Compare August 13, 2021 15:37
@tklauser
Copy link
Member Author

Rebased to pick up fixes for EKS CI 3.0 tests.

@tklauser
Copy link
Member Author

tklauser commented Aug 13, 2021 •
edited by maintainer-s-little-helper bot
Loading

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests L2-less with Wireguard provisioned via kube-wireguarder Tests NodePort BPF

Failure Output

FAIL: Can not connect to service "http://192.168.36.11:32472" from outside cluster (1/10)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

@tklauser
Copy link
Member Author

tklauser commented Aug 16, 2021 •
edited
Loading

test-gke

previous run timed out waiting for images to be built: https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/6212/

@tklauser
Copy link
Member Author

tklauser commented Aug 17, 2021 •
edited by maintainer-s-little-helper bot
Loading

/mlh new-flake Cilium-PR-K8s-1.16-net-next

👍 created #17176

@tklauser
Copy link
Member Author

test-1.16-netnext

@tklauser tklauser force-pushed the ethtool-x-sys-unix branch from e402210 to 8e6e773 Compare August 19, 2021 11:32
@tklauser
Copy link
Member Author

tklauser commented Aug 19, 2021 •
edited
Loading

Rebased and also dropped unrelated file renames which I accidentally committed.

@tklauser
Copy link
Member Author

tklauser commented Aug 19, 2021 •
edited by maintainer-s-little-helper bot
Loading

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Checks ClusterIP Connectivity

Failure Output

FAIL: Expected

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@tklauser tklauser force-pushed the ethtool-x-sys-unix branch from 8e6e773 to 3c6255d Compare August 23, 2021 10:16
@tklauser
Copy link
Member Author

tklauser commented Aug 23, 2021 •
edited
Loading

test-me-please

Rebased to resolve merge conflicts

@tklauser tklauser force-pushed the ethtool-x-sys-unix branch from 3c6255d to 4e40ea7 Compare August 23, 2021 10:26
@tklauser
Copy link
Member Author

tklauser commented Aug 23, 2021 •
edited by maintainer-s-little-helper bot
Loading

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' hit: #17176 (93.07% similarity)

Job 'Cilium-PR-K8s-1.16-net-next' has 3 failures but they might be new flakes since it also hit 1 known flakes: #17060 (89.68)

Job 'Cilium-PR-K8s-1.16-net-next' hit: #17060 (83.06% similarity)

This was referenced Aug 23, 2021
Closed
Closed
@jrfastab
Copy link
Contributor

Is the incorrect padding causing an issue now? Meaning should this be marked as a bugfix?

@tklauser
Copy link
Member Author

Is the incorrect padding causing an issue now? Meaning should this be marked as a bugfix?

Good point. I'm currently not aware of any issues this is causing, but I guess it could potentially cause issues. These will likely be hard to track down. I'll thus mark it as bugfix and for backports.

@tklauser tklauser added kind/bug This is a bug in the Cilium logic. needs-backport/1.9 labels Aug 30, 2021
@tklauser
vendor: bump golang.org/x/sys to latest version
1cc59e0 
This pulls in a few fixes around ioctl wrappers wrt. unsafe.Pointer
usage and fixes ifreqEthtool to be correctly padded.

Ref. golang/sys@e5e7981
Ref. golang/sys@b450225

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser
datapath/linux/ethtool: use IoctlGetEthtoolDrvinfo
6e3336e 
Use the ioctl wrapper provided in the golang.org/x/sys/unix package with
the correctly padded ifreqData struct, rather than providing our own
wrapper and struct which is incorrectly padded.

Also add a simple unit test and make sure the package is only built on
Linux.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser
Copy link
Member Author

tklauser commented Sep 9, 2021 •
edited
Loading

test-me-please

Rebased to pick up CI fixes

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 9, 2021
@maintainer-s-little-helper
Copy link

Commit 8e9a627 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 Sep 10, 2021
@kaworu kaworu merged commit d82ac6f into cilium:master Sep 10, 2021
@tklauser tklauser deleted the ethtool-x-sys-unix branch September 10, 2021 19:55
@kaworu kaworu mentioned this pull request Sep 14, 2021
Merged
@kaworu kaworu mentioned this pull request Sep 14, 2021
Merged
@joestringer joestringer mentioned this pull request Oct 13, 2021
Merged
@joestringer joestringer mentioned this pull request Nov 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfastab jrfastab jrfastab approved these changes

@rolinh rolinh rolinh approved these changes

@ti-mo ti-mo ti-mo approved these changes

Assignees
No one assigned
Labels
dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tklauser @jrfastab @rolinh @ti-mo @kaworu @joestringer @aanm