Skip to content

Neutron v2: ScheduleBGPSpeakerOpts, RemoveBGPSpeaker, Lis…#2417

Merged
mandre merged 4 commits intogophercloud:masterfrom
shhgs:2022-05-30
Sep 8, 2022
Merged

Neutron v2: ScheduleBGPSpeakerOpts, RemoveBGPSpeaker, Lis…#2417
mandre merged 4 commits intogophercloud:masterfrom
shhgs:2022-05-30

Conversation

@shhgs
Copy link
Copy Markdown
Contributor

@shhgs shhgs commented Jun 1, 2022

Neutron V2: BGP Dynamic Routing
For #2208

$ git diff --stat official/master                                                                                                                     
 acceptance/openstack/networking/v2/extensions/agents/agents_test.go | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++                                                                                    
 openstack/networking/v2/extensions/agents/doc.go                    | 29 +++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/agents/requests.go               | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/agents/results.go                | 14 ++++++++++++++
 openstack/networking/v2/extensions/agents/testing/fixtures.go       | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/agents/testing/requests_test.go  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 openstack/networking/v2/extensions/agents/urls.go                   | 15 +++++++++++++++
 7 files changed, 330 insertions(+)

@shhgs
Copy link
Copy Markdown
Contributor Author

shhgs commented Jun 1, 2022 •
edited
Loading

  1. agents.ListDRAgentHostingBGPSpeakers would GET /v2.0/bgp-speakers/{bgp-speaker-id}/bgp-dragents, but I have to put it in agents. Because had I put it in bgp/speakers that would cause cyclic dependency.
  2. On our testing environment, when we create a BGP Speaker, it would be assigned to a BGP agent instantaneously. Therefore in the acceptance test, I would do ListDRAgentHostingBGPSpeakers first and use that agent to test Add/Remove BGP Agent.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 1, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 79.991% when pulling 3721112 on shhgs:2022-05-30 into 0ab2b5d on gophercloud:master.

mandre
mandre reviewed Jun 26, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the small comments I had in the acceptance tests, this LGTM.

For anyone looking for the documentation, it's missing from https://docs.openstack.org/api-ref/network/v2/index.html#bgp-dynamic-routing, however it's still on https://docs.openstack.org/neutron-dynamic-routing/wallaby/reference/index.html and the API seemingly has not changed.

@shhgs shhgs force-pushed the 2022-05-30 branch 5 times, most recently from 184af9e to e663c99 Compare September 7, 2022 19:08
@shhgs shhgs force-pushed the 2022-05-30 branch 2 times, most recently from 1fce9ba to 65556ca Compare September 7, 2022 20:30
mandre
mandre reviewed Sep 8, 2022
View reviewed changes
openstack/networking/v2/extensions/agents/urls.go Outdated

// return /v2.0/bgp-speakers/{bgp-speaker-id}/bgp-dragents
func listDRAgentHostingBGPSpeakersURL(c *gophercloud.ServiceClient, speakerID string) string {
return c.ServiceURL("bgp-speakers", speakerID, "bgp-dragents")
Copy link
Copy Markdown
Contributor

@mandre mandre Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make "bgp-speakers" and "bgp-dragents" constants.

acceptance/openstack/networking/v2/extensions/agents/agents_test.go Outdated
}

func TestBGPAgentRUD(t *testing.T) {
waitTime := 30 * time.Second
Copy link
Copy Markdown
Contributor

@mandre mandre Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a WaitFor() helper function if you need to wait for a certain condition to be satisfied. You can use it instead of the time.Sleep() calls you've added to this test.

Copy link
Copy Markdown
Contributor Author

@shhgs shhgs Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really tricky. waitFor can only wait 10 minutes, that's too much. On the other hand, waitForTimeout might be better, however this is not a timeout problem.

Copy link
Copy Markdown
Contributor

@mandre mandre Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitFor waits for a maximum of 10 min, and errors out if the predicate couldn't be satisfied in these 10 mins. Hopefully it returns much faster than the 30 seconds you initially had in the test.

@shhgs shhgs force-pushed the 2022-05-30 branch 2 times, most recently from 3f456b6 to 43e329d Compare September 8, 2022 14:38
mandre
mandre approved these changes Sep 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@mandre mandre merged commit a75271e into gophercloud:master Sep 8, 2022
@pierreprinetti pierreprinetti added this to the v1.1.0 milestone Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mandre mandre mandre approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@shhgs @coveralls @mandre @pierreprinetti