Add SubnetPool controller#844
Conversation
dlaw4608
left a comment
dlaw4608
left a comment
There was a problem hiding this comment.
Thanks for the PR @winiciusallan ran through the e2e tests all work locally, overall the controller looks really good, only have the minor question regarding the mutability of the shared field
$ go run ./cmd/scaffold-controller -interactive=false \
-gophercloud-client=NewNetworkV2 \
-kind SubnetPool \
-gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/subnetpools \
-import-dependency Project \
-import-dependency AddressScope \
-optional-create-dependency Project \
-optional-create-dependency AddressScope
winiciusallan
force-pushed
the
subnet-pool-controller
branch
from
97fd880 to
64d0da4
Compare
winiciusallan
force-pushed
the
subnet-pool-controller
branch
from
64d0da4 to
8923c5f
Compare
eshulman2
left a comment
eshulman2
left a comment
There was a problem hiding this comment.
I believe this is a blocking issue for this one (from initial review) will make a second pass later
| } | ||
| } | ||
|
|
||
| func handlePrefixesUpdate(updateOpts *subnetpools.UpdateOpts, resource *resourceSpecT, osResource *osResourceT) { |
There was a problem hiding this comment.
Please note the following:
A list of subnet prefixes to assign to the subnet pool. The API merges adjacent prefixes and treats them as a single prefix. Each subnet prefix must be unique among all subnet prefixes in all subnet pools that are associated with the address scope.
https://docs.openstack.org/api-ref/network/v2/#id295
Neutron merges adjacent prefixes meaning that the response for:
{
"prefixes": ["192.168.0.0/24", "192.168.1.0/24", "172.16.0.0/21"]
}
Would look like this:
{
"prefixes": ["192.168.0.0/23", "172.16.0.0/21"]
}
(from the example in the link above)
Because this compares the literal sorted slices, the desired and observed values will never match in that case. The controller will repeatedly call UpdateSubnetPool and request another refresh, even though the address space is already correct.
Can you compare normalized CIDR sets instead?
Please also add a KUTTL test that creates a pool with two adjacent prefixes and verifies that it reaches Available=True and Progressing=False. That should catch a continuous reconciliation loop.
| // +kubebuilder:validation:MaxItems:=64 | ||
| // +listType=set | ||
| // +required | ||
| Prefixes []CIDR `json:"prefixes,omitempty"` |
There was a problem hiding this comment.
Prefix is mutable but neutron would only accept a superset of the old list of prefixes meaning that removing a prefix from the list would cause neutron to fail (although CRD update would be allowed). This means users are allowed to request an operation that the controller can't complete.
To actually allow update I think we should add a reconciler that uses the add and remove prefix endpoint. This will allow real mutability for this field.
There was a problem hiding this comment.
While I think this is the approach to be followed, we're currently missing the API calls for operations on subnet pool prefixes in Gophercloud. I opened gophercloud/gophercloud#3850 to track this work, but while this is not merged, what do you think of having this field immutable?
There was a problem hiding this comment.
Sounds like a fair compromise for now.
3 participants
This PR introduces the SubnetPool controller, which delegates the manage of subnet addresses allocation to Neutron.
Notes:
DefaultQuota, because we can't unset it, neither create it as0.<Min|Max>PrefixLengthrequired in the API, because we can't unset them when updating. Another possibility is to keep them immutable.Closes #393