Staskraev/l3 agent scheduler#2501
Conversation
There was a problem hiding this comment.
Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.
mandre
left a comment
There was a problem hiding this comment.
Nice, thanks a lot for the PR. It looks good to me in general. I've left a couple of comments. This is almost good to merge.
|
|
||
| agentID := "0e1095ae-6f36-40f3-8322-8e1c9a5e68ca" | ||
| routerID := "e6fa0457-efc2-491d-ac12-17ab60417efd" | ||
| err = agents.RemoveL3Router(neutron, "0e1095ae-6f36-40f3-8322-8e1c9a5e68ca", "e6fa0457-efc2-491d-ac12-17ab60417efd").ExtractErr() |
There was a problem hiding this comment.
Was the intention of defining the agentID and routerID variables to use them here?
| err = agents.RemoveL3Router(neutron, "0e1095ae-6f36-40f3-8322-8e1c9a5e68ca", "e6fa0457-efc2-491d-ac12-17ab60417efd").ExtractErr() | |
| err = agents.RemoveL3Router(neutron, agentID, routerID).ExtractErr() |
|
|
||
| agentID := "0e1095ae-6f36-40f3-8322-8e1c9a5e68ca" | ||
| routerID := "e6fa0457-efc2-491d-ac12-17ab60417efd" | ||
| err = agents.ScheduleL3Router(neutron, agentID, agents.ScheduleL3RouterOpts{routerID}).ExtractErr() |
There was a problem hiding this comment.
Better to be explicit:
| err = agents.ScheduleL3Router(neutron, agentID, agents.ScheduleL3RouterOpts{routerID}).ExtractErr() | |
| err = agents.ScheduleL3Router(neutron, agentID, agents.ScheduleL3RouterOpts{RouterID: routerID}).ExtractErr() |
|
|
||
| func listL3RoutersURL(c *gophercloud.ServiceClient, id string) string { | ||
| // TODO | ||
| // hmm list should be the plain l3RoutersURL but dhcp example tell otherwise |
There was a problem hiding this comment.
Leftover comment?
Both listL3RoutersURL and listDHCPNetworksURL look good to me. What was the issue?
There was a problem hiding this comment.
leftover from original research
| th.AssertNoErr(t, err) | ||
| hostingAgents, err := routers.ExtractL3Agents(allPages) | ||
| th.AssertNoErr(t, err) | ||
| th.AssertEquals(t, len(hostingAgents) > 0, true) |
There was a problem hiding this comment.
There's a AssertIntGreaterOrEqual() helper function you could use for comparing values.
mandre
left a comment
There was a problem hiding this comment.
Just an off by 1 error to fix, then I think it's ready to merge. Thanks.
acceptance/openstack/networking/v2/extensions/layer3/l3_scheduling_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Martin André <martin.andre@gmail.com>
Fixes #2500
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
remove_router_from_l3_agent
list_routers_on_l3_agent
add_router_to_l3_agent
Acceptance test can't be run in CI because necessary extension is disabled, I am able to run it locally just fine.