-
Notifications
You must be signed in to change notification settings - Fork 524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Staskraev/l3 agent scheduler #2501
Staskraev/l3 agent scheduler #2501
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to be explicit:
err = agents.ScheduleL3Router(neutron, agentID, agents.ScheduleL3RouterOpts{routerID}).ExtractErr() | |
err = agents.ScheduleL3Router(neutron, agentID, agents.ScheduleL3RouterOpts{RouterID: routerID}).ExtractErr() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
func listDHCPNetworksURL(c *gophercloud.ServiceClient, id string) string { | ||
return dhcpNetworksURL(c, id) | ||
} | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover comment?
Both listL3RoutersUR
L and listDHCPNetworksURL
look good to me. What was the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a AssertIntGreaterOrEqual() helper function you could use for comparing values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
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.