🏃 Refactor all network client calls into networking package #950

macaptain · 2021-07-22T14:04:59Z

This refactors all networking client behavior into the networking package. The network client has been removed from the compute Service struct, instead, we use the networking Service. Ports and trunking logic have been moved out of instance.go in the compute package into port.go and trunk.go in the networking package.

The process for refactoring is simple enough:

Delete network client from compute package
Move any functions using the network client to the networking package
Refactor the functions to call the networking client interface instead of gophercloud directly

What this PR does / why we need it:
This will allow us to write unit tests for ports and trunk functionality using the mocks in the networking package. It also considerably tidies up instance.go, since now all networking client calls are made in the networking package only.

Which issue(s) this PR fixes:
Continuing work on #843, finishes #883 after #935 was merged.

Special notes for your reviewer:
This is going to cause some rebasing to be required on open PRs around trunks and port security features, namely #934, #914, #921, but will at least allow these features to have unit tests written for them. I think the rebase should be straightforward, the getOrCreatePort function in instance.go has simply moved to GetOrCreatePort in the networking package. Alternatively, I don't mind waiting and rebasing this PR if those features are ready to go in.

PR is ready to go!

squashed commits

/hold

k8s-ci-robot · 2021-07-22T14:05:08Z

Hi @macaptain. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

macaptain · 2021-07-22T14:07:29Z

Hi @hidekazuna, thanks for reviewing and merging #935. I saw you closed #883. Hopefully this PR does the same thing after #935.

/assign @hidekazuna

jichenjc · 2021-07-23T02:38:23Z

/ok-to-test

jichenjc · 2021-07-28T04:19:30Z

I am not sure how big trouble this can cause to other ongoing PR
maybe we need let those functional PRs merge then do the refactory all-in-one..
ok to any suggestions instead

macaptain · 2021-07-28T06:21:12Z

I am not sure how big trouble this can cause to other ongoing PR
maybe we need let those functional PRs merge then do the refactory all-in-one..
ok to any suggestions instead

I agree with this, there is a lot of discussion on those other PRs and I don't want to complicate them any further. I am happy to hold the refactor until they are merged.

hidekazuna · 2021-07-30T04:22:34Z

I agree with this, there is a lot of discussion on those other PRs and I don't want to complicate them any further. I am happy to hold the refactor until they are merged.

@macaptain Sorry late. Thank you for taking my PR into account. This PR looks fine for me as of now. It is OK to wait for other PRs not to bother them.

macaptain · 2021-08-06T12:30:38Z

Now I'm just waiting for #934 to be merged and then I'll rebase and this PR will be ready for review again.

hidekazuna · 2021-09-08T08:56:10Z

@macaptain Now that #934 has been merged, rebase please, if you do not mind.

This refactors all networking client behavior into the networking package. The network client has been removed from the compute Service struct, instead, we use the networking Service. Ports and trunking logic have been moved out of instance.go in the compute package into port.go and trunk.go in the networking package. The process for refactoring is simple enough: 1. Delete network client from compute package 2. Move any functions using the network client to the networking package 3. Refactor the functions to call the networking client interface instead of gophercloud directly

macaptain · 2021-09-09T13:09:40Z

pkg/cloud/services/networking/port.go

