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

Add network best practice document #429

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add network best practice document #429

wants to merge 3 commits into from

Conversation

yaocw2020
Copy link
Contributor

No description provided.

@harvesterhci-io-github-bot
Copy link
Collaborator

harvesterhci-io-github-bot commented Sep 11, 2023 •
edited
Loading

✅ Deploy Preview for harvester-preview ready!

Name Link
🔨 Latest commit 901f960
🔍 Latest deploy log https://app.netlify.com/sites/harvester-preview/deploys/65423bca793f4229f58d2513
😎 Deploy Preview https://65423bca793f4229f58d2513--harvester-preview.netlify.app

vickyhella
vickyhella reviewed Sep 12, 2023
View reviewed changes
Copy link
Contributor

@vickyhella vickyhella left a comment

Choose a reason for hiding this comment

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

Only some nits.

docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
vickyhella
vickyhella approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@vickyhella vickyhella left a comment

Choose a reason for hiding this comment

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

LGTM!

LucasSaintarbor
LucasSaintarbor approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@LucasSaintarbor LucasSaintarbor left a comment

Choose a reason for hiding this comment

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

Small NITs. LGTM, thanks!

docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/best-practice.md Outdated Show resolved Hide resolved
docs/networking/deep-dive.md Outdated Show resolved Hide resolved
@ibrokethecloud
Copy link
Contributor

the best practice only covers what is needed from the routing/switching end. Though this is a good reference we should ideally leave it up to the end users.

would it make sense to also have more info on how to configure cluster networks for different interfaces, and then created the VM networks?

Some sample VM Network configs would be nice as well, specially for untagged traffic as that seems to be the most common question from community.

@yaocw2020
Copy link
Contributor Author

the best practice only covers what is needed from the routing/switching end. Though this is a good reference we should ideally leave it up to the end users.

would it make sense to also have more info on how to configure cluster networks for different interfaces, and then created the VM networks?

Some sample VM Network configs would be nice as well, specially for untagged traffic as that seems to be the most common question from community.

Good suggestion. I would add some reference cases about cluster networks and VM networks.

w13915984028
w13915984028 reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

It is great to have such documents. Besides the current wonderful commit, also some suggestions:

There are a few traditional network setting scenarios in Harvester:
(1) All flat network, no isolation (home lab /trial)
(2) All vlan network, even for management, dedicated nics (production deployment)
(3) Mixed un-tagged and tagged network; only 1 nic (home lab /trial/poc...)
(4) Mixed un-tagged and tagged network; multi nics (poc ...)

This doc covers (4); we may also list (1)~(3) and update them now or future. thanks.

ibrokethecloud
ibrokethecloud approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@ibrokethecloud ibrokethecloud left a comment

Choose a reason for hiding this comment

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

LGTM. thanks.

w13915984028
w13915984028 approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@bk201
Copy link
Member

bk201 commented Oct 31, 2023

@w13915984028 Can you help finalize this PR by creating pages in https://github.com/harvester/docs/tree/main/versioned_docs/version-v1.2 as well? I guess at the writing time, the v1.2 was not released yet.

@ibrokethecloud
Copy link
Contributor

This is not part of this PR but in deep-dive.md, line 110, there is a typo, where mode is spelt as mdoe

any chance we can please fix this too?

yaocw2020 and others added 2 commits November 1, 2023 12:20
@yaocw2020 @vickyhella @w13915984028
Add network best practice document
519f105 
Signed-off-by: yaocw2020 <[email protected]>
Co-authored-by: vickyhella <[email protected]>
@yaocw2020 @LucasSaintarbor @w13915984028
Apply suggestions from code review
4797cdc 
Co-authored-by: Lucas Saintarbor <[email protected]>
@w13915984028
Copy link
Member

@bk201 This PR is ready for final merging now.

Thanks again for @yaocw2020 's great effort.

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

@guangbochen guangbochen guangbochen approved these changes

@ibrokethecloud ibrokethecloud ibrokethecloud approved these changes

@w13915984028 w13915984028 w13915984028 approved these changes

@vickyhella vickyhella vickyhella approved these changes

@LucasSaintarbor LucasSaintarbor LucasSaintarbor approved these changes

Assignees

@w13915984028 w13915984028

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yaocw2020 @harvesterhci-io-github-bot @ibrokethecloud @bk201 @w13915984028 @guangbochen @vickyhella @LucasSaintarbor