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

Fix IPAM doc and enhance validation #3009

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Fix IPAM doc and enhance validation #3009

merged 1 commit into from
Nov 12, 2021

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Nov 9, 2021

  • Multiple CIDRs for an IP family is not supported by NodeIPAM
  • nodeCIDRMaskSizeIPv6 defaults to 64, use a larger CIDR as example
  • AntreaIPAM is introduced in v1.4

Signed-off-by: Quan Tian qtian@vmware.com

@tnqn tnqn requested a review from jianjuns November 9, 2021 14:58
@tnqn tnqn mentioned this pull request Nov 9, 2021
Merged
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021 •
edited
Loading

Codecov Report

Merging #3009 (8df3271) into main (55b5303) will decrease coverage by 0.88%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3009      +/-   ##
==========================================
- Coverage   60.57%   59.68%   -0.89%     
==========================================
  Files         292      292              
  Lines       24708    24708              
==========================================
- Hits        14966    14748     -218     
- Misses       8104     8336     +232     
+ Partials     1638     1624      -14
Flag Coverage Δ
kind-e2e-tests 46.72% <ø> (-0.97%) ⬇️
unit-tests 40.15% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/features/antrea_features.go 16.66% <ø> (ø)
pkg/agent/util/net.go 16.32% <0.00%> (-22.45%) ⬇️
pkg/agent/route/route_linux.go 26.88% <0.00%> (-19.50%) ⬇️
pkg/agent/util/ipset/ipset.go 61.53% <0.00%> (-7.70%) ⬇️
pkg/util/k8s/client.go 46.34% <0.00%> (-4.88%) ⬇️
pkg/agent/openflow/pipeline.go 68.78% <0.00%> (-4.07%) ⬇️
pkg/agent/proxy/types/groupcounter.go 90.56% <0.00%> (-3.78%) ⬇️
pkg/agent/proxy/proxier.go 57.43% <0.00%> (-2.78%) ⬇️
pkg/ovs/openflow/ofctrl_action.go 67.91% <0.00%> (-1.67%) ⬇️
...kg/agent/flowexporter/connections/conntrack_ovs.go 76.36% <0.00%> (-1.22%) ⬇️
... and 12 more

jianjuns
jianjuns reviewed Nov 9, 2021
View reviewed changes
docs/antrea-ipam.md Outdated
@@ -32,7 +32,7 @@ controller. Default is false.

- `clusterCIDRs`: CIDR Ranges for Pods in cluster. String array containing single
CIDR range, or multiple ranges. The CIDRs could be either IPv4 or IPv6. Example
Copy link
Contributor

Choose a reason for hiding this comment

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

We should mention only one CIDR of an IP family can be used, and we should validate it in controller Options.Validate().

Copy link
Contributor

Choose a reason for hiding this comment

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

And in this case, do you think we should consider two parameters for v4 and v6 respectively? Or we are thinking about future extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.
I think the parameters are following kube-controller-manager style where a single parameter is used to take CIDRs for two IP families. And yes, it's possible to extend it to support multiple CIDRs per IP family.

docs/antrea-ipam.md Outdated
@@ -32,7 +32,7 @@ controller. Default is false.

- `clusterCIDRs`: CIDR Ranges for Pods in cluster. String array containing single
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change "Ranges" to "ranges"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@tnqn tnqn force-pushed the ipam-doc branch 3 times, most recently from 8d4de9a to 40721dc Compare November 10, 2021 15:31
@tnqn tnqn changed the title Fix IPAM related doc and comment Fix IPAM doc and enhance validation Nov 10, 2021
jianjuns
jianjuns previously approved these changes Nov 10, 2021
View reviewed changes
@tnqn
Fix IPAM doc and enhance validation
8df3271 
* Multiple CIDRs for an IP family is not supported by NodeIPAM
* nodeCIDRMaskSizeIPv6 defaults to 64, use a larger CIDR as example
* AntreaIPAM is introduced in v1.4

Signed-off-by: Quan Tian <qtian@vmware.com>
@tnqn
Copy link
Member Author

tnqn commented Nov 11, 2021

/test-all

@tnqn
Copy link
Member Author

tnqn commented Nov 12, 2021

@jianjuns could you approve again? I resolved merge conflicts

@tnqn tnqn merged commit 07b8168 into antrea-io:main Nov 12, 2021
@tnqn tnqn deleted the ipam-doc branch November 12, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianjuns jianjuns jianjuns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tnqn @codecov-commenter @jianjuns