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

Drop pkg/util/ipcmd, port to vishvananda/netlink (mostly) #15834

Merged
merged 2 commits into from
Sep 18, 2017
Merged

Drop pkg/util/ipcmd, port to vishvananda/netlink (mostly) #15834

merged 2 commits into from
Sep 18, 2017

Conversation

danwinship
Copy link
Contributor

This drops our wrapper around /sbin/ip and ports the remaining code to use the netlink library instead.

Except for the one place we were using ipcmd in pod_linux because netlink doesn't yet have a LinkSetTxQLen. I've submitted a PR for that (vishvananda/netlink#257) but until then, I just changed the code to call exec directly.

@openshift-merge-robot openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 17, 2017
@danwinship
Copy link
Contributor Author

/assign dcbw

@danwinship
Copy link
Contributor Author

/test extended_networking

@bparees
Copy link
Contributor

bparees commented Aug 17, 2017

/unassign

@danwinship danwinship mentioned this pull request Aug 17, 2017
Merged
@danwinship
Copy link
Contributor Author

changes break the cross build; see #15838 for the fix.

openshift-merge-robot added a commit that referenced this pull request Aug 26, 2017
@openshift-merge-robot
Merge pull request #15838 from danwinship/sdn-linux-only
be52b45 
Automatic merge from submit-queue (batch tested with PRs 15904, 15962, 15838, 15965, 15963)

Only build pkg/sdn/plugin on linux

We are currently building (most of) the SDN code on Darwin and Windows, even though it is totally linux-specific.

I ran into this when trying to make more use of the "netlink" library in the sdn code (#15834). The library only builds on linux, so any file that imports it has to also be only built on linux.

So this tries to fix the main openshift binary to only include the SDN code when building on Linux. I don't know if it's correct stylistically. I'm happy to rewrite it however.

@openshift/networking FYI
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2017
@danwinship
Copy link
Contributor Author

extended_conformance_gce flake is #15969, cross build failure was probably real but the logs are already gone...
/retest

@danwinship
Copy link
Contributor Author

/test cross

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2017
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 31, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017
@danwinship danwinship assigned eparis and unassigned smarterclayton Sep 2, 2017
@danwinship
Copy link
Contributor Author

@eparis can you approve this? (Needs someone to approve the changes to pkg/util, which consist solely of removing pkg/util/ipcmd.)

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 6, 2017
@danwinship
Copy link
Contributor Author

The netlink PR merged, so I updated this to bump the netlink import and use netlink.SetTxQLen().

The netlink bump was pretty huge (we previously had a version from 2015), but it's small (a few months) relative to the version currently in kubernetes. And the tests pass...

@dcbw
Copy link
Contributor

dcbw commented Sep 8, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2017
@eparis
Copy link
Member

eparis commented Sep 11, 2017

/approve
/hold
Please run the /hold cancel monday 18th after dev cut ends. Bug fixes only right now.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw, eparis

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2017
@danwinship
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2017
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@danwinship
Copy link
Contributor Author

/test extended_networking_minimal
COPR flake

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 18, 2017 •
edited
Loading

@danwinship: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_gce ca7be16 link /test extended_conformance_gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions 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.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15834, 16321, 16353, 15298, 15433)

@openshift-merge-robot openshift-merge-robot merged commit 89e7c5d into openshift:master Sep 18, 2017
@danwinship danwinship deleted the drop-util-ipcmd branch September 24, 2017 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eparis eparis

@dcbw dcbw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/networking lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@danwinship @bparees @dcbw @eparis @openshift-merge-robot @openshift-ci-robot @smarterclayton