WIP virt-controller, ipam: Improve ipamclaim package isolation #11892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
kubevirt-bot
added
do-not-merge/work-in-progress
|
[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 |
kubevirt-bot
added
size/XXL
sig/buildsystem
oshoval
force-pushed
the
ipam2
branch
3 times, most recently
from
4fe8610
to
a997dea
Compare
oshoval
changed the title
kubevirt-bot
removed
the
do-not-merge/work-in-progress
kubevirt-bot
added
the
needs-rebase
oshoval
changed the title
|
Lets wait with the review please, because original PR can be changed, drafting |
kubevirt-bot
added
the
do-not-merge/work-in-progress
oshoval
changed the title
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>
Make GenerateMultusCNIAnnotationFromNameScheme as a builder. This allows amending the annotations, using external go packages. Signed-off-by: Or Shoval <oshoval@redhat.com>
WithIPAMClaimRef method finds a multus annotation according to the network name (and a non-empty interface request), and adds the desired ipam claim reference to it. Signed-off-by: Or Shoval <oshoval@redhat.com>
kubevirt-bot
removed
the
needs-rebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
Improve isolation between network and ipamclaim packages by presenting a builder
for
GenerateMultusCNIAnnotationFromNameScheme.Drop ipamclaims types package, now that the cycling dependency is solved.
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 4 commits are relevant, rest is cherry-pick of #11410
If desired, the original logic of
GenerateMultusCNIAnnotationFromNameSchemecan be split to morebuilder methods.
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