⚠️ OpenStackCluster api general cleanup #1930
Conversation
k8s-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mdbooth
force-pushed
the
openstackcluster_apicleanup
branch
2 times, most recently
from
72e6ad0
to
74510ef
Compare
|
This adds a fix for the externalNetwork issue which was causing CI failures. The issue was CI is emitting the following in the cluster spec: spec:
externalNetwork:
id: nullThe previous logic in ReconcileExternalNetwork correctly treated this as an empty stanza and executed the fallback 'list all external networks' query. The new logic which failed treated this as an explicitly given empty external network filter and executed it, returning all networks including non-external networks. This was an error as there were more than one. The principal problem here is that we were executing an external network query without the external flag. This is fixed by setting it for both kinds of query (explicit and implicit). Separately I have also opened #1932 to fix the weird external network stanza in the CI templates. |
mdbooth
force-pushed
the
openstackcluster_apicleanup
branch
2 times, most recently
from
cec64a2
to
08beb1d
Compare
This makes the behaviour slightly more ergonomic and safer as we turn these into pointers.
This improves safety and ergonomics checking it when NetworkFilter can be be a pointer type.
For non-mergeable list types, make the default 'atomic' behaviour explicit.
ExternalNetwork was already marked optional. This change allows it to be omitted when marshalling and unmarshalling the object in Go. This change also adds the 'external' flag when executing an explicit external network query.
Affects go marshalling only.
…rPort->optional Affects go marshalling only.
The following types become a corresponding optional type: * NetworkMTU * DisableExternalNetwork * DisableAPIServerFloatingIP * DisablePortSecurity * ControlPlaneOmitAvailabilityZone
mdbooth
force-pushed
the
openstackcluster_apicleanup
branch
from
08beb1d
to
e6a194b
Compare
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
|
|
||
| loadBalancerName := getLoadBalancerName(clusterName) | ||
| s.scope.Logger().Info("Reconciling load balancer member", "loadBalancerName", loadBalancerName) | ||
|
|
||
| lbID := openStackCluster.Status.APIServerLoadBalancer.ID | ||
| portList := []int{int(openStackCluster.Spec.ControlPlaneEndpoint.Port)} | ||
| var portList []int | ||
| if openStackCluster.Spec.ControlPlaneEndpoint != nil { |
There was a problem hiding this comment.
Do we really need the if since we already checked for it at L521?
I guess it's ok. Just in case it was a leftover.
There was a problem hiding this comment.
No, but I've done similar in a few places. In my first pass at this I didn't double-check any pointers, but after hitting a few panics in various iterations I've decided to err on the side of pretty much always checking. Apart from anything else, this is safer against future cut and paste, where somebody might move either use such that one or the other is no longer guarded.
I'd prefer to fix this through the type system, but I'm not having much joy with that today: https://hachyderm.io/@mattb/112077588227331296
|
I've reviewed the commits one by one as suggested and everything looks good to me. I have a minor comment inline, but I think this PR is good to go. Thanks for this huge work! /lgtm |
k8s-ci-robot
added
the
lgtm
|
This looks good /lgtm |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Interestingly, this PR introduces no substantial changes to the CRDs. It primarily affects Go marshalling and documentation. The changes are mostly mechanical.
The PR should hopefully be simpler to review by commit.
The first 2 commits are preparatory, and slightly separate to the other commits. They add some new functionality to the pkg/utils/optional types and update the existing PortOpts to use that functionality.
The rest are structed as changes to single fields, or related sets of fields.
/hold