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.

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

Review and update #15074

Merged
merged 4 commits into from
Jul 2, 2019
Merged

Review and update #15074

merged 4 commits into from
Jul 2, 2019

Conversation

shavidissa
Copy link
Contributor

@shavidissa shavidissa commented Jun 23, 2019

Updates the following:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 23, 2019
@shavidissa
Copy link
Contributor Author

/assign @steveperry-53
/assign @sftim
/assign @zacharysarah

@netlify
Copy link

netlify bot commented Jun 23, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 7294207

https://deploy-preview-15074--kubernetes-io-master-staging.netlify.com

@shavidissa
Copy link
Contributor Author

/assign @tengqm

@tengqm
Copy link
Contributor

tengqm commented Jun 24, 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 Jun 24, 2019
This Service will also be assigned an IP address (sometimes called the "cluster IP"),
which is used by the service proxies
This Service also assigns an IP address (sometimes called the "cluster IP"),
which is used by the Service proxies
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This specification creates a new Service named "my-service", ...

nit, could the link be cleaned up a bit:
This Service also assigns an IP address, or "cluster IP", which is used by the Virtual IPs and service proxies.

This offers a lot of flexibility for deploying and evolving your Services.
For example, you can change the port number that pods expose in the next
For example, you can change the port number the Pods are exposed in the next
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not sure about this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was fixed by a previous commit. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, 🚫 passive voice.

backend sets. For each Endpoint object, it installs iptables rules which
select a backend Pod.

By default, kube-proxy in iptables mode chooses a backend at random.

Using iptables to handle traffic has a lower system overhead, because traffic
is handled by Linux netfilter without the need switch between userspace and the
is handled by Linux netfilter. It removes the need to switch between userspace and the
kernel space. This approach is also likely to be more reliable.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the it, iptables or Linux netfilter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true.. That's confusing.. am reverting back to the old sentence and did a small nit there.

When kube-proxy starts in IPVS proxy mode, it will verify whether IPVS
kernel modules are available, and if those are not detected then kube-proxy
When kube-proxy starts in IPVS proxy mode, it verifies whether IPVS
kernel modules are available, and if those are not detected, then kube-proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fall vs. falls
nit: could the sentence be broken up?

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

Hi @kbhawkey and team,

Will you be able to help lgtm and approve the PR if the content is good to go, please? :)

@kbhawkey
Copy link
Contributor

kbhawkey commented Jul 2, 2019

@kbhawkey
Copy link
Contributor

kbhawkey commented Jul 2, 2019

hello @shavidissa. I read through the changes. Seems fine. Nice cleanup.
I think this concept page, though, could use some updates in a future PR, such as:

  • reduce the size of the diagrams and fix the overflow of the text in the diagrams
  • create a separate page or address the AWS specific content?
  • reword the Motivation section -- the description of The Life of a Pod gives me a chuckle. A paragraph about What is a Service seems more appropriate.
  • Are there more links that could be added to Future work?

@zacharysarah
Copy link
Contributor

/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 Jul 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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 Jul 2, 2019
@zacharysarah
Copy link
Contributor

@kbhawkey 👋 I'm giving the LGTM/approve here to keep things moving, but your feedback here is great and I don't want it to get lost.

@k8s-ci-robot k8s-ci-robot merged commit 78229a1 into kubernetes:master Jul 2, 2019
joaovitor pushed a commit to joaovitor/website that referenced this pull request Jul 5, 2019
* Review and update

* Fix issue 3735

* Update as per the comments

* Update as per the comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants