kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
279 stars 252 forks source link

⚠️ Store []ResolvedPortSpec in ReferencedMachineResources #1951

Closed mdbooth closed 4 months ago

mdbooth commented 4 months ago

The purpose of this change is fix an issue where we are storing unresolved references in ReferencedMachineResources. Specifically we are storing a PortOpts, which is a user-intent struct. PortOpts can contain unresolved references to both subnets and security groups, as well fields requiring additional processing which reference external objects: the port name, description, and tags.

We create a new type, ResolvedPortSpec, which can contain only fully resolved data. This can be seen in the new signature of CreatePorts(), which no longer requires any source of data other than the []ResolvedPortSpec from ReferencedMachineResources, and is now greatly simplified.

Fully resolving the port name also allows a simplification in port adoption.

All of the complexity now moves to ConstructPorts(), which is updated to return []ResolvedPortSpec instead of []PortOpts. ConstructPorts() is updated to resolve security groups, port name, description, and all subnets referenced in FixedIPs.

Fixes: #1943

TODO:

/hold

k8s-ci-robot commented 4 months ago

[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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/OWNERS)~~ [mdbooth] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
netlify[bot] commented 4 months ago

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
Latest commit b6d7748a7800f0cfde7d6edea510b3d55f0503fc
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65fd5bc413150c00080ada66
Deploy Preview https://deploy-preview-1951--kubernetes-sigs-cluster-api-openstack.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

@dulek Comments addressed.

mdbooth commented 4 months ago

Nothing obvious, could be a flake. /test pull-cluster-api-provider-openstack-e2e-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

Error was:

INFO: Applying the cluster template yaml to the cluster
[FAILED] Expected success, but got an error:
    <*errors.fundamental | 0xc002709368>: 
    exit status 1: stderr: 
    {
        msg: "exit status 1: stderr: ",
        stack: [0x1e615ee, 0x1efc805, 0x84ee73, 0x862ecd, 0x47b0c1],
    }
In [It] at: /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.6.0/e2e/clusterctl_upgrade.go:389 @ 03/21/24 13:25:03.601

Referring to https://github.com/kubernetes-sigs/cluster-api/blob/14efefeb46dbe8d0cd0f5b7d1718e00ec58fc079/test/e2e/clusterctl_upgrade.go#L389

That's not very illuminating! Lets see if it was a flake.

mdbooth commented 4 months ago

Same error.

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 4 months ago

I'm not going to re-run the full test on this latest push because it just updates function and variable names.

Actually we just broke the upgrade test with a search and replace the other day. Do I learn nothing? 🤦

mdbooth commented 4 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

huxcrux commented 4 months ago

/lgtm

Lets hope the full e2e test passes 🤞

mdbooth commented 4 months ago

Passed!

/hold cancel