Add ListBGPSpeakers by shhgs · Pull Request #2228 · gophercloud/gophercloud

shhgs · 2021-10-05T14:07:04Z

I isolated the ListBGPSpeakers and create a new PR. The issue number is #[2207]

I created a new branch, whose url is
[https://github.com/shhgs/gophercloud/tree/PR-2228]

10:02 $ git diff --stat official/master                                                                                                                                                                             
 openstack/networking/v2/extensions/agents/doc.go                   | 15 +++++++++++++++                                                                                                                           
 openstack/networking/v2/extensions/agents/requests.go              | 10 ++++++++++
 openstack/networking/v2/extensions/agents/testing/fixtures.go      | 25 +++++++++++++++++++++++++
 openstack/networking/v2/extensions/agents/testing/requests_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/agents/urls.go                  |  6 ++++++
 openstack/networking/v2/extensions/bgp/speaker/results.go          | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 187 insertions(+)

theopenlab-ci · 2021-10-05T15:18:14Z

Build failed.

gophercloud-unittest : POST_FAILURE in 5m 11s
gophercloud-acceptance-test : FAILURE in 53m 44s
gophercloud-acceptance-test-ironic : SUCCESS in 1h 06m 49s

shhgs · 2021-10-05T17:22:52Z

$ curl -s  -o - https://logs.openlabtesting.org/logs/28/2228/508ee6a0dda499447e11f5cdc4c2facb71939702/check/gophercloud-acceptance-test/a21b824/job-output.json.gz | gunzip - | sed -n 5559,5566p | xargs -I LINE printf "%s\n" LINE
+ echo 'Running devstack',
Running devstack,
+ echo '... this takes 10 - 15 minutes (logs in logs/devstacklog.txt.gz)',
... this takes 10 - 15 minutes (logs in logs/devstacklog.txt.gz),
++ date +%s,
+ start=1633443531,
+ /tmp/ansible/bin/ansible primary -f 5 -i /home/zuul/workspace/inventory -m shell -a 'cd '\\''/opt/stack/new/devstack'\\'' && sudo -H -u stack DSTOOLS_VERSION=0.4.0 stdbuf -oL -eL ./stack.sh 2>&1 executable=/bin/bash',
ERROR: the main setup script run by this job failed - exit code: 2,

jtopjian · 2021-10-05T18:07:33Z

@shhgs Can you please provide links to the the service-side API code that defines this extension? This section of the tutorial will explain how to do that.

shhgs · 2021-10-05T18:49:19Z

@jtopjian Yeah, the ListBGPSpeakers API is NOT included in the neutron doc. It was documented in the policy doc.

Acutally I noticed there's another mistakes in the official doc. /v2.0/bgp-speakers/{bgp-speaker-id}/add-bgp-peer. Actually the endpoint is /bgp-speakers/{id}/add_bgp_peer.

shhgs · 2021-10-05T18:56:19Z

os-ne1-lab:~$ curl -s -o - -H "X-Auth-Token: $TOKEN" ${ENDPOINT}/v2.0/agents/86697938-2a32-4bfa-974d-e098bdbfc334/bgp-drinstances/
{"bgp_speakers": []}

It also works on our prod env, where we do have speakers.

jtopjian

@shhgs I've done a quick scan of the code and left some comments. I'm happy to discuss more in detail.

jtopjian · 2021-10-05T18:30:01Z

openstack/networking/v2/extensions/agents/doc.go

 		panic(err)
 	}

+Example to List bgp speakers by dragent


Nit: BGP instead of bgp

Sure, I would fix it.

jtopjian · 2021-10-05T18:53:00Z

openstack/networking/v2/extensions/agents/requests.go

