✨Infer port network from subnet #1519

mdbooth · 2023-03-29T14:04:34Z

NetworkID is a required field when creating a neutron port. We previously passed on this requirement in the Ports API. However we didn't have this restriction in the Networks API and inferred the network from a subnet if one was defined. This change eases the transition from Networks to Ports by removing this restriction for Ports.

TODOs:

tests

/hold

netlify · 2023-03-29T14:04:41Z

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

Name	Link
🔨 Latest commit	`6e59991`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/643d3cb0b684d7000811235a
😎 Deploy Preview	https://deploy-preview-1519--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 settings.

dulek

It looks quite complicated for a simple change, a few ideas inline.

dulek · 2023-03-30T09:15:55Z

pkg/cloud/services/compute/instance.go

+			networkID, err := func() (string, error) {
 				networkingService, err := s.getNetworkingService()
 				if err != nil {
-					return nil, err
+					return "", err
 				}

-				netIDs, err := networkingService.GetNetworkIDsByFilter(port.Network.ToListOpt())
-				if err != nil {
-					return nil, err
+				for i, fixedIP := range port.FixedIPs {
+					if fixedIP.Subnet == nil {
+						continue
+					}
+
+					subnetOpts := fixedIP.Subnet.ToListOpt()
+					subnets, err := networkingService.GetSubnetsByFilter(&subnetOpts)
+					if err != nil {
+						return "", err
+					}
+					for len(subnets) > 1 {
+						s.scope.Logger().V(4).Info("Can't infer network from subnet %d: multiple subnets match filter", i)
+						continue
+					}
+					for _, subnet := range subnets {
+						s.scope.Logger().V(4).Info("Inferred network %s from subnet %s", subnet.NetworkID, subnet.ID)
+						return subnet.NetworkID, nil
+					}


Feels like a pattern we often have, I'd at least extract it into a named function for code clarity.

Looks like we use this pattern everywhere we call GetSubnetsByFilter() 🤔

...except in getServerNetworks(), which I'm hoping to delete shortly.

I started pulling this out, btw. I kept the non-related parts in a separate commit.

pkg/cloud/services/compute/instance.go

mdbooth · 2023-03-30T12:42:37Z

It looks quite complicated for a simple change, a few ideas inline.

I agree, but there are a few things going on here.

Firstly is the stated change itself. This is itself more complex than it sounds because:

There may or may not be subnets
There may be arbitrarily many subnets
There may be fixed ips which don't have subnets

So you have to loop over it until you find one which works. Incidentally, documenting this behaviour isn't going to be fun, and I still need to do that ☹️ If it wasn't for the Network->Ports upgrade driver I probably wouldn't bother and just continue to require that network is defined.

Secondly I've refactored it a bit because it's complex enough to require its own unit tests, which I haven't written yet. Hence pulling out the normalizePorts function.

Lastly there's a slightly hidden agenda here. I'm writing this in the context of removing Networks, so I'm going to be throwing away the non-Ports part of constructNetworks in another PR. You'll notice, however, that this function returns []infrav1.Network. That struct doesn't make any sense any more, and I want to throw it away entirely and just pass Ports around. So in the new world order you normalize your ports, then use them. You don't have to wrap them in a different pointless struct first. Some of the code motion in this PR is with that in mind.

I want to backport this PR to version 0.7, though, so I can't fully do that here. This is to help anybody who needs to manually update their ports in v0.7 before upgrading to v0.8.

dulek · 2023-03-31T15:06:13Z

Alright, let's see the completed version then!

mdbooth · 2023-04-03T10:37:34Z

Alright, let's see the completed version then!

That isn't going to come in this PR, btw. I need to remove Networks first, which I'm doing in #1518.

However, this PR now has tests and docs. Hopefully ready to roll.

mdbooth · 2023-04-03T10:38:17Z

/cherry-pick release-0.7

k8s-infra-cherrypick-robot · 2023-04-03T10:38:19Z

@mdbooth: once the present PR merges, I will cherry-pick it on top of release-0.7 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.7

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.

mdbooth · 2023-04-03T14:24:15Z

I'm working on this in parallel with the Networks removal PR, btw, which is why I keep updating it. I don't want to move this code twice, so I'm effectively pulling Network removal requirements back into this PR.

I'm now at the point where I might move the entire 'use PortOpts natively' thing into this PR instead. The defaulting behaviour of ports is currently spread untidily across networking/port.go and compute/instance.go with very little in the way of pattern. I'm going to try to consolidate it.

dulek

Looks great, tiny suggestion inline.

dulek · 2023-04-04T15:31:28Z

pkg/cloud/services/compute/instance.go

-func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster, instanceSpec *InstanceSpec) ([]infrav1.Network, error) {
-	trunkRequired := false
+// normalizePortTarget ensures that the port has a network ID.
+func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster *infrav1.OpenStackCluster, portIdx int) error {


normalizePortTarget() sounds a bit confusing if it's just normalizePortNetwork().

I would update this, but I plan to refactor this code a fair bit anyway after backporting this to 0.7.

k8s-ci-robot · 2023-04-04T15:33:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, 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

dulek · 2023-04-04T15:33:10Z

/lgtm

It has hold, so /lgtm is safe.

jichenjc · 2023-04-06T04:03:20Z

docs/book/src/clusteropenstack/configuration.md

+
+`fixedIPs`, including all fields available in the `subnet` filter, are described in [the CRD](https://doc.crds.dev/github.com/kubernetes-sigs/cluster-api-provider-openstack/infrastructure.cluster.x-k8s.io/OpenStackMachine/v1alpha6@v0.7.1#spec-ports-fixedIPs).
+
+If no `fixedIPs` are specified, the port will get an address from every subnet in the network.


every subnet => is this first subnet of the network? This seems to be the way I provide my openstack to my end user..

I tested this few days ago, if network has multiple subnets, creating a port specifying just the network gives you a port with address in every of the network's subnets:

A POST with:

{ "port": { "admin_state_up": true, "name": "test", "network_id": "6acb08e4-86a1-47fa-a806-2f79a0b48abb" } }

Will get you:

{ "port": { <SNIP> "fixed_ips": [ { "ip_address": "192.168.25.127", "subnet_id": "4b73762e-8fbb-4555-9df6-fcd2b8fe2734" }, { "ip_address": "2001:db8::2c8", "subnet_id": "d808f460-99b0-4356-b158-92c3b6f97efd" } ], "id": "f5d15252-9f61-4d95-885b-9c3346fa2608", <SNIP> "name": "test", "network_id": "6acb08e4-86a1-47fa-a806-2f79a0b48abb", <SNIP> } }

mdbooth · 2023-04-06T08:27:43Z

I'm going to rework this a bit because it was starting to combine a refactor of the port creation code with a behavioural change. I'm going to try to separate them into 2 separate PRs.

NetworkID is a required field when creating a neutron port. We previously passed on this requirement in the Ports API. However we didn't have this restriction in the Networks API and inferred the network from a subnet if one was defined. This change eases the transition from Networks to Ports by removing this restriction for Ports.

mdbooth · 2023-04-17T12:33:34Z

I'm going to rework this a bit because it was starting to combine a refactor of the port creation code with a behavioural change. I'm going to try to separate them into 2 separate PRs.

I've changed my mind on this. I want to backport this change, but it was getting too invasive and too big. I'd like to land this version after all, and I'll do any refactoring on top of it in main.

dulek · 2023-04-18T15:25:45Z

/lgtm

Question inline, feel free to unhold this if that TODO is supposed to stay there.

dulek · 2023-04-18T15:21:50Z

pkg/cloud/services/compute/instance.go

+	// TODO: These are spec errors: they should set the machine to failed
+	if len(netIDs) > 1 {
+		return fmt.Errorf("network filter for port %d returns more than one result", portIdx)
+	} else if len(netIDs) == 0 {
+		return fmt.Errorf("network filter for port %d returns no networks", portIdx)
+	}


Do we allow this to be TODO?

It's already the case: I'm just adding the TODO because I noticed it. However, fixing these requires the refactor I'm trying to avoid here. I want to split the validation of ports such that everything which needs to be looked up is done for all ports before we start to create any of them. It just involves a ton of code motion.

mdbooth · 2023-04-18T15:32:22Z

/hold cancel

k8s-infra-cherrypick-robot · 2023-04-18T16:55:42Z

@mdbooth: #1519 failed to apply on top of branch "release-0.7":

Applying: Add GetSubnetByFilter
Using index info to reconstruct a base tree...
M	controllers/openstackcluster_controller.go
M	pkg/cloud/services/networking/network.go
M	pkg/cloud/services/networking/router.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloud/services/networking/router.go
Auto-merging pkg/cloud/services/networking/network.go
Auto-merging controllers/openstackcluster_controller.go
Applying: Infer port network from subnet
Using index info to reconstruct a base tree...
M	docs/book/src/clusteropenstack/configuration.md
M	go.mod
M	pkg/cloud/services/compute/instance.go
M	pkg/cloud/services/compute/instance_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloud/services/compute/instance_test.go
Auto-merging pkg/cloud/services/compute/instance.go
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging docs/book/src/clusteropenstack/configuration.md
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Infer port network from subnet
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.7

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 the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2023

k8s-ci-robot requested a review from dulek March 29, 2023 14:04

k8s-ci-robot requested a review from jichenjc March 29, 2023 14:04

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

dulek suggested changes Mar 30, 2023

View reviewed changes

mdbooth force-pushed the port-nonetwork branch from e5421fc to 00ff874 Compare April 3, 2023 10:35

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

mdbooth force-pushed the port-nonetwork branch 2 times, most recently from 22d0be8 to 9680b33 Compare April 3, 2023 12:54

dulek approved these changes Apr 4, 2023

View reviewed changes

k8s-ci-robot assigned dulek Apr 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2023

jichenjc reviewed Apr 6, 2023

View reviewed changes

mdbooth marked this pull request as draft April 6, 2023 08:26

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2023

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

Add GetSubnetByFilter

6a6b86a

mdbooth force-pushed the port-nonetwork branch from 9680b33 to 6e59991 Compare April 17, 2023 12:33

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 17, 2023

mdbooth marked this pull request as ready for review April 18, 2023 08:26

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 18, 2023

k8s-ci-robot requested review from jichenjc and tobiasgiese April 18, 2023 08:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2023

dulek reviewed Apr 18, 2023

View reviewed changes

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

k8s-ci-robot merged commit 17d3f36 into kubernetes-sigs:main Apr 18, 2023

mdbooth deleted the port-nonetwork branch April 19, 2023 15:35

mdbooth mentioned this pull request Apr 19, 2023

✨Infer port network from subnet #1526

Merged

This was referenced May 25, 2023

Add cherrypicker to cloud-provider-openstack kubernetes/test-infra#29596

Merged

🐛 Always filter cluster subnets by cluster network ID #1572

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Infer port network from subnet #1519

✨Infer port network from subnet #1519

mdbooth commented Mar 29, 2023 •

edited

Loading

netlify bot commented Mar 29, 2023 •

edited

Loading

dulek left a comment

dulek Mar 30, 2023

mdbooth Mar 30, 2023 •

edited

Loading

mdbooth Apr 3, 2023

mdbooth commented Mar 30, 2023 •

edited

Loading

dulek commented Mar 31, 2023

mdbooth commented Apr 3, 2023

mdbooth commented Apr 3, 2023

k8s-infra-cherrypick-robot commented Apr 3, 2023

mdbooth commented Apr 3, 2023

dulek left a comment

dulek Apr 4, 2023

mdbooth Apr 18, 2023

k8s-ci-robot commented Apr 4, 2023

dulek commented Apr 4, 2023

jichenjc Apr 6, 2023

dulek Apr 6, 2023

mdbooth commented Apr 6, 2023

mdbooth commented Apr 17, 2023

dulek commented Apr 18, 2023

dulek Apr 18, 2023

mdbooth Apr 18, 2023

mdbooth commented Apr 18, 2023

k8s-infra-cherrypick-robot commented Apr 18, 2023


		`fixedIPs`, including all fields available in the `subnet` filter, are described in [the CRD](https://doc.crds.dev/github.com/kubernetes-sigs/cluster-api-provider-openstack/infrastructure.cluster.x-k8s.io/OpenStackMachine/v1alpha6@v0.7.1#spec-ports-fixedIPs).

		If no `fixedIPs` are specified, the port will get an address from every subnet in the network.

✨Infer port network from subnet #1519

✨Infer port network from subnet #1519

Conversation

mdbooth commented Mar 29, 2023 • edited Loading

netlify bot commented Mar 29, 2023 • edited Loading

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

dulek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Mar 30, 2023 • edited Loading

dulek commented Mar 31, 2023

mdbooth commented Apr 3, 2023

mdbooth commented Apr 3, 2023

k8s-infra-cherrypick-robot commented Apr 3, 2023

mdbooth commented Apr 3, 2023

dulek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 4, 2023

dulek commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Apr 6, 2023

mdbooth commented Apr 17, 2023

dulek commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Apr 18, 2023

k8s-infra-cherrypick-robot commented Apr 18, 2023

mdbooth commented Mar 29, 2023 •

edited

Loading

netlify bot commented Mar 29, 2023 •

edited

Loading

mdbooth Mar 30, 2023 •

edited

Loading

mdbooth commented Mar 30, 2023 •

edited

Loading