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

Support for using hostPort when using flannel #7295

Merged
merged 1 commit into from
Sep 3, 2019
Merged

Support for using hostPort when using flannel #7295

merged 1 commit into from
Sep 3, 2019

Conversation

shamil
Copy link
Contributor

@shamil shamil commented Jul 21, 2019 •
edited
Loading

Similar to #3206 but for flannel, related to #3132

In this PR I updated the flannel's cni config file to enable portmap plugin and changed its extension from .conf to .conflist in order to support multiple plugins.

P.S.
I haven't changed pre 1.6 manifests, to avoid breaking old clusters that doesn't ship portmap plugin

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @shamil. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 21, 2019
@shamil
Copy link
Contributor Author

shamil commented Jul 21, 2019

/assign @justinsb
/assign @geojaz

@mikesplain
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2019
@shamil
Copy link
Contributor Author

shamil commented Jul 22, 2019

@mikesplain one check says Pod pending timeout, should we retest?

@shamil
Copy link
Contributor Author

shamil commented Jul 23, 2019

/retest

@shamil
Copy link
Contributor Author

shamil commented Jul 23, 2019

@justinsb @geojaz @mikesplain can you please review and see if this can be merged? All tests passed as it seems.
Thanks

@shamil
Copy link
Contributor Author

shamil commented Jul 29, 2019

Kindly, can this PR get some attention please.
/auto-cc

geojaz
geojaz previously requested changes Aug 21, 2019
View reviewed changes
Copy link
Member

@geojaz geojaz left a comment

Choose a reason for hiding this comment

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

@shamil Sorry we're so slow to respond on this. This looks fine to me, but would turning on hostPort mode change the default behavior of the flannel cni addon? If so, I would much rather expose it in a way that makes this optional so that users' experience is more predictable. That said, I don't have enough context on flannel to say, but I'll listen to what you think :)

An aside that's completely out of scope for you and this PR is why are we still trying to maintain pre-1.6 compatibility? I'll take a look at the issues and if none exist, will backlog my thoughts.

@shamil
Copy link
Contributor Author

shamil commented Aug 24, 2019 •
edited
Loading

@shamil Sorry we're so slow to respond on this. This looks fine to me, but would turning on hostPort mode change the default behavior of the flannel cni addon? If so, I would much rather expose it in a way that makes this optional so that users' experience is more predictable. That said, I don't have enough context on flannel to say, but I'll listen to what you think :)

Before this change hostPort with flannel didn't do anything, it had no affect at all when configured in the k8s manifests.

For all the users that were using flannel without hostPort plugin enabled will not see any difference after this change. The change is only for those that do want to use hostPort with flannel. This PR is very similar to #3206

We have been running with this change in our production for more than a month already, no issues discovered so far. We didn't need to change our existing resources unless we want explicitly enable hostPort

@geojaz
Copy link
Member

geojaz commented Aug 24, 2019 •
edited
Loading

@shamil- thanks for explaining and helping me get up to speed on this. Lgtm!
Lgtm
/Approve

@geojaz geojaz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2019
@geojaz geojaz dismissed their stale review August 24, 2019 11:47

Obsolete

@geojaz geojaz added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geojaz, shamil

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

@shamil
Copy link
Contributor Author

shamil commented Aug 24, 2019

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2019
@shamil
Copy link
Contributor Author

shamil commented Aug 24, 2019

@geojaz seems like pull-kops-e2e-kubernetes-aws Job failing, I couldn't find anything related there, do you know who can help me troubleshoot this?

Also I did rebase from latest master, seems like the PR needs another approval....

@shamil
Copy link
Contributor Author

shamil commented Aug 29, 2019

/retest

@shamil
Copy link
Contributor Author

shamil commented Aug 29, 2019

after another retest the remaining test (pull-kops-e2e-kubernetes-aws) passed as well,
@geojaz, can I get another lgtm please :)

@geojaz
Copy link
Member

geojaz commented Sep 3, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2019
@k8s-ci-robot k8s-ci-robot merged commit a225054 into kubernetes:master Sep 3, 2019
@andsens andsens mentioned this pull request Aug 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geojaz geojaz geojaz left review comments

@justinsb justinsb Awaiting requested review from justinsb

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

@granular-ryanbonham granular-ryanbonham Awaiting requested review from granular-ryanbonham

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@justinsb justinsb

@geojaz geojaz

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.
5 participants
@shamil @k8s-ci-robot @mikesplain @geojaz @justinsb