-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
WIP virt-controller, ipam: Improve ipamclaim package isolation #11892
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
4fe8610
to
a997dea
Compare
Lets wait with the review please, because original PR can be changed, drafting |
Allows us to use the new IPAMClaimReference which is required for the new secondary networks IPAM feature. Signed-off-by: Or Shoval <oshoval@redhat.com>
Instead only checking if a VMI has an owner, return it also in case it does. Usable when we need to owner itself if exists. The next commit will require it, because it needs the VMI owner ref itself, in case it exists. It adds the same owner ref to the IPAM Claim it creates. This way we don't need to do seperate check if it has an owner ref and then to fetch if it does. Signed-off-by: Or Shoval <oshoval@redhat.com>
PersistentIPs FG should be enabled in order to consume a NAD that uses allowPersistentIPs knob. Signed-off-by: Or Shoval <oshoval@redhat.com>
Refactor GetNetworkToResourceMap logic to first get all the NADs, and then extract the required info. This in turn will allow extracing more required info from the NAD on following commits, without reading them twice in the same context. Signed-off-by: Or Shoval <oshoval@redhat.com>
Packname should be network_test, update it. Remove non exported functions unit testing as we need to unit test only the package API. Export GetNamespaceAndNetworkName, so we can use it as part of createNADs. Signed-off-by: Or Shoval <oshoval@redhat.com>
This way we can use it in a more generic way, i.e on packages which the network package itself imports, without creating cycling dependency. Signed-off-by: Or Shoval <oshoval@redhat.com>
Include the new required ipamclaims package. Signed-off-by: Or Shoval <oshoval@redhat.com>
Signed-off-by: Or Shoval <oshoval@redhat.com>
The generated client-go of ipamclaims is already available at https://github.com/k8snetworkplumbingwg/ipamclaims so we don't need to generate it ourselves as done for other clients (see hack/generate.sh). It is exactly the client we need already. Signed-off-by: Or Shoval <oshoval@redhat.com>
This pacakge allows supporting IPAM for virtualization workloads. Allowing VMs to get the same IP addresses during: 1. VM restart 2. VM migration Hot plug is not supported yet, more info in the relevant commit. Signed-off-by: Or Shoval <oshoval@redhat.com>
Once a new pod is created, create the required ipam claims. Note we don't support hotplug yet, so it the new method can support only hashed interfaces naming scheme. We are using a builder method WithIPAMClaimManager, this way we don't affect the non relevant unit tests, so we don't need to mock anything. Signed-off-by: Or Shoval <oshoval@redhat.com>
Interfaces that requested persistent IPs, are annotated with the corresponding ipam claim. This way the CNI knows that the feature is enabled, and which ipam claim is associated with each interface. Signed-off-by: Or Shoval <oshoval@redhat.com>
… interfaces Since we don't support yet persistent IPs hotplug / hotunplug, block it on VM level, and log about the blocking. This way the VMI will still reflect the right state as done for interfaces with ordinal naming scheme. On the other hand, we must pass the ipam claim parameters, when recalculating the annotations upon hotplug / hotunplug, so the ipam claim reference won't be stripped, once other interface is hotplugged / hotunplugged. Signed-off-by: Or Shoval <oshoval@redhat.com>
This is a pre reafactor that removes passing networkToIPAMClaimParams when generating multus annotations. The next commit will present an alternative that allows better isolation between network and ipamclaims packages. Signed-off-by: Or Shoval <oshoval@redhat.com>
f31db05
to
47cbbdd
Compare
Make GenerateMultusCNIAnnotationFromNameScheme as a builder. This allows amending the annotations, using external packages. Signed-off-by: Or Shoval <oshoval@redhat.com>
WithIPAMClaimRef method finds the matching multus annotation, and adds the desired ipam claim reference to it. Signed-off-by: Or Shoval <oshoval@redhat.com>
PR needs rebase. 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. |
What this PR does
Improve isolation between network and ipamclaim packages by presenting a builder
for
GenerateMultusCNIAnnotationFromNameScheme
.Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Special notes for your reviewer
Just last 3 commits are relevant, rest is cherry-pick of #11410
If desired, the original logic of
GenerateMultusCNIAnnotationFromNameScheme
can be split tobuilder methods, on a follow-up PR.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note