⚠️ Store []ResolvedPortSpec in ReferencedMachineResources #1951

mdbooth · 2024-03-15T13:37:11Z

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:

Merge 🌱 Deduplicate AdoptMachinePorts and AdoptBastionPorts #1944

/hold

k8s-ci-robot · 2024-03-15T13:37:16Z

[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~~ [mdbooth]

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

netlify · 2024-03-15T13:37:27Z

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

Name	Link
🔨 Latest commit	`b6d7748`
🔍 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... Use your smartphone camera to open QR code link.

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

mdbooth · 2024-03-15T13:42:39Z

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

mdbooth · 2024-03-15T13:51:09Z

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

mdbooth · 2024-03-15T20:10:36Z

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

mdbooth · 2024-03-16T17:45:34Z

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

mdbooth · 2024-03-18T11:22:07Z

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

dulek

Comments inline, most of them are more of a question thank a remark.

api/v1beta1/types.go

dulek · 2024-03-18T17:40:14Z

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml

@@ -6626,6 +6430,10 @@ spec:
                                implementations. What type of vNIC is actually available depends on
                                deployments. If not specified, the Neutron default value is used.
                              type: string
+                          required:
+                          - description


I think this sucks that description is required, but if it's to change in a later patch I'm okay.

This is the resolved port spec in the status, which will always have a description: if it didn't have a description we would have generated one. This is expected.

api/v1beta1/openstackmachine_types.go

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml

dulek · 2024-03-18T18:00:39Z

pkg/cloud/services/networking/port.go

 	}
 	return normalizedPorts, nil
 }

-// normalizePortTarget ensures that the port has a network ID.


I think all below should be double-checked by @MaysaMacedo. It looks right to me from dual stack perspective, but this is the delicate part in this PR.

It should be just code motion with changes for context. I didn't intentionally change the logic here.

test/e2e/suites/e2e/e2e_test.go

mdbooth · 2024-03-19T15:21:47Z

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

mdbooth · 2024-03-20T10:01:40Z

@dulek Comments addressed.

mdbooth · 2024-03-21T11:38:00Z

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

mdbooth · 2024-03-21T12:51:32Z

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

mdbooth · 2024-03-21T14:06:54Z

/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/[email protected]/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 · 2024-03-21T16:04:04Z

Same error.

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.

mdbooth · 2024-03-21T18:55:43Z

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

huxcrux

I looked thru the code and think it looks good. Only one inline comment regarding the function clusterName that actually returns a name used for resources and is not the cluster name.

It can safely be ignored however may cause confusion for someone reading the code. When I see clusterName i almost every time ask myself why cluster.Name is not used since cluster exists almost everywhere already

huxcrux · 2024-03-21T21:23:41Z

pkg/utils/names/names.go

@@ -36,3 +38,7 @@ func GetFloatingAddressClaimName(openStackMachineName string) string {
 func GetOpenStackMachineNameFromClaimName(claimName string) string {
 	return strings.TrimSuffix(claimName, fmt.Sprintf("-%s", FloatingAddressIPClaimNameSuffix))
 }
+
+func ClusterName(cluster *clusterv1.Cluster) string {


Maybe we should call this variable something else? I get this is used for a lot of resources however it is not the clusters name.

Maybe clusterResourceName or something could work?

I really like the initiative to move this to a function since the name are present in a lot of places in the code.

That's a good point, and I agree. Confusion from this was also the cause of a bug in @EmilienM's ports series a few weeks back that took both of us working together over a day to track down. I think it's worth picking a better name.

I've pushed a separate commit which changes it everywhere. Note that clusterName in the e2e tests actually does mean cluster name, so I haven't changed it there.

mdbooth · 2024-03-22T10:38:36Z

~~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 · 2024-03-22T11:07:58Z

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

huxcrux · 2024-03-22T11:09:02Z

/lgtm

Lets hope the full e2e test passes 🤞

mdbooth · 2024-03-22T12:58:08Z

Passed!

/hold cancel

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2024

k8s-ci-robot requested a review from EmilienM March 15, 2024 13:37

k8s-ci-robot requested a review from jichenjc March 15, 2024 13:37

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 15, 2024

mdbooth force-pushed the issue1943 branch 2 times, most recently from e8507e0 to 8281e5a Compare March 15, 2024 13:50

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2024

mdbooth force-pushed the issue1943 branch from 8281e5a to 26d0113 Compare March 15, 2024 16:53

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 15, 2024

mdbooth mentioned this pull request Mar 15, 2024

🐛 Fix port name after port creation failure #1941

Merged

mdbooth force-pushed the issue1943 branch from 26d0113 to 031f9c2 Compare March 15, 2024 19:57

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2024

mdbooth force-pushed the issue1943 branch from e6c66c1 to 24d72e2 Compare March 18, 2024 11:21

dulek suggested changes Mar 18, 2024

View reviewed changes

mdbooth force-pushed the issue1943 branch from 24d72e2 to 44f7a16 Compare March 19, 2024 09:38

mdbooth mentioned this pull request Mar 19, 2024

RELEASE NOTE: Tags are now applied to the bastion #1958

Closed

mdbooth force-pushed the issue1943 branch from 44f7a16 to 51b34bd Compare March 20, 2024 10:01

mdbooth requested a review from dulek March 20, 2024 12:12

mdbooth force-pushed the issue1943 branch from 51b34bd to dc543ce Compare March 20, 2024 16:28

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2024

mdbooth force-pushed the issue1943 branch from dc543ce to b349c0c Compare March 21, 2024 10:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2024

mdbooth mentioned this pull request Mar 21, 2024

🌱 Rename referencedResources and dependentResources #1965

Merged

1 task

mdbooth force-pushed the issue1943 branch from 6e4c544 to 8017c89 Compare March 21, 2024 18:55

huxcrux reviewed Mar 21, 2024

View reviewed changes

Rename ClusterName to ClusterResourceName

b6d7748

k8s-ci-robot assigned huxcrux Mar 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2024

k8s-ci-robot merged commit cb99e17 into kubernetes-sigs:main Mar 22, 2024
10 checks passed

mdbooth deleted the issue1943 branch March 22, 2024 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Store []ResolvedPortSpec in ReferencedMachineResources #1951

⚠️ Store []ResolvedPortSpec in ReferencedMachineResources #1951

mdbooth commented Mar 15, 2024 •

edited

Loading

k8s-ci-robot commented Mar 15, 2024

netlify bot commented Mar 15, 2024 •

edited

Loading

mdbooth commented Mar 15, 2024

mdbooth commented Mar 15, 2024

mdbooth commented Mar 15, 2024

mdbooth commented Mar 16, 2024

mdbooth commented Mar 18, 2024

dulek left a comment

dulek Mar 18, 2024

mdbooth Mar 19, 2024

dulek Mar 18, 2024

mdbooth Mar 19, 2024

mdbooth commented Mar 19, 2024

mdbooth commented Mar 20, 2024

mdbooth commented Mar 21, 2024

mdbooth commented Mar 21, 2024

mdbooth commented Mar 21, 2024 •

edited

Loading

mdbooth commented Mar 21, 2024

mdbooth commented Mar 21, 2024

huxcrux left a comment

huxcrux Mar 21, 2024

mdbooth Mar 22, 2024

mdbooth Mar 22, 2024

mdbooth commented Mar 22, 2024 •

edited

Loading

mdbooth commented Mar 22, 2024

huxcrux commented Mar 22, 2024

mdbooth commented Mar 22, 2024

⚠️ Store []ResolvedPortSpec in ReferencedMachineResources #1951

⚠️ Store []ResolvedPortSpec in ReferencedMachineResources #1951

Conversation

mdbooth commented Mar 15, 2024 • edited Loading

k8s-ci-robot commented Mar 15, 2024

netlify bot commented Mar 15, 2024 • edited Loading

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

mdbooth commented Mar 15, 2024

mdbooth commented Mar 15, 2024

mdbooth commented Mar 15, 2024

mdbooth commented Mar 16, 2024

mdbooth commented Mar 18, 2024

dulek left a comment

Choose a reason for hiding this comment

dulek Mar 18, 2024

Choose a reason for hiding this comment

mdbooth Mar 19, 2024

Choose a reason for hiding this comment

dulek Mar 18, 2024

Choose a reason for hiding this comment

mdbooth Mar 19, 2024

Choose a reason for hiding this comment

mdbooth commented Mar 19, 2024

mdbooth commented Mar 20, 2024

mdbooth commented Mar 21, 2024

mdbooth commented Mar 21, 2024

mdbooth commented Mar 21, 2024 • edited Loading

mdbooth commented Mar 21, 2024

mdbooth commented Mar 21, 2024

huxcrux left a comment

Choose a reason for hiding this comment

huxcrux Mar 21, 2024

Choose a reason for hiding this comment

mdbooth Mar 22, 2024

Choose a reason for hiding this comment

mdbooth Mar 22, 2024

Choose a reason for hiding this comment

mdbooth commented Mar 22, 2024 • edited Loading

mdbooth commented Mar 22, 2024

huxcrux commented Mar 22, 2024

mdbooth commented Mar 22, 2024

mdbooth commented Mar 15, 2024 •

edited

Loading

netlify bot commented Mar 15, 2024 •

edited

Loading

mdbooth commented Mar 21, 2024 •

edited

Loading

mdbooth commented Mar 22, 2024 •

edited

Loading