🐛 Don't make unnecessary REST API calls in getServerNetworks #994

mdbooth · 2021-09-13T16:21:57Z

What this PR does / why we need it:

If we specify a network with a subnet filter but no network filter, we now execute a single subnet query rather than executing one for every network. We also no longer execute queries at all when the user has specified a uuid explicitly.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #993

Special notes for your reviewer:

This PR is intentionally structured in 2 commits:

The first adds unit tests for getServerNetworks (ironically using the excellent work in #935, which also happened to be the commit which introduced the regression). The second fixes getServerNetworks and makes the necessary changes to the unit tests.

The purpose of doing this is to highlight functional changes introduced by the second commit. These can be clearly seen in the unit test changes:

We make fewer gophercloud calls without changing the returned data
The target case no longer returns an error

Apart from the target case, we can see that the output of the test cases are unaffected.

netlify · 2021-09-13T16:22:04Z

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

🔨 Explore the source changes: 357e525

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/614b3c00a8f3b70008f6f850

😎 Browse the preview: https://deploy-preview-994--kubernetes-sigs-cluster-api-openstack.netlify.app

iamemilio · 2021-09-13T18:49:20Z

pkg/cloud/services/compute/instance.go

+			// Don't lookup subnet if it was specified explicitly by UUID
+			if subnet.UUID != "" {
+				addSubnet(netID, subnet.UUID)
+				continue


nit pick, IMO continue is just a more confusing way to write if {} else {}, but its a very minor nit pick

Yeah. Sometimes it's worth it if the following block is longer, but I agree an else block would have been clearer here.

I'll change this if I have to update the patch for any other reason.

I updated this.

iamemilio · 2021-09-14T17:53:07Z

overall, LGTM. Really appreciate the tests to cover this code.

tobiasgiese · 2021-09-15T06:32:00Z

pkg/cloud/services/compute/instance.go

+		for _, subnet := range networkParam.Subnets {
+			// Don't lookup subnet if it was specified explicitly by UUID
+			if subnet.UUID != "" {
+				addSubnet(netID, subnet.UUID)


I think you should validate if netID is also not an empty string.

Or don't we need the network ID here, as the subnet ID is sufficient?

You are right. There are 2 ways we can go here:

Fetch the subnet and therefore the network

Return a validation error

I'm inclined to return the error as this is most efficiently fixed by the caller.

/hold

Thinking out loud:

Q: How is this different to specifying a subnet filter without a network?
A: A UUID implies the caller already has the details of a specific resource. We are already trusting the caller if they specify a UUID (we use it without further validation): they will get a server create error instead if it was incorrect.

IOW if the caller specifies a network or subnet by UUID, we are explicitly delegating the validation of those UUIDs to the caller. If the caller specifies a filter we execute it for them and let them know if something was wrong (i.e. it was empty).

Is that a good enough argument? If so, should we codify it in the docs?

I think your assumption is correct. We should also document this behavior, I agree.
We don't have to make any code changes then. But it might be helpful to add a small comment. WDYT?

Thanks. I will:

update to return a validation here. If it's not sufficiently obvious from the validation error I'll also add a comment.

add a unit test to cover this error.

try to find some documentation to update.

On the last point, I want to make the following explicit in the docs:

The controller will not validate an explicit network or subnet UUID.

An invalid network or subnet UUID will result in a server create error.

When specifying subnet UUID, network UUID must also be specified.

mdbooth · 2021-09-21T08:38:26Z

/hold cancel
@tobiasgiese Is this better?

tobiasgiese · 2021-09-21T12:02:02Z

/hold cancel
@tobiasgiese Is this better?

That looks much better! Thanks :)

/lgtm

macaptain

The test cases are exhaustive so I can see what's going on and the implementation looks good to me. It's also useful from the tests to see that when specifying UUIDs for networks explicitly, then no OpenStack calls are made. (I expect if UUID is incorrect, then we fail at getOrCreatePort time.)

Something I was reminded of: when a network has multiple subnets and we choose that network with a filter that matches multiple subnets, we end up with a port on every subnet of that network, which is a little bit surprising to me, but it's the way this has always worked. It's good to see the tests cover that too.

macaptain · 2021-09-21T13:19:51Z

api/v1alpha4/types.go

 	UUID string `json:"uuid,omitempty"`

-	// Filters for optional network query
+	// Filters for optional subnet query


So you can search for a subnet by ID (your use case), you just have to do it in the SubnetFilter. Makes sense.

macaptain · 2021-09-22T12:11:19Z

pkg/cloud/services/compute/instance_test.go

+				t.Errorf("Service.getServerNetworks() error = %+v, wantErr %+v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {


Minor point / nit: I've noticed the tests don't use Gomega for pattern matching.

The Cluster API book suggests we should use Gomega for testing (https://cluster-api.sigs.k8s.io/developer/testing.html#gomega):

In Cluster API all the test MUST use Gomega assertions.

You get a bit nicer output on errors if you use Gomega. An example of how to use Gomega for matching test assertions is here:

cluster-api-provider-openstack/pkg/cloud/services/networking/floatingip_test.go

Lines 83 to 84 in 6aa7494

g.Expect(err).ShouldNot(HaveOccurred())

g.Expect(got).To(Equal(tt.want))

As it's trivial and MUST is in caps I updated it.

When specifying a subnet filter without a network filter we would first execute a network list with no filter, which returns all networks, then execute the subnet filter for each network. With this change we: * Don't filter subnets by network if there is no network filter * Don't execute a network query if we have a network uuid * Don't execute a subnet query if we have a subnet uuid The changes to unit tests are: * Don't expect gophercloud calls which we no longer make * "Subnet without network" now returns subnets instead of error

mdbooth · 2021-09-24T08:44:56Z

/test all

iamemilio · 2021-09-27T18:11:59Z

/lgtm

macaptain · 2021-09-28T18:21:19Z

lgtm

hidekazuna · 2021-09-29T01:06:41Z

/approve

k8s-ci-robot · 2021-09-29T01:06:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, macaptain, 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~~ [hidekazuna]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 13, 2021

k8s-ci-robot requested review from hidekazuna and sbueringer September 13, 2021 16:22

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2021

mdbooth force-pushed the emptynetworkfilter branch from 5007a9b to 85fe929 Compare September 13, 2021 16:26

iamemilio reviewed Sep 13, 2021

View reviewed changes

tobiasgiese reviewed Sep 15, 2021

View reviewed changes

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

mdbooth force-pushed the emptynetworkfilter branch 2 times, most recently from 1e51cf8 to 82780b2 Compare September 20, 2021 10:36

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

k8s-ci-robot assigned tobiasgiese Sep 21, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2021

macaptain approved these changes Sep 21, 2021

View reviewed changes

macaptain reviewed Sep 22, 2021

View reviewed changes

mdbooth added 2 commits September 22, 2021 14:17

Add unit tests for compute.getServerNetworks()

c8e4966

mdbooth force-pushed the emptynetworkfilter branch from 82780b2 to 357e525 Compare September 22, 2021 14:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2021

mdbooth mentioned this pull request Sep 24, 2021

✨ getOrCreatePort: add support to configure QoS profile ID on ports #1008

Closed

k8s-ci-robot assigned iamemilio Sep 27, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2021

k8s-ci-robot merged commit 180f320 into kubernetes-sigs:master Sep 29, 2021

mdbooth deleted the emptynetworkfilter branch September 29, 2021 08:29

mandre mentioned this pull request Sep 29, 2021

Add CI for openshift/mapo openshift/release#20896

Merged

mdbooth mentioned this pull request Oct 7, 2021

Consolidate Networks and Ports in OpenStackMachineSpec #1023

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Don't make unnecessary REST API calls in getServerNetworks #994

🐛 Don't make unnecessary REST API calls in getServerNetworks #994

mdbooth commented Sep 13, 2021

netlify bot commented Sep 13, 2021 •

edited

Loading

iamemilio Sep 13, 2021

mdbooth Sep 14, 2021

mdbooth Sep 22, 2021

iamemilio commented Sep 14, 2021 •

edited

Loading

tobiasgiese Sep 15, 2021

tobiasgiese Sep 15, 2021

mdbooth Sep 15, 2021

mdbooth Sep 15, 2021 •

edited

Loading

tobiasgiese Sep 15, 2021 •

edited

Loading

mdbooth Sep 15, 2021

mdbooth commented Sep 21, 2021

tobiasgiese commented Sep 21, 2021

macaptain left a comment •

edited

Loading

macaptain Sep 21, 2021

macaptain Sep 22, 2021 •

edited

Loading

mdbooth Sep 22, 2021

mdbooth commented Sep 24, 2021

iamemilio commented Sep 27, 2021

macaptain commented Sep 28, 2021

hidekazuna commented Sep 29, 2021

k8s-ci-robot commented Sep 29, 2021

	g.Expect(err).ShouldNot(HaveOccurred())
	g.Expect(got).To(Equal(tt.want))

🐛 Don't make unnecessary REST API calls in getServerNetworks #994

🐛 Don't make unnecessary REST API calls in getServerNetworks #994

Conversation

mdbooth commented Sep 13, 2021

netlify bot commented Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamemilio commented Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

tobiasgiese Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Sep 21, 2021

tobiasgiese commented Sep 21, 2021

macaptain left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macaptain Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Sep 24, 2021

iamemilio commented Sep 27, 2021

macaptain commented Sep 28, 2021

hidekazuna commented Sep 29, 2021

k8s-ci-robot commented Sep 29, 2021

netlify bot commented Sep 13, 2021 •

edited

Loading

iamemilio commented Sep 14, 2021 •

edited

Loading

mdbooth Sep 15, 2021 •

edited

Loading

tobiasgiese Sep 15, 2021 •

edited

Loading

macaptain left a comment •

edited

Loading

macaptain Sep 22, 2021 •

edited

Loading