-
Notifications
You must be signed in to change notification settings - Fork 22
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
[WIP] Add load balancer support. #218
base: master
Are you sure you want to change the base?
Conversation
This is still WIP because we have to find a way to not reparse all load balancers from the NB DB every time. Signed-off-by: Dumitru Ceara <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
@putnopvut It would be great if you could have a look at this, thanks! |
@@ -418,6 +418,11 @@ def setup_switch_per_node(self, fake_multinode_args = {}, | |||
lport_create_args = {}, | |||
port_bind_args = {}, | |||
create_mgmt_port = True): | |||
# TODO: figure out how to not reload the context? |
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.
@putnopvut This is the hack I was talking about.
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.
Can you go into a bit more detail about what's going on here? I've been away from this for long enough that I don't even know what it means to reload the context.
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.
self.context
gets populated once before any iteration of the scenario is run, in class OvnNorthboundContext.setup()
. Then, if I'm not wrong, each scenario gets a copy of the context. However, because we're updating load balancers in each iteration, ideally we would have to update the original context, such that upcoming iterations have the updated set of load balancers.
The rally runner doesn't allow that because iterations might be run in parallel, I presume.
To work around it I explicitly updated self.context["ovn-nb-lbs"] with the results of ovn_nbctl.lb_list()
.
tokens = line.split('=') | ||
if len(tokens) == 0: | ||
lb_name = None | ||
continue | ||
lb_vip = tokens[0] | ||
lb_backends = tokens[1] if len(tokens) == 2 else '' | ||
lbs[lb_name] = (lb_vip, lb_backends) | ||
lb_name = None | ||
|
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.
If the =
token is not found, then split()
will return a list containing the whole string as the first item. Therefore, len(tokens)
wont' ever be 0.
You can just replace this whole block with
lbs[lb_name] = tuple(line.split('=', 2))
lb_name = None
This will ensure that lbs[lb_name]
is set to a tuple of 2 items. If the =
is not present, then the second item of the tuple will be an empty string.
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.
Way better, will do, thanks!
self.run("lb-add", [], params) | ||
|
||
def lb_set_vip_backends(self, lb_name, lb_vip, lb_backends): | ||
vips_set='vips=\'\"{}\"=\"{}\"\''.format(lb_vip, lb_backends) |
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.
Nit, you can get rid of the \
before the double quotes. It might make the line a bit easier to read.
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.
Ack, thanks.
@@ -418,6 +418,11 @@ def setup_switch_per_node(self, fake_multinode_args = {}, | |||
lport_create_args = {}, | |||
port_bind_args = {}, | |||
create_mgmt_port = True): | |||
# TODO: figure out how to not reload the context? |
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.
Can you go into a bit more detail about what's going on here? I've been away from this for long enough that I don't even know what it means to reload the context.
No description provided.