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

Configure the level of IPAM with UDNs #1708

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phoracek
Copy link
Member

@phoracek phoracek commented Oct 25, 2024

The current proposal for UDN's IPAM configuration is limiting as it would not allow a (simple) expansion covering other anticipated use-cases. This patch aims to make it more future-proof.

There are two limitations that this patch should solve:

  1. The current proposal does not leave space for default Subnets value
  2. The current proposal does not leave space for granular control of IPAM

This patch should solve both by introducing a new attribute called IPAMLevel. This attribute dictates how much of the network IP configuration will be done by OVN and how much is left to the user.

The initial implementation of 4.18 will need only two values: Disabled and FullyManaged. Later releases should introduce a new level, where OVN will manage configuration of logical routers (routing, NAT, default GW), but configuration of individual Pod's / VM's IPs will be left to the user.

Apart from introducing this new field, this patch also changes the behavior of nil Subnets. While with the current enhancement not setting subnets means disabling IPAM, with this patch, not setting subnets is not allowed unless Disabled IPAMLevel is explicitly set. This will allow us to introduce default subnet value in the future, if we decide to go that path.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2024
Copy link
Contributor

openshift-ci bot commented Oct 25, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@phoracek phoracek force-pushed the ipam-level branch 2 times, most recently from 595d0c8 to aa47656 Compare October 25, 2024 13:27
Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

I have some wording suggestions to make it clearer.

I think it is a nice suggestion, that opens the door for a more granular IPAM configuration, according to the use-cases sought by the user.

enhancements/network/user-defined-network-segmentation.md Outdated Show resolved Hide resolved
enhancements/network/user-defined-network-segmentation.md Outdated Show resolved Hide resolved
enhancements/network/user-defined-network-segmentation.md Outdated Show resolved Hide resolved
@phoracek phoracek marked this pull request as ready for review October 25, 2024 14:43
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2024
Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Tks.

Copy link
Contributor

openshift-ci bot commented Oct 25, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maiqueb
Once this PR has been reviewed and has the lgtm label, please assign abhat for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@tssurya
Copy link
Contributor

tssurya commented Oct 25, 2024

/assign @trozet / @npinaeva / @ormergi / @jcaamano for reviews

@@ -464,6 +465,7 @@ Suggested API validation rules:
- `Subnets` are mandatory for `Layer3` topology.
- `Localnet` topology is not supported for primary network.
- `IPAMLifecycle` is supported for `Layer2` and `Localnet` topology.
- When `IPAMLevel` is `Disabled`, `Subnets` cannot be set. When it is `FullyManaged`, `Subnets` are required.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume for layer3 there can never be a disabled mode right? for both role primary and role secondary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so too - L3 has to do routing between nodes, and it could not do routing without subnet awareness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tell me if you would prefer me to reword this line. I think that the condition is already present in the list, but it is not obvious (subnets and ipamlevel points need to be merged).

The current proposal for UDN's IPAM configuration is limited and would
not allow a simple expansion covering other use-cases. This patch
aims to make it more future-proof.

There are two limitations that this should solve:

1) The current proposal does not leave space for default subnets value
2) The current proposal does not leave space for granular control of IPAM

This patch should solve both by introducing a new attribute called
`IPAMLevel`. This attribute dictates how much of the network IP
configuration will be done by OVN and how much is left to the user.

The initial implementation of 4.18 will need only two values:
`Disabled` and `FullyManaged`. Later releases should introduce a new
level, when OVN will manage configuration of logical routers (routing,
NAT, default GW), but configuration of individual Pod's / VM's IPs will
be left to the user.

Apart from introducing this new field, this patch also changes the
behavior of nil subnets. While with the current enhancement not setting
subnets means disabling IPAM, with this patch, not setting subnets is
not allowed unless `Disabled` `IPAMLevel` is explicitly set. This will
allow us to introduce default subnet value in the future, if we decide
to go that path.

Signed-off-by: Petr Horacek <[email protected]>
Copy link
Contributor

openshift-ci bot commented Oct 25, 2024

@phoracek: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@npinaeva
Copy link
Member

npinaeva commented Nov 5, 2024

I think if we already have some future options like partially-managed in mind, it would be nice to include those options into this enhancement, to make sure our current API will be properly extended when needed. we don't need to implement all of the options at the same time though.

Trying to figure out important parts of the subnet management process

  1. disabled (empty subnet now) - doesn't support most of the ovn-k features, including egress traffic outside the cluster (would be nice to have a full list of unsupported features, or, if easier, what is supported). Is not allowed for primary networks.
  2. subnet-no-ipam - subnet is known to the ovn-k, but the ip allocation from that subnet is not done by ovn-k. That should allow egress traffic (again, full list of supported/unsupported features would be nice here). Based on the feature set, we may need to figure out if this should be allowed for primary networks or not.
  3. fully-managed - IPAM is done by ovn-k, all UDN features are supported, can be used with any network type.
    IIC, Options 2 and 3 could support default subnet allocation, but as we know UDN inter-connect may happen at some point, we should discuss the algorithm for that default subnet selection from the beginning.

@phoracek does that ^ sound right?

@phoracek
Copy link
Member Author

phoracek commented Nov 6, 2024

@npinaeva thanks for the review. My goal with this PR is to keep the API open for the anticipated enhancements. I wanted to avoid designing the specific IPAM feature here - I don't trust myself to do it right and I'd rather leave that to you subject matter experts.

But if you would find it useful to have an example of such an extension in this PR, I'm happy to do that (just take it with a grain of salt).

Your summary of the 3 options matches my understanding / expectations +1.

@npinaeva what would you like me to add to this PR? I can add the third option for IPAMLevel and describe it with a few words. I don't feel comfortable going in detail about each of the options though - this PR is not introducing those functions, and I'm not an SME/authority to say what their "mission" is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants