🐛 Remove cluster dependency to resolve ports #2040

EmilienM · 2024-04-29T23:02:34Z

What this PR does / why we need it:

The default network and some other ports parameters are now figured out
outside of the port functions, so they don't rely on the OpenStackMachine spec directly anymore.

/hold

k8s-ci-robot · 2024-04-29T23:02:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mdbooth for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

netlify · 2024-04-29T23:02:50Z

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

Name	Link
🔨 Latest commit	`3a2ad29`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/66325bccb3a5ba0008fbbea2
😎 Deploy Preview	https://deploy-preview-2040--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.

EmilienM · 2024-04-30T00:20:26Z

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

EmilienM · 2024-04-30T01:04:32Z

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

controllers/openstackcluster_controller.go

EmilienM · 2024-04-30T12:49:15Z

controllers/openstackmachine_controller_test.go

@@ -413,30 +400,6 @@ func Test_reconcileDelete(t *testing.T) {
 },
 wantRemoveFinalizer: true,
 },
- {


I had to remove this test, it was not passing anymore. No calls were made at all. I guess it's an upgrade issue?

mdbooth

Sorry, barely got started with this review. I'll continue tomorrow.

mdbooth · 2024-04-30T10:58:56Z

controllers/openstackcluster_controller.go

+ return networkID, fixedIPs
+}
+
+func DefaultPortsOpts(openStackCluster *infrav1.OpenStackCluster, ports []infrav1.PortOpts, instanceTrunk bool) []infrav1.PortOpts {


nit: Can we kill PortOpts everywhere except the type name (and eventually there, too: it's a PortSpec). These are just default ports.

ok, I can do it separately and globally.

EmilienM · 2024-04-30T18:50:24Z

/test pull-cluster-api-provider-openstack-e2e-test
a flake

EmilienM · 2024-05-01T14:24:50Z

pkg/cloud/services/networking/port.go

-
- defaultSecurityGroupIDs, err := s.GetSecurityGroups(spec.SecurityGroups)
+// If no ports are in the spec, returns an error.
+func (s *Service) ConstructPorts(portsOpts []infrav1.PortOpts, securityGroups []infrav1.SecurityGroupParam, instanceTrunk bool, clusterResourceName, baseName string, managedSecurityGroup *string, baseTags []string) ([]infrav1.ResolvedPortSpec, error) {


for myself, check whether securityGroups could be renamed to defaultSecurityGroups?

EmilienM · 2024-05-01T14:25:49Z

pkg/cloud/services/networking/port.go

- defaultSecurityGroupIDs, err := s.GetSecurityGroups(spec.SecurityGroups)
+// If no ports are in the spec, returns an error.
+func (s *Service) ConstructPorts(portsOpts []infrav1.PortOpts, securityGroups []infrav1.SecurityGroupParam, instanceTrunk bool, clusterResourceName, baseName string, managedSecurityGroup *string, baseTags []string) ([]infrav1.ResolvedPortSpec, error) {
+ defaultSecurityGroupIDs, err := s.GetSecurityGroups(securityGroups)


for myself, check whether clusterResourceName could be replaced by something with "description" in it, if it's only used for port description.

EmilienM · 2024-05-01T14:28:28Z

pkg/cloud/services/networking/port.go

- }
- resolvedPort.SecurityGroups = defaultSecurityGroupIDs
- resolvedPort.NetworkID, resolvedPort.FixedIPs, _ = defaultNetworkTarget(defaultNetwork)
+ return nil, fmt.Errorf("no networks or ports found in the machine spec")


we could also return an empty list and noop.

The default network and some other ports parameters are now figured out outside of the port functions, so they don't rely on the OpenStack cluster anymore.

k8s-ci-robot · 2024-05-01T16:25:16Z

@EmilienM: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-openstack-e2e-full-test	`6fb7b8c`	link	false	`/test pull-cluster-api-provider-openstack-e2e-full-test`
pull-cluster-api-provider-openstack-test	`3a2ad29`	link	true	`/test pull-cluster-api-provider-openstack-test`
pull-cluster-api-provider-openstack-e2e-test	`3a2ad29`	link	true	`/test pull-cluster-api-provider-openstack-e2e-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot · 2024-05-02T17:37:39Z

PR needs rebase.

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.

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 29, 2024

k8s-ci-robot requested review from dulek and jichenjc April 29, 2024 23:02

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 29, 2024

EmilienM changed the title ~~Remove cluster dependency to resolve ports~~ 🐛 Remove cluster dependency to resolve ports Apr 29, 2024

EmilienM commented Apr 30, 2024

View reviewed changes

controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved

EmilienM commented Apr 30, 2024

View reviewed changes

controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved

EmilienM commented Apr 30, 2024

View reviewed changes

mdbooth reviewed Apr 30, 2024

View reviewed changes

EmilienM force-pushed the om-undep-cluster-port branch from 6fb7b8c to 0e5028d Compare April 30, 2024 18:14

EmilienM commented May 1, 2024

View reviewed changes

Remove cluster dependency to resolve ports

3a2ad29

The default network and some other ports parameters are now figured out outside of the port functions, so they don't rely on the OpenStack cluster anymore.

EmilienM force-pushed the om-undep-cluster-port branch from 0e5028d to 3a2ad29 Compare May 1, 2024 15:12

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 1, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Remove cluster dependency to resolve ports #2040

🐛 Remove cluster dependency to resolve ports #2040

EmilienM commented Apr 29, 2024 •

edited

k8s-ci-robot commented Apr 29, 2024

netlify bot commented Apr 29, 2024 •

edited

EmilienM commented Apr 30, 2024

EmilienM commented Apr 30, 2024

EmilienM Apr 30, 2024

mdbooth left a comment

mdbooth Apr 30, 2024

EmilienM Apr 30, 2024 •

edited

EmilienM commented Apr 30, 2024

EmilienM May 1, 2024

EmilienM May 1, 2024

EmilienM May 1, 2024

k8s-ci-robot commented May 1, 2024

k8s-ci-robot commented May 2, 2024

🐛 Remove cluster dependency to resolve ports #2040

Are you sure you want to change the base?

🐛 Remove cluster dependency to resolve ports #2040

Conversation

EmilienM commented Apr 29, 2024 • edited

k8s-ci-robot commented Apr 29, 2024

netlify bot commented Apr 29, 2024 • edited

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

EmilienM commented Apr 30, 2024

EmilienM commented Apr 30, 2024

EmilienM Apr 30, 2024

Choose a reason for hiding this comment

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth Apr 30, 2024

Choose a reason for hiding this comment

EmilienM Apr 30, 2024 • edited

Choose a reason for hiding this comment

EmilienM commented Apr 30, 2024

EmilienM May 1, 2024

Choose a reason for hiding this comment

EmilienM May 1, 2024

Choose a reason for hiding this comment

EmilienM May 1, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented May 1, 2024

k8s-ci-robot commented May 2, 2024

EmilienM commented Apr 29, 2024 •

edited

netlify bot commented Apr 29, 2024 •

edited

EmilienM Apr 30, 2024 •

edited