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

Cleanup networking docs #9349

Merged
merged 2 commits into from
Jun 15, 2020
Merged

Cleanup networking docs #9349

merged 2 commits into from
Jun 15, 2020

Conversation

ari-becker
Copy link
Contributor

As discussed on Slack.

Signed-off-by: Ari Becker [email protected]

@ari-becker
Cleanup networking docs
1905d23 
Signed-off-by: Ari Becker <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 12, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @ari-becker. 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 Jun 12, 2020
@ari-becker
Copy link
Contributor Author

cc @olemarkus

@olemarkus
Copy link
Member

Thanks for this! Really like these changes.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2020
@ari-becker
Copy link
Contributor Author

/assign @mikesplain

^ as asked by the bot. FYI this PR is the result of a discussion on Slack revolving around confusion over the "External" CNI language

@olemarkus
Copy link
Member

/assign @mikesplain

@hakman
Copy link
Member

hakman commented Jun 12, 2020

/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 Jun 12, 2020
@ari-becker
Copy link
Contributor Author

/retest

@ari-becker
Copy link
Contributor Author

Any ideas why the PR is failing? It's a docs-only PR...

@rifelpet
Copy link
Member

there was an AWS outage that impacted e2e tests, it should be good now
/retest

johngmyers
johngmyers reviewed Jun 12, 2020
View reviewed changes
docs/networking.md
* `kubenet` Kubernetes native networking via a CNI plugin. This is the default.
* `cni` Container Network Interface(CNI) style networking, often installed via a Daemonset.
* `external` networking is done via a Daemonset. This is used in some custom implementations.
We do recommended something other than `kubenet` for production clusters due to `kubenet`'s limitations, as explained [below][#kubenet-default].
Copy link
Member

Choose a reason for hiding this comment

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

I do believe the distinction between kubenet-based providers and CNI-based providers is important to document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johngmyers I felt I did? Kubenet is still in an entirely separate section from CNI and carries appropriate warnings.

rifelpet
rifelpet reviewed Jun 15, 2020
View reviewed changes
docs/networking.md Outdated Show resolved Hide resolved
@ari-becker @rifelpet
Remove URL from text
d36492d 
Co-authored-by: Peter Rifel <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2020
@rifelpet
Copy link
Member

looks good, thanks!
/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ari-becker, rifelpet

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2020
@k8s-ci-robot k8s-ci-robot merged commit e487a69 into kubernetes:master Jun 15, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 15, 2020
@ari-becker ari-becker deleted the networking-docs branch June 15, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers left review comments

@rifelpet rifelpet rifelpet left review comments

@mikesplain mikesplain Awaiting requested review from mikesplain

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

Assignees

@olemarkus olemarkus

@mikesplain mikesplain

@rifelpet rifelpet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation 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
v1.19
Development

Successfully merging this pull request may close these issues.
7 participants
@ari-becker @k8s-ci-robot @olemarkus @hakman @rifelpet @johngmyers @mikesplain