+
+// GetPortFromInstanceIP returns at most one port attached to the instance with given ID
+// and with the IP address provided.
+func (s *Service) GetPortFromInstanceIP(instanceID string, ip string) ([]ports.Port, error) {


This is the only function I had to create (to avoid a circular dependency with GetManagementPort). All other functions in this file are moved from instance.go.

This looks fine to me.

macaptain · 2021-09-09T13:18:10Z

@macaptain Now that #934 has been merged, rebase please, if you do not mind.

@hidekazuna - I've rebased and this is ready for review again. Although the diff is quite large, the steps to make this change were not so complicated (they are in the PR description). I think the side-by-side diff will show what has moved. Thanks!

hidekazuna · 2021-09-13T01:27:58Z

@macaptain Now that #934 has been merged, rebase please, if you do not mind.

@hidekazuna - I've rebased and this is ready for review again. Although the diff is quite large, the steps to make this change were not so complicated (they are in the PR description). I think the side-by-side diff will show what has moved. Thanks!

@macaptain Thank s rebasing and advising to review. I will review in a few days.

hidekazuna · 2021-09-14T00:20:35Z

/approve

hidekazuna · 2021-09-14T00:21:21Z

@jichenjc PTAL

jichenjc · 2021-09-14T00:46:10Z

I see some issue reported #993
which likely to be a regression of #935 and this PR seems to be a follow up of #935

not sure whether this PR will make things worse or has conflict to the proposed fix of #993
need more time to read the code and maybe @macaptain @mdbooth can help us in understand more

before that, I Think how about let's hold this?

hidekazuna · 2021-09-14T02:06:21Z

before that, I Think how about let's hold this?

@jichenjc I agree to hold this PR. revisit this PR after #993 fixed.

mdbooth · 2021-09-14T08:34:29Z

I haven't yet reviewed properly, but in general I'm very much in favour of continuing the refactor started by #935. The regression it introduced was caused by 2 very minor additions (checks for a zero length result) to a couple of networking functions which weren't strictly part of the refactor. However, it enables a pattern of unit testing which we can use to catch exactly this kind of bug in the future. I would like to see the same refactor in the compute package.

I suspect this change is going to conflict with everything, so I'd personally be in favour of merging it asap. I will try to review today.

mdbooth

This mostly looks good to me. Just some minor issues which should be addressed before merging. Nits and bikesheds are up to you 😉

I would personally be in favour of merging this quickly despite the merge conflicts it will inevitably cause.

mdbooth · 2021-09-14T08:43:44Z

pkg/cloud/services/networking/port.go

+
+// GetPortFromInstanceIP returns at most one port attached to the instance with given ID
+// and with the IP address provided.
+func (s *Service) GetPortFromInstanceIP(instanceID string, ip string) ([]ports.Port, error) {


This looks fine to me.

mdbooth · 2021-09-14T09:01:51Z

pkg/cloud/services/networking/client.go

 }

 func (c networkClient) DeletePort(id string) error {
-	return ports.Delete(c.serviceClient, id).ExtractErr()
+	mc := metrics.NewMetricPrometheusContext("port", "delete")
+	return mc.ObserveRequest(ports.Delete(c.serviceClient, id).ExtractErr())


This would be a regression because we're now logging an error in prometheus on delete of an object which has already been deleted where we previously were not. I think we need to make this, and all Delete* methods in this interface, call ObserveRequestsIgnoreNotFound.

We have the same issue in all Delete* methods.

mdbooth · 2021-09-14T09:06:09Z

pkg/cloud/services/networking/trunk.go

+	retryIntervalTrunkDelete = 5 * time.Second
+)
+
+func (s *Service) GetTrunkSupport() (bool, error) {


Aside: I wonder how expensive this call is. We call it on every create and delete, but it doesn't seem like it's ever going to change. And if it did change, would it break inter-op with servers created before the change? What if it changed between create and delete? 🤔

Not for this PR!

mdbooth · 2021-09-14T09:16:07Z

pkg/cloud/services/networking/client.go

+	return mc.ObserveRequest(trunks.Delete(c.serviceClient, id).ExtractErr())
+}
+
+func (c networkClient) ListTrunk(opts trunks.ListOptsBuilder) ([]trunks.Trunk, error) {


Bikeshed: This should be plural: ListTrunks (along with all the other List* methods).

Leave it for now unless you want to change all the others in a separate commit, and it's obviously not super important! It does make me wince, though 😉

mdbooth · 2021-09-14T09:26:25Z

pkg/cloud/services/networking/trunk.go

+}
+
+func (s *Service) replaceAllAttributesTags(eventObject runtime.Object, trunkID string, tags []string) error {
+	mc := metrics.NewMetricPrometheusContext("trunk", "update")


This is inconsistent. We should move this prometheus logging into the client, consistent with all the other logging.

That is going to inevitably change the logging from "trunk", "update" to something like "attributetags", "update" 🤔 I personally think that's ok, certainly less serious than the difference between logging errors and not logging errors on delete. Interested in other opinions, though.

mdbooth · 2021-09-14T09:38:45Z

pkg/cloud/services/networking/port.go

+	return nil
+}
+
+func (s *Service) DeletePorts(eventObject runtime.Object, nets []servers.Network) error {


nit: I personally wouldn't have moved this method. Its function is to iterate over servers.Network, which is a compute thing. Honestly I'd inline it in createInstance.

mdbooth · 2021-09-14T09:42:45Z

pkg/cloud/services/networking/port.go

+
+func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string) error {
+	portList, err := s.client.ListPort(ports.ListOpts{
+		Name: instanceName,


Not for this PR, but I'm pretty sure this is broken. This is listing ports which have the same name as the instance, but we don't create ports with the same name as the instance. I wonder if we should be deleting these by DeviceID instead 🤔

NOT FOR THIS PR!

mdbooth · 2021-09-14T09:44:32Z

pkg/cloud/services/compute/service.go

@@ -33,7 +33,6 @@ type Service struct {
 	projectID         string
 	computeClient     *gophercloud.ServiceClient
 	identityClient    *gophercloud.ServiceClient
-	networkClient     *gophercloud.ServiceClient


mdbooth · 2021-09-14T09:46:50Z

pkg/cloud/services/compute/instance.go

@@ -211,7 +196,7 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 	}

 	if instanceSpec.Subnet != "" && accessIPv4 == "" {
-		if err := s.deletePorts(eventObject, portList); err != nil {
+		if err := s.networkingService.DeletePorts(eventObject, portList); err != nil {


nit: As mentioned elsewhere, I would personally have left this method here.

jichenjc · 2021-09-15T01:36:55Z

/approve
/hold cancel

ok, let's merge this given agreement and clearance of regression issue
and for all the nits, I guess we have chance to submit follow up PRs

jichenjc · 2021-09-15T01:37:03Z

/lgtm

k8s-ci-robot · 2021-09-15T01:37:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, jichenjc, macaptain

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,jichenjc]

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

macaptain

Thanks for the thoughtful review @mdbooth ! I'll be addressing your points in follow-up PRs. I've raised #997 for making the metrics usage consistent.

@jichenjc - thanks for merging. This refactor means it's now possible to write unit tests for some of the network and ports logic, which will make this logic easier to maintain in future.

pkg/cloud/services/networking/port.go

+	return nil
+}
+
+func (s *Service) DeletePorts(eventObject runtime.Object, nets []servers.Network) error {


pkg/cloud/services/networking/client.go

+	return mc.ObserveRequest(trunks.Delete(c.serviceClient, id).ExtractErr())
+}
+
+func (c networkClient) ListTrunk(opts trunks.ListOptsBuilder) ([]trunks.Trunk, error) {


pkg/cloud/services/compute/instance.go

@@ -211,7 +196,7 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 	}

 	if instanceSpec.Subnet != "" && accessIPv4 == "" {
-		if err := s.deletePorts(eventObject, portList); err != nil {
+		if err := s.networkingService.DeletePorts(eventObject, portList); err != nil {


pkg/cloud/services/networking/trunk.go

+}
+
+func (s *Service) replaceAllAttributesTags(eventObject runtime.Object, trunkID string, tags []string) error {
+	mc := metrics.NewMetricPrometheusContext("trunk", "update")


pkg/cloud/services/networking/client.go

 }

 func (c networkClient) DeletePort(id string) error {
-	return ports.Delete(c.serviceClient, id).ExtractErr()
+	mc := metrics.NewMetricPrometheusContext("port", "delete")
+	return mc.ObserveRequest(ports.Delete(c.serviceClient, id).ExtractErr())


pkg/cloud/services/networking/port.go

+
+func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string) error {
+	portList, err := s.client.ListPort(ports.ListOpts{
+		Name: instanceName,


pkg/cloud/services/networking/trunk.go

+	retryIntervalTrunkDelete = 5 * time.Second
+)
+
+func (s *Service) GetTrunkSupport() (bool, error) {


macaptain · 2021-09-15T07:13:44Z

@jichenjc oops, I managed to hide all my comments in response to @mdbooth 😆. I don't have permission to unhide them, so if you can unhide for me, that would be great.

jichenjc · 2021-09-15T07:16:42Z

@macaptain I don't know how to unhide them :(
@hidekazuna do you know ?

Following up on kubernetes-sigs#950, this moves the deletePorts function back to the compute package where it can be made a private function.

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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2021

k8s-ci-robot requested review from prankul88 and sbueringer July 22, 2021 14:05

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 22, 2021

k8s-ci-robot assigned hidekazuna Jul 22, 2021

macaptain changed the title ~~🏃 Refactor all networking into networking package~~ 🏃 Refactor all network client calls into networking package Jul 22, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2021

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

macaptain force-pushed the move-ports-to-networking branch from d4f968c to 94d5f53 Compare September 9, 2021 07:41

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

macaptain force-pushed the move-ports-to-networking branch from 94d5f53 to 7910b69 Compare September 9, 2021 12:01

macaptain force-pushed the move-ports-to-networking branch from 7910b69 to bd0e3cb Compare September 9, 2021 13:06

macaptain commented Sep 9, 2021

View reviewed changes

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

mdbooth requested changes Sep 14, 2021

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 Sep 15, 2021

k8s-ci-robot assigned jichenjc Sep 15, 2021

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

k8s-ci-robot merged commit c773ca0 into kubernetes-sigs:master Sep 15, 2021

macaptain commented Sep 15, 2021

View reviewed changes

macaptain deleted the move-ports-to-networking branch September 15, 2021 07:10

This was referenced Sep 21, 2021

🏃 Move deletePorts back to compute package #1002

Merged

🏃 Standardize metrics in networking package #1003

Merged

macaptain mentioned this pull request Oct 6, 2021

REQUEST: New membership for macaptain kubernetes/org#3030

Closed

7 tasks

mdbooth mentioned this pull request Dec 13, 2021

REQUEST: New membership for mdbooth kubernetes/org#3142

Closed

7 tasks

🏃 Refactor all network client calls into networking package #950

🏃 Refactor all network client calls into networking package #950

Conversation

macaptain commented Jul 22, 2021 • edited Loading

k8s-ci-robot commented Jul 22, 2021

macaptain commented Jul 22, 2021

jichenjc commented Jul 23, 2021

jichenjc commented Jul 28, 2021

macaptain commented Jul 28, 2021

hidekazuna commented Jul 30, 2021

macaptain commented Aug 6, 2021

hidekazuna commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macaptain commented Sep 9, 2021

hidekazuna commented Sep 13, 2021

hidekazuna commented Sep 14, 2021

hidekazuna commented Sep 14, 2021

jichenjc commented Sep 14, 2021

hidekazuna commented Sep 14, 2021 • edited Loading

mdbooth commented Sep 14, 2021

mdbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

jichenjc commented Sep 15, 2021

jichenjc commented Sep 15, 2021

k8s-ci-robot commented Sep 15, 2021

macaptain left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

macaptain commented Sep 15, 2021

jichenjc commented Sep 15, 2021

macaptain commented Jul 22, 2021 •

edited

Loading

hidekazuna commented Sep 14, 2021 •

edited

Loading