🌱 optimize ip allocation

[schrej]

What this PR does / why we need it:
It splits the allocation of IP addresses from IP pools into two steps: creating the claim, and attempting to fetch the address.
This way we can give the IPAM providers a little bit of time to fulfil the claim.

[metal3-io-bot]

Hi @schrej. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[lentzi90]

/ok-to-test

[kashifest]

What this PR does / why we need it: It splits the allocation of IP addresses from IP pools into two steps: creating the claim, and attempting to fetch the address. This way we can give the IPAM providers a little bit of time to fulfil the claim. To avoid unnecessary re-queues we wait for 200ms if a claim should be fetched again before attempting to fetch the address.

In an ideal case, one or more claims would be created because they do not exist. We then wait 200ms, fetch the claims again, hoping that they were updated, and then fetch the IP address the claim was fulfilled with.

The delay of 200ms was chosen arbitrarily. Not sure what a good value would be here.

A bit unsure about the motivation here. Whats the harm if the reconciliation loop is requeing ? If you are concerned about too many reques, perhaps introducing something like this would make it much cleaner ?

[schrej]

A bit unsure about the motivation here. Whats the harm if the reconciliation loop is requeing ? If you are concerned about too many reques, perhaps introducing something like this would make it much cleaner ?

That's how it's working right now. If any of the IPAddresses is missing, it will requeue after 30s. Therefore a M3M takes at least two reconciliation cycles with 30s delay after initial creation. The goal is to handle an entire machine with a single reconciliation after it was first created, at least if the IP gets allocated quickly enough (e.g. within 200ms). I've initially suggested it in Slack, and just went for an implementation as it's not that complicated.

The ideal solution would be registering watches on IPClaims so we can react as soon as something changes, but with the CAPI contract that would be quite difficult to do.

[lentzi90]

This looks like a good cleanup to me. But I think the 200ms wait that is mentioned in the description is now gone?

[lentzi90]

/test-centos-e2e-main

[schrej]

Yeah. We discussed this during the community call and it was decided that we don't wait after creation. Iirc the main reason was that we don't want to block the reconciliation queue unnecessarily. I'll update the description.

This is also missing a test for claim creation. I had one before, but the complexity increased a lot since preallocations were introduced in the meantime, which requires to mock m3dt, m3m, ma and bmh in addition to m3d.

[furkatgofurov7]

/test-ubuntu-integration-main

[lentzi90]

/test-centos-e2e-parallel-main

[lentzi90]

/lgtm

[furkatgofurov7]

Thanks for the PR @schrej

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [furkatgofurov7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[furkatgofurov7]

/test golangci-lint