Skip to content
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.

Sign up for GitHub

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

Check openflow(OF) rules on each hyperviors after binding lports #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Check openflow(OF) rules on each hyperviors after binding lports #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions rally_ovs/plugins/ovs/ovsclients.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@ def get_lswitch_info(info):
return lswitches


def get_of_in_port(of_port_list_raw, port_name_full):
# NOTE (huikang): the max length of portname shown in ovs-ofctl show is 15
port_name = port_name_full[0:15]
lines = of_port_list_raw.splitlines()
line = ""
for line in lines:
if (line.find(port_name) >= 0):
break
position = line.find("(")
return line[:position]


def set_colval_args(*col_values):
args = []
for entry in col_values:
Expand Down
42 changes: 34 additions & 8 deletions rally_ovs/plugins/ovs/ovsclients_impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
import pipes
import sys
import itertools
import pipes
import StringIO
from rally.common import logging
from rally_ovs.plugins.ovs.ovsclients import *
from rally_ovs.plugins.ovs.utils import get_ssh_from_credential

LOG = logging.getLogger(__name__)

@configure("ssh")
class SshClient(OvsClient):
Expand Down Expand Up @@ -167,7 +169,7 @@ def acl_del(self, lswitch):

def show(self, lswitch=None):
params = [lswitch] if lswitch else []
stdout = StringIO()
stdout = StringIO.StringIO()
self.run("show", args=params, stdout=stdout)
output = stdout.getvalue()

Expand Down Expand Up @@ -203,7 +205,7 @@ def set_sandbox(self, sandbox, install_method="sandbox"):
self.sandbox = sandbox
self.install_method = install_method

def run(self, cmd, opts=[], args=[]):
def run(self, ovs_cmd, cmd, opts=[], args=[], stdout=sys.stdout):
self.cmds = self.cmds or []

# TODO: tested with non batch_mode only for docker
Expand All @@ -213,16 +215,17 @@ def run(self, cmd, opts=[], args=[]):
if self.sandbox and self.batch_mode == False:
if self.install_method == "sandbox":
self.cmds.append(". %s/sandbox.rc" % self.sandbox)
cmd = itertools.chain(["ovs-vsctl"], opts, [cmd], args)
cmd = itertools.chain([ovs_cmd], opts, [cmd], args)
self.cmds.append(" ".join(cmd))
elif self.install_method == "docker":
self.cmds.append("sudo docker exec %s ovs-vsctl " % self.sandbox + cmd + " " + " ".join(args))
self.cmds.append("sudo docker exec %s " % self.sandbox + " "
+ ovs_cmd + " " + cmd + " " + " ".join(args))

if self.batch_mode:
return

self.ssh.run("\n".join(self.cmds),
stdout=sys.stdout, stderr=sys.stderr)
stdout=stdout, stderr=sys.stderr)

self.cmds = None

Expand All @@ -242,13 +245,36 @@ def flush(self):

def add_port(self, bridge, port, may_exist=True):
opts = ['--may-exist'] if may_exist else None
self.run('add-port', opts, [bridge, port])
self.run("ovs-vsctl", 'add-port', opts, [bridge, port])


def db_set(self, table, record, *col_values):
args = [table, record]
args += set_colval_args(*col_values)
self.run("set", args=args)
self.run("ovs-vsctl", "set", args=args)

def of_check(self, bridge, port, mac_addr, may_exist=True):
in_port = ""
stdout = StringIO.StringIO()
self.run("ovs-ofctl", "show br-int", stdout=stdout)
show_br_int_output = stdout.getvalue()
in_port = get_of_in_port(show_br_int_output, port)
LOG.info("Check port: in_port: %s; mac: %s" % (in_port.strip(), mac_addr))
appctl_cmd = " ofproto/trace br-int in_port=" + in_port.strip()
Copy link
Collaborator

@hzhou8 hzhou8 Aug 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If the purpose is to verify the binding operation finished and reflected in flows, how about simply: ofctl dump-flows br-int table=| grep

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hzhou8 thanks for reviewing the PR. You are right; binding operation could be verified by ofctl dump-flow. However, IMO, tracing a flow, which is used OVN tutorial, seems more accurate and straightforward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the flow trace seems too heavy and subject to change in the future. For example, when there are ACL rules, the programmed flows can also result in a drop action. So I think dump-flow for the ARP table is straightforward and simple. And less operations, e.g. no need to find out the ofport first.

appctl_cmd += ",dl_src=" + mac_addr
appctl_cmd += ",dl_dst=00:00:00:00:00:03 -generate "
self.run("ovs-appctl", appctl_cmd, stdout=stdout)
of_trace_output = stdout.getvalue()

# NOTE(HuiKang): if success, the flow goes through table 1 to table
# 32. However, since we use sandbox, table 32 seems not setup
# correctly. Therefore after table 34, the datapath action is
# Datapath actions: 100. If failed, the datapatch action is "drop"
for line in of_trace_output.splitlines():
if (line.find("Datapath actions") >= 0):
if (line.find("drop") >= 0):
return False
return True

def create_client(self):
print "********* call OvnNbctl.create_client"
Expand Down
40 changes: 40 additions & 0 deletions rally_ovs/plugins/ovs/scenarios/ovn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ def _create_lswitches(self, lswitch_create_args):

print("create lswitch")
self.RESOURCE_NAME_FORMAT = "lswitch_XXXXXX_XXXXXX"
self.port_mac = dict()

amount = lswitch_create_args.get("amount", 1)
batch = lswitch_create_args.get("batch", amount)
Expand Down Expand Up @@ -124,6 +125,7 @@ def _create_lports(self, lswitch, lport_create_args = [], lport_amount=1):
ovn_nbctl.enable_batch_mode()

base_mac = [i[:2] for i in self.task["uuid"].split('-')]
base_mac[0] = str(hex(int(base_mac[0], 16) & 254))
base_mac[3:] = ['00']*3


Expand All @@ -138,6 +140,7 @@ def _create_lports(self, lswitch, lport_create_args = [], lport_amount=1):

ovn_nbctl.lport_set_addresses(name, [mac, ip])
ovn_nbctl.lport_set_port_security(name, mac)
self.port_mac[name] = mac

lports.append(lport)

Expand Down Expand Up @@ -251,6 +254,43 @@ def _create_networks(self, network_create_args):
return lswitches


@atomic.action_timer("ovn_network.of_check_port")
def _of_check_ports(self, lports, sandboxes, port_bind_args):
Copy link
Collaborator

@hzhou8 hzhou8 Aug 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Could you write some comments about why, what and how is it checked?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why? In our OVN scalability test with dataplane (NOTE: not ovn-scale-test/sandbox), we found that some lport is not pingable after binding. After certain time period, the lport becomes pingable. One suspicious point is that the openflow rules after binding is not setup fast enough. Therefore checking "port state up" is insufficient.
What? In ovn-scale-test, since there is no real kernel module installed, the ping tool is not available. Therefore, we have to use OVS trace tool to verify the openflow rules.
How? Use the OVS built-in trace tool to verify the openflow rules are set correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, the purpose is to evaluate the end-to-end time spent in port-binding and flow installation in HVs, right? Yes, checking "port state up" is insufficient. But there are problems of checking flows on HVs:

  1. It is not accurate. The periodical poll have gaps.
  2. We need to check all related HVs (sandboxes), instead of only the ones doing port-binding. And because of this, this approach doesn't scale, because checking flows on so many sandboxes and collect the data would take time.

Because of this, we need a notification mechanism to accurately evaluate when all the HVs have finished processing the port-binding. It is handled by [1]. I believe it is a better approach, but I haven't worked on it yet.

[1] #97

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hzhou8 I agreed with both of your points; we should use the --wait hv [1] to measure the latency of binding lport. But I still have two questions:

  1. In Use ovn-nbctl --wait hv to get end-to-end port binding time #97, can we simply add --wait hv to all the nb-ctl command so that the time in rally-ovs results reflect the binding time from issuing the command to the OF rules installed on all involved hypervisors? Is this enough?
  2. Even with --wait hv, we somehow still want verify the correctness of the flows. As flow tracing is too heave as your mentioned, do you think I can implement it as a asynchronous thread? In other words, besides the main rally-ovs thread, a separate python thread is launched to trace the flow. What do you think? Thanks.

[1] #97

Copy link
Collaborator

Choose a reason for hiding this comment

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

@huikang
For question 1): Hopefully that simple change would work, but I haven't tried myself yet. I guess I won't have time this week, so it would be highly appreciated if you could try. Let me know if you'd like to take the ticket :)

For 2): I wonder if it is really needed to check correctness of flows in scale test. IMHO the focus here is the performance, and OVN functional tests should ensure correctness (the tests under OVS repo, and also openstack networking-ovn etc.). If we do suspect some scalability related bug, it may be helpful to have such kind of check AFTER the scale test complete. If we decide to implement such checks, we should make sure those checks won't impact the performance data. Instead of a separate thread running in parallel, I'd prefer a post-test validation. It should not be enabled by default because it would take too long considering the number of ports in scale test, and only enabled when we are testing correctness. Priority should not be high, though. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hzhou8 Thanks for your response.

  1. I am very happy to give the first shot and will try to submit a PR to Use ovn-nbctl --wait hv to get end-to-end port binding time #97 soon.
  2. Putting flow verification as an option in the post-test stage sounds good to me.

port_bind_args = port_bind_args or {}
wait_up = port_bind_args.get("wait_up", False)

sandbox_num = len(sandboxes)
lport_num = len(lports)
lport_per_sandbox = (lport_num + sandbox_num - 1) / sandbox_num

LOG.info("Checking OF lports method: %s" % self.install_method)
install_method = self.install_method

j = 0
for i in range(0, len(lports), lport_per_sandbox):
lport_slice = lports[i:i+lport_per_sandbox]

sandbox = sandboxes[j]["name"]
farm = sandboxes[j]["farm"]
ovs_vsctl = self.farm_clients(farm, "ovs-vsctl")
ovs_vsctl.set_sandbox(sandbox, install_method)
ovs_vsctl.enable_batch_mode()

for lport in lport_slice:
port_name = lport["name"]

LOG.info("of check %s to %s on %s" % (port_name, sandbox, farm))

# check if OF rules installed correctly
mac_addr = self.port_mac[port_name]
of_check = ovs_vsctl.of_check('br-int', port_name, mac_addr)
if of_check is False:
LOG.info("Return false" )
raise exceptions.NotFoundException(message="openflow rule")

ovs_vsctl.flush()
j += 1


@atomic.action_timer("ovn_network.bind_port")
def _bind_ports(self, lports, sandboxes, port_bind_args):
Expand Down
1 change: 1 addition & 0 deletions rally_ovs/plugins/ovs/scenarios/ovn_network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def create_and_bind_ports(self,
for lswitch in lswitches:
lports = self._create_lports(lswitch, port_create_args, ports_per_network)
self._bind_ports(lports, sandboxes, port_bind_args)
self._of_check_ports(lports, sandboxes, port_bind_args)


def bind_ports(self):
Expand Down