-
Notifications
You must be signed in to change notification settings - Fork 364
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
[doc][flexible-ipam] Update document for Multiple-VLAN support #3507
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3507 +/- ##
===========================================
- Coverage 65.44% 38.25% -27.19%
===========================================
Files 277 114 -163
Lines 27500 15208 -12292
===========================================
- Hits 17996 5818 -12178
- Misses 7585 8909 +1324
+ Partials 1919 481 -1438
Flags with carried forward coverage won't be shown. Click here to find out more. |
docs/antrea-ipam.md
Outdated
subnets. | ||
All traffic to a Pod in different VLAN will be sent to the gateway of "underlay" network. | ||
Inter-Node traffic will be sent to the "underlay" network from the source Node, and | ||
forwarded to the destination Node by the "underlay" network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think quotes are not needed with underlay
as those are not used in other places (like below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, removed quotes.
docs/antrea-ipam.md
Outdated
All traffic to a local Pod in same VLAN will be sent to the Pod's OVS port directly, | ||
after the destination MAC is rewritten to the Pod's MAC address. This includes | ||
`AntreaIPAM` Pods and regular `Subnet per Node` IPAM Pods, even they are not in the same | ||
subnets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: even when they are not in the same subnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed.
docs/antrea-ipam.md
Outdated
after the destination MAC is rewritten to the Pod's MAC address. This includes | ||
`AntreaIPAM` Pods and regular `Subnet per Node` IPAM Pods, even when they are not in the | ||
same subnet. | ||
All traffic to a Pod in different VLAN will be sent to the gateway of underlay network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to start a new paragraph for it? If yes, an empty line is needed before it to indicate a new paragraph in markdown. So do the following two cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is to make the raw text clear, not to start a new paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it is even confusing. If you want not a new paragraph for readers, why start a new in "raw text"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianjuns Already changed to one line according your suggestion.
If we start a new line for every sentence, we don't need to correct the layout for the whole paragraph when we add/remove some words before this sentence in this paragraph. Thus why I used this method, but I'm OK to remove the newline for every sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many IDE/editor can wrap lines automatically. A line for a sentence doesn't look tidy and makes "paragraph" meaningless, and it's not a convention in practice in any project I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it.
docs/antrea-ipam.md
Outdated
Inter-Node traffic will be sent to the Node network from the source Node, and forwarded | ||
to the destination Node by the Node network. | ||
Traffic from `AntreaIPAM` Pods without VLAN, regular `Subnet per Node` IPAM Pods, and K8s | ||
Nodes are recognized as VLAN 0 (untagged). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are -> is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks.
docs/antrea-ipam.md
Outdated
Traffic from `AntreaIPAM` Pods without VLAN, regular `Subnet per Node` IPAM Pods, and K8s | ||
Nodes are recognized as VLAN 0 (untagged). | ||
|
||
All traffic to a local Pod in same VLAN will be sent to the Pod's OVS port directly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All traffic -> Traffic
same VLAN -> the Pod's VLAN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Nodes are recognized as VLAN 0 (untagged). | ||
|
||
All traffic to a local Pod in same VLAN will be sent to the Pod's OVS port directly, | ||
after the destination MAC is rewritten to the Pod's MAC address. This includes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we always override destination MAC, or only for cross-subnet case? If the latter, probably remove this sentence. If you want can add it to the last sentence:
This includes
AntreaIPAM
Pods and regularSubnet per Node
IPAM Pods. If the source and destination are not in the same subnet, the packets' destination MAC will be overridden to the destination Pod's MAC".
same subnet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. DstMAC will be rewritten always for same vlan
docs/antrea-ipam.md
Outdated
after the destination MAC is rewritten to the Pod's MAC address. This includes | ||
`AntreaIPAM` Pods and regular `Subnet per Node` IPAM Pods, even when they are not in the | ||
same subnet. | ||
All traffic to a Pod in different VLAN will be sent to the gateway of underlay network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All traffic -> Traffic
We do not explicitly send the traffic to gateway, so maybe:
will be sent to the underlay network, where the underlay router will route the traffic to the destination VLAN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/antrea-ipam.md
Outdated
`AntreaIPAM` Pods and regular `Subnet per Node` IPAM Pods, even when they are not in the | ||
same subnet. | ||
All traffic to a Pod in different VLAN will be sent to the gateway of underlay network. | ||
Inter-Node traffic will be sent to the underlay network from the source Node, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this sentence is not necessary. But up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Removed this.
docs/antrea-ipam.md
Outdated
pools for IPv4 and IPv6 respectively will be supported. | ||
The IPs in the `IPPools` without VLAN must be in the same "underlay" subnet as the Node | ||
IP, because inter-Node traffic of AntreaIPAM Pods is forwarded by the Node network. | ||
The IPs in the `IPPools` with VLAN must not overlap with other network subnets, and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove "The IPs in the"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/antrea-ipam.md
Outdated
The IPs in the `IPPools` without VLAN must be in the same "underlay" subnet as the Node | ||
IP, because inter-Node traffic of AntreaIPAM Pods is forwarded by the Node network. | ||
The IPs in the `IPPools` with VLAN must not overlap with other network subnets, and the | ||
gateway of underlay network should provide the network connectivity for these VLANs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe say "the underlay network router" to be more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
docs/antrea-ipam.md
Outdated
IP, because inter-Node traffic of AntreaIPAM Pods is forwarded by the Node network. | ||
The IPs in the `IPPools` with VLAN must not overlap with other network subnets, and the | ||
gateway of underlay network should provide the network connectivity for these VLANs. | ||
Only a single IP pool can be included in the Namespace annotation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to keep lines aligned at 80th column, rather than creating a new line for each sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Fixed.
docs/antrea-ipam.md
Outdated
inter-Node traffic of AntreaIPAM Pods is forwarded by the Node network. Only a single IP | ||
pool can be included in the Namespace annotation. In the future, annotation of up to two | ||
pools for IPv4 and IPv6 respectively will be supported. | ||
The IPs in the `IPPools` without VLAN must be in the same "underlay" subnet as the Node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - we can remove "" around "underlay" now, as underlay is talked a lot in this doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed.
Signed-off-by: gran <[email protected]>
/skip-all |
Signed-off-by: gran [email protected]