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

util/iptables: fix cross-build failures due to syscall.Flock() #45601

Merged
merged 1 commit into from
May 15, 2017
Merged

util/iptables: fix cross-build failures due to syscall.Flock() #45601

merged 1 commit into from
May 15, 2017

Conversation

dcbw
Copy link
Member

@dcbw dcbw commented May 10, 2017

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 10, 2017
@dcbw dcbw force-pushed the fix-iptables-crossbuild branch 2 times, most recently from 3524eb8 to 62d42ce Compare May 10, 2017 17:11
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 10, 2017
@dcbw dcbw force-pushed the fix-iptables-crossbuild branch from 62d42ce to 7d0b518 Compare May 10, 2017 18:02
@vishh vishh added priority/P0 release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 10, 2017
@vishh
Copy link
Contributor

vishh commented May 10, 2017

@dcbw can you run hack/udpate-bazel.sh again?

@vishh
Copy link
Contributor

vishh commented May 10, 2017

@dcbw this is blocking other PRs, can we get this through today?

@vishh vishh self-assigned this May 10, 2017
@dcbw
Copy link
Member Author

dcbw commented May 10, 2017 •
edited

@vishh yeah, working on this; it's repushed with a bazel fix 30m ago.

hack/update-bazel.sh doesn't work because it doesn't correctly handle GOPATH like all the rest of the hack/ stuff does (eg, as a subpath of build/). IIRC I tried to make it do that a couple months ago, but we can't because it does really odd things with looking at package imports. So anyway, I'm stuck making the Jenkins stuff do the bazel verification.

@dcbw
Copy link
Member Author

dcbw commented May 10, 2017

Test Failures
k8s.io/kubernetes/vendor/k8s.io/kube-aggregator/pkg/controllers/autoregister TestCheckAPIService 12s

seems to be a flake?

@dcbw
Copy link
Member Author

dcbw commented May 10, 2017

@k8s-bot unit test this issue #43781

@luxas
Copy link
Member

luxas commented May 10, 2017 •
edited

@deads2k known flake? ^ (issue has been pending for a long time)

@luxas
Copy link
Member

luxas commented May 10, 2017 •
edited

@dcbw I've always updated bazel like this:

docker run -it -v $(pwd):/go/src/k8s.io/kubernetes -w /go/src/k8s.io/kubernetes golang:1.7 /bin/bash -c "hack/update-bazel.sh"

has worked well for me for a long time

@dcbw
Copy link
Member Author

dcbw commented May 10, 2017

@luxas thanks, that works! I'll use that from now on.

@dcbw
Copy link
Member Author

dcbw commented May 10, 2017

@luxas @vishh either of you know how to trigger a cross-build so we can make sure this fixes the issue?

@dcbw
Copy link
Member Author

dcbw commented May 10, 2017

@k8s-bot pull-kubernetes-cross test this

@k82cn
Copy link
Member

k82cn commented May 10, 2017 •
edited

@dcbw , golang.org/x/sys/unix does not work in windows.

@dcbw
Copy link
Member Author

dcbw commented May 11, 2017

@k82cn yeah, I thought that by the "use golang.org/x/syscall" the suggestion was to just use that. I'd originally done a split out with build +linux, and I'll just do both things.

@dcbw dcbw force-pushed the fix-iptables-crossbuild branch from 7d0b518 to f8e1d28 Compare May 11, 2017 18:28
@k8s-github-robot k8s-github-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 11, 2017
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 11, 2017
@dcbw
Copy link
Member Author

dcbw commented May 11, 2017

@k8s-bot pull-kubernetes-cross test this

@dcbw
Copy link
Member Author

dcbw commented May 11, 2017

@k82cn @luxas PTAL, thanks!

vishh
vishh reviewed May 11, 2017
View reviewed changes
pkg/util/iptables/iptables_linux.go
@@ -0,0 +1,93 @@
// +build linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the build tags necessary even with explicit naming?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://golang.org/pkg/go/build/#hdr-Build_Constraints

@vishh
Copy link
Contributor

vishh commented May 11, 2017

@dcbw a minor nit. Otherwise this PR is ready to be merged

@k82cn
Copy link
Member

k82cn commented May 12, 2017

@dcbw , the platform part is OK to me; delegate lock/iptables part to other reviewers.

@luxas luxas self-assigned this May 12, 2017
luxas
luxas approved these changes May 12, 2017
View reviewed changes
Copy link
Member

@luxas luxas 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 for fixing!
There are some nits but they are easily fixable.

And you found the right cross-build command already, perfect!

pkg/util/iptables/iptables_linux.go Outdated
// +build linux

/*
Copyright 2014 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Now 2017 ;)

@rmmh rmmh mentioned this pull request May 13, 2017
Closed
@dcbw dcbw force-pushed the fix-iptables-crossbuild branch from f8e1d28 to a4624a0 Compare May 15, 2017 04:38
@dcbw
Copy link
Member Author

dcbw commented May 15, 2017

@luxas fixed the dates, PTAL thanks!

luxas
luxas approved these changes May 15, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

LGTM, no harm done although there are both _linux.go and build linux

@luxas
Copy link
Member

luxas commented May 15, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, luxas

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 15, 2017 •
edited

@dcbw: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce a4624a0 link @k8s-bot pull-kubernetes-federation-e2e-gce test this

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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 885ddcc into kubernetes:master May 15, 2017
@danwinship danwinship mentioned this pull request May 15, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishh vishh vishh left review comments

@luxas luxas luxas approved these changes

Assignees

@luxas luxas

@thockin thockin

@vishh vishh

@eparis eparis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@dcbw @k8s-reviewable @vishh @luxas @k82cn @k8s-github-robot @k8s-ci-robot @thockin @eparis