+
+// List the uuid of the BGP Speakers hosted by a specific dragent
+// GET /v2.0/agents/{agent-id}/bgp-drinstances
+func ListBGPSpeakers(c *gophercloud.ServiceClient, agentID string) pagination.Pager {


This is a little complicated.

Generally, there isn't anywhere else in Gophercloud that integrates packages unless it's to re-use a result struct.

This agents package implements some DHCP functions, but that was because the result was only a list of networks. Perhaps that was a poor decision years ago as it may have caused confusion here.

Using your doc example above:

pages, err := agents.ListBGPSpeakers(c, agentID).AllPages() if err != nil { log.Panic(err) } allSpeakers, err := speaker.ExtractBGPSpeakers(pages) if err != nil { log.Panic(err) }

There isn't anywhere in Gophercloud that uses one package to retrieve a dataset and another package to extract it.

If this ListBGPSpeakers was contained in the forthcoming bgp/speaker (which should be bgp/speakers) package, then the example code now looks like:

pages, err := speakers.ListByDynamicRoutingAgent(c, agentID).AllPages() if err != nil { log.Panic(err) } allSpeakers, err := speakers.ExtractBGPSpeakers(pages) if err != nil { log.Panic(err) }

which provides a more consistent and intuitive workflow.

The bulk of the Dynamic Routing specific code can also be moved out of the agents package and into the bgp/speakers package. The only tradeoff is that a URL in the bgp/speakers package has the word agents in it.

I recommend trying this out and seeing how it looks.

Thoughts?

ListBGPSpeakers is very similar to ListDHCPNetworks. The only difference here is it use pagination to return the result. Actually the first implementation was to directly return a list of BGP speakers. But during our internal review, we find the package README says:

All requests that enumerate a collection return a Pager struct that is used to iterate through the results one page at a time.

Personally speaking, I don't like pagination in this case. A BGP agent would not have more than a handful of speakers, there's no point to force the users to write more code. And not to say it contracts with the ListDHCPNetworks. I would be very happy to change it back if you like.

ListBGPSpeakers is very similar to ListDHCPNetworks. The only difference here is it use pagination to return the result.

Actually, it's more involved. The DHCP stuff in the agents package includes everything including the Extract method on the result:

https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/extensions/agents/results.go#L161-L175

This way, both the function call and the extract are housed within the same package.

The way the ListBGPSpeakers is implemented now, one is implemented in agents and one is implemented in speaker which is inconsistent.

This is why I mentioned that this is complicated 🙂

Putting my Past @jtopjian hat on, I probably thought "well the Extract method is small, so it can just be in the agents package" which might have been a bad decision for long-term design.

I know it's a lot of work on your side, but I suggest testing with this branch and moving all of this to the speaker package and you'll see how everything (the function, the result, the test fixtures etc) should fit nicely. The only oddity being that the urls.go file will have the word agents in it which might feel out of place.

I could be wrong about this, so please do push back if I'm missing something.

There's no perfect, clean solution here. This is an API extension that feels like it attached itself to an existing URL route.

Personally speaking, I don't like pagination in this case.

Regarding paging, there is a SinglePageBase that helps with this situation.

jtopjian · 2021-10-05T18:54:22Z

openstack/networking/v2/extensions/bgp/speaker/results.go

@@ -0,0 +1,88 @@
+package speaker


nit: speakers

Hmmm. My idea is we have different types of agents, we have dhcp/bgp/l3/ovs ..., but we have only one type of BGP speaker and BGP peer. In this case I would suggest not to change it.

jtopjian · 2021-10-05T18:54:34Z

openstack/networking/v2/extensions/bgp/speaker/results.go

+	"github.com/gophercloud/gophercloud/pagination"
+)
+
+const jroot = "bgp_speaker"


This appears to be unused?

This is a bug in the isolation. It will be used in the future. ;-)

I will remove it for now.

jtopjian · 2021-10-05T18:55:13Z

openstack/networking/v2/extensions/bgp/speaker/results.go

+	Peers []string `json:"peers"`
+}
+
+func (n *BGPSpeaker) UnmarshalJSON(b []byte) error {


I don't think we need a custom UnmarshalJSON method here. It looks like everything can be handled by the default unmarshaller.

Try removing this method completely - the code should still work.

Yeah, it is not required for now. But once we have all the features added, we have to add it back.

I can remove it for now.

jtopjian · 2021-10-05T18:59:40Z

openstack/networking/v2/extensions/bgp/speaker/results.go

+	pagination.LinkedPageBase
+}
+
+// OpenStack API always return one page


If only one page is returned, then you can use a SinglePageBase:

gophercloud/openstack/compute/v2/flavors/results.go

Lines 140 to 143 in 5137346

// AccessPage contains a single page of all FlavorAccess entries for a flavor.

type AccessPage struct {

pagination.SinglePageBase

}

Sure, I would change the code.

jtopjian · 2021-10-05T20:15:42Z

Yeah, the ListBGPSpeakers API is NOT included in the neutron doc.

Just to clarify: the docs are helpful, but we're looking for the actual Neutron code. Part 3 of the contributor tutorial has a lot of details about this - let me know if it's confusing in any way so I can get it cleared up.

In the case of this PR, here are the kinds of links that are relevant:

Acutally I noticed there's another mistakes in the official doc. /v2.0/bgp-speakers/{bgp-speaker-id}/add-bgp-peer. Actually the endpoint is /bgp-speakers/{id}/add_bgp_peer.

This is exactly why the code is more important than the docs. That's not to say the docs are useless and no criticism to the people who tirelessly work on them, but when it comes to writing API bindings, the code is a better reference:

https://github.com/openstack/neutron-dynamic-routing/blob/28c83208af039a18781b05c9467fd1b0f70a73b0/neutron_dynamic_routing/policies/bgp_speaker.py#L68-L78

Add ListBGPSpeakers

508ee6a

shhgs closed this Oct 5, 2021

shhgs deleted the PR-2227 branch October 5, 2021 14:07

shhgs restored the PR-2227 branch October 5, 2021 14:09

shhgs reopened this Oct 5, 2021

jtopjian requested changes Oct 5, 2021

View reviewed changes

shhgs closed this Oct 6, 2021

shhgs mentioned this pull request Oct 7, 2021

Add ListBGPSpeakers (continued) #2229

Merged

	// AccessPage contains a single page of all FlavorAccess entries for a flavor.
	type AccessPage struct {
	pagination.SinglePageBase
	}

Conversation

shhgs commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Oct 5, 2021

Uh oh!

shhgs commented Oct 5, 2021

Uh oh!

jtopjian commented Oct 5, 2021

Uh oh!

shhgs commented Oct 5, 2021

Uh oh!

shhgs commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtopjian commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shhgs commented Oct 5, 2021 •

edited

Loading

shhgs commented Oct 5, 2021 •

edited

Loading