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

Bug 1720332: images/kube-proxy: update to 4.1, add iptables wrappers #23235

Merged
merged 2 commits into from Jul 17, 2019
Merged

Bug 1720332: images/kube-proxy: update to 4.1, add iptables wrappers #23235

merged 2 commits into from Jul 17, 2019

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Jun 21, 2019 •
edited

This backports the image change in #23163.

This image is currently unused, but is needed by third-parties. So soak time will have no effect.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 21, 2019
@openshift-ci-robot
Copy link

@squeed: This pull request references a valid Bugzilla bug.

In response to this:

Bug 1720332: images/kube-proxy: update to 4.1, add iptables wrappers

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.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 21, 2019
@openshift-ci-robot
Copy link

@squeed: This pull request references a valid Bugzilla bug.

In response to this:

Bug 1720332: images/kube-proxy: update to 4.1, add iptables wrappers

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.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019
@dcbw
Copy link
Member

dcbw commented Jun 21, 2019

/lgtm

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

eparis commented Jun 24, 2019

I don't see a 4.2 bugzilla which was VERIFIED that this was fixed in 4.2.

I also don't see how this solves $whatever the problem is. It just changes where the iptables binaries come from, right? Can I get a better explaination of the problem and how it is fixed in the bugzilla(s)?

@squeed
Copy link
Contributor Author

squeed commented Jun 25, 2019

/hold

@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 Jun 25, 2019
@squeed
Copy link
Contributor Author

squeed commented Jun 25, 2019 •
edited

Turns out this broke the ART build (but not OSBS, go figure) in 4.2 - need #23250 to merge.

@eparis This is a bit of a funny thing - we are building this image exclusively for third-party network vendors who don't use openshift-sdn, so that they don't have to deploy an out-of-tree kube-proxy binary. Otherwise, it's not deployed. So, QE isn't really doing any validation.

We can't use the existing iptables binaries because of the RHEL7+8 problem: they use very different netfilter mechanisms, and there's no consistent kernel API. The only branching point we can use is which binary is symlinked on the host. This is what we do in production for openshift-sdn in 4.1, so this is bringing kube-proxy in-line with what is already deployed and tested.

@squeed
Copy link
Contributor Author

squeed commented Jun 25, 2019

FWIW, the PR to have the operator install standalone kube-proxy for 4.2 is openshift/cluster-network-operator#201 and is QE'd in SDN-247. We probably won't backport the operator change, but still shoud to release the 4.1 image for partners to do development

@eparis
Copy link
Member

eparis commented Jun 25, 2019

we'll re-review next week. but this is going to miss 4.1.4. the need for 23250 scares me enough not to destabilize 4.1.4 :)

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 26, 2019
@squeed
Copy link
Contributor Author

squeed commented Jun 26, 2019

OK, cherry-picked the fixes so that it builds in ART too, not just OSBS.

@pecameron, please review + lgtm
@eparis let me know if this still scares you. It's not deployed by default, so it shouldn't be too scary ;-)

@squeed
Copy link
Contributor Author

squeed commented Jun 26, 2019

/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 Jun 26, 2019
@squeed squeed mentioned this pull request Jun 26, 2019
Closed
pecameron
pecameron approved these changes Jun 26, 2019
View reviewed changes
Copy link
Contributor

@pecameron pecameron left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019
@jwforres
Copy link
Member

jwforres commented Jul 2, 2019

@knobunc I would like group lead sign-off on this one before approving a cherry-pick given it sounds like there is no QE involvement with validating this

@knobunc
Copy link
Contributor

knobunc commented Jul 3, 2019

This is needed for third-party SDN plugins. It is not used by the OpenShift SDN, so it can not destabilize the core product. But this change fixes a problem we found with the SDN image, and it will be a problem for third-party images, and thus should be back-ported. Thanks!

/approve
/lgtm

@squeed
Copy link
Contributor Author

squeed commented Jul 3, 2019

FYI, after this merges, we need to notify Adam Haile to tweak some settings in the ART pipeline.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2019
@squeed
Copy link
Contributor Author

squeed commented Jul 11, 2019

@crawford updated both commits.

@knobunc
Copy link
Contributor

knobunc commented Jul 12, 2019

/retest
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, knobunc, pecameron, squeed

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

18 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@derekwaynecarr
Copy link
Member

@knobunc @squeed thanks for the updates.

approved.

@derekwaynecarr derekwaynecarr added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 16, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 45ef5c8 into openshift:release-4.1 Jul 17, 2019
@openshift-ci-robot
Copy link

@squeed: All pull requests linked via external trackers have merged. The Bugzilla bug has been moved to the MODIFIED state.

In response to this:

Bug 1720332: images/kube-proxy: update to 4.1, add iptables wrappers

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pecameron pecameron pecameron approved these changes

@dcbw dcbw Awaiting requested review from dcbw

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@knobunc knobunc

@dcbw dcbw

@pecameron pecameron

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet