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

Clarify cluster diagram as one possible reference architecture #47164

Merged

Conversation

robert-cronin
Copy link
Contributor

@robert-cronin robert-cronin commented Jul 15, 2024

This PR addresses the concerns raised in issue #47111 regarding the potential misunderstanding of component distribution in Kubernetes clusters, particularly the presence of kubelet and kube-proxy on control plane nodes.

image

Before
After

I'm not entirely sure how much additional detail is appropriate for this introductory page on cluster architecture. I'm open to removing certain details for brevity if needed. Feedback on the level of detail and which points are most crucial to keep would be greatly appreciated. Also if there is anything I have gotten wrong about cluster architecture, please feel free to correct me.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 15, 2024
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jul 15, 2024
Copy link

netlify bot commented Jul 15, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 02c9e9b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/66ba8cbd2c5bdc00086be5d5
😎 Deploy Preview https://deploy-preview-47164--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

i think this is a great addition. thanks
/lgtm

but not sure if the formatting / indentation is correct.

/assign @tengqm
as per your comment on
#47111 (comment)

/sig architecture

@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Jul 15, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 15436ba45b96f9a92facd68bf7d93143c71de60b

@robert-cronin
Copy link
Contributor Author

i think this is a great addition. thanks /lgtm

but not sure if the formatting / indentation is correct.

/assign @tengqm as per your comment on #47111 (comment)

/sig architecture

Thank you for the swift review, @neolit123! I appreciate your feedback on the formatting and indentation. If you're aware of any similar pages with correct formatting, or if you have any specific suggestions for improvement, I'd be grateful for your guidance.

@tamilselvan1102
Copy link
Contributor

page preview: https://deploy-preview-47164--kubernetes-io-main-staging.netlify.app/docs/concepts/architecture/

@neolit123
Copy link
Member

would leave the formatting correctness to the docs team.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks

I think if we merged this we'd immediately want to file an issue calling for more changes. You can revise this PR so that it can merge without needing that issue.

Here's some specific feedback.

content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2024
robert-cronin and others added 2 commits July 19, 2024 22:05
Co-authored-by: Tim Bannister <tim@scalefactory.com>
Co-authored-by: Tim Bannister <tim@scalefactory.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 22, 2024
@robert-cronin
Copy link
Contributor Author

robert-cronin commented Jul 22, 2024

The latest commit includes the following changes:

  • Migrated the text from /docs/concepts/overview/components to /docs/concepts/architecture
  • Added a section to /docs/concepts/architecture on the "Architecture Variations"
  • Added simple overview to /docs/concepts/overview/components

I'd love any feedback on the overall organization of the content or suggestions on how I can improve the content I added

/docs/concepts/architecture/
Before
After

/docs/concepts/overview/components/
Before
After

@robert-cronin robert-cronin force-pushed the clarify-reference-architecture branch from 5deae5d to a8edb2d Compare July 22, 2024 09:49
@robert-cronin
Copy link
Contributor Author

Latest commit adds some small points of optimization to make the pages mesh more coherently and make the overview more of an overview.

@network-charles
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

@network-charles: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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-sigs/prow repository.

@GarbageYard
Copy link

The proposed change mentions following under Node Components section: "These components are essential for node operation but are not always considered part of the core control plane". I have one query: in which case are node components (kubelet and kube-proxy) not considered part of the core control plane?

@sftim
Copy link
Contributor

sftim commented Aug 10, 2024

I have one query: in which case are node components (kubelet and kube-proxy) not considered part of the core control plane?

Always. The control plane does not include the kubelet and kube-proxy; see https://kubernetes.io/docs/concepts/overview/components/

You might run the control plane on a computer (or several) computers that are running kubelet, and possibly also running kube-proxy; however, the control plane is still:

  • kube-apiserver
    • and a backing store, canonically etcd
  • kube-scheduler
  • kube-controller-manager
  • (optional) a cloud controller manager to integrate with your cloud provider

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

Here's some feedback to make this even better.

content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
content/en/docs/concepts/architecture/_index.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 60b2326103ed6daed7b3bf5b978c2346b78f68d2

Improves clarity and consistency with existing documentation

Co-authored-by: Tim Bannister <tim@scalefactory.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2024
@robert-cronin
Copy link
Contributor Author

robert-cronin commented Aug 11, 2024

@sftim I've added a follow up commit to fix some remaining issues and improve consistency. I am a little confused what the third-party notice is targeting so perhaps my placement is worth double checking. Thanks for the feedback!

@GarbageYard
Copy link

@sftim : That's much clearer. Thanks.

However, if that's the case then shouldn't the revised doc mention:
"These components are essential for node operation but are never considered part of the core control plane"
instead of
"These components are essential for node operation but are not always considered part of the core control plane"?

@sftim
Copy link
Contributor

sftim commented Aug 12, 2024

“never“ feels correct to me @GarbageYard

You could think of an electrical generating station. Maybe there is a generator set specifically for the control room (a good idea for resilience), and that generator may be able to supply the plant or even the power network.
However, you wouldn't say that generators are sometimes considered part of the control room.

In that analogy, you could be more specific about why there is a generator in a strange place on the plant diagram. In a Kubernetes cluster, if you run the control plane on nodes, you can explain why some servers are nodes and have both a kubelet and control plane components on them.

Co-authored-by: Tim Bannister <tim@scalefactory.com>
@sftim
Copy link
Contributor

sftim commented Aug 13, 2024

/lgtm
/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 13, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d2242616513ea8c876c871323493fa1268bd86aa

Copy link
Contributor

@nate-double-u nate-double-u left a comment

Choose a reason for hiding this comment

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

This is a really good addition @robert-cronin, thanks so much for writing it up!
(and thanks to all the reviewers too)

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nate-double-u

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 Aug 24, 2024
@k8s-ci-robot k8s-ci-robot merged commit af0e08c into kubernetes:main Aug 24, 2024
6 checks passed
@robert-cronin
Copy link
Contributor Author

This is a really good addition @robert-cronin, thanks so much for writing it up!
(and thanks to all the reviewers too)

/approve

Thank you so much, @nate-double-u, and thanks to all the commenters and reviewers! 😊

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/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants