Hi Iwamoto-San,

Thank you for reviewing my patch.


On 2017年07月25日 10:55, IWAMOTO Toshihiro wrote:
On Tue, 25 Jul 2017 10:32:25 +0900,
IWASE Yusuke wrote:

Currently, CLI/file configration option are defined in each application,
but in this case, if that application is not loaded before the
initialization in "ryu/cmd/manager.py" (ryu-manager), the application's
option is NOT enabled.
For example, "ovsdb-timeout" option which defined in
"ryu/lib/ovsdb/bridge.py" is not loaded in ryu-manager, so this option
is not shown in ryu-manager CLI option.

This patch moves the option definitions into flags.py and avoids this
problem.

For example, OpenStack ovs-agent imports ryu.controller.controller but
not ryu.cmd.manager.
I haven't tested but it seems this change would break such a use case.

Oh, I didn't know that...

Okay, I will update my patch to change only minimum required options.

The initial motivate that I moved some definitions is importing
"ryu.controller.ofp_event" from "ryu.cmd.manager" (on 2/2 patch) causes
the cyclic import at "ryu.topology.switches".

Which is the better defining option possible in "flags.py" or defining
them in each module?
In OpenStack, which is prefer?

Thanks,
Iwase



Signed-off-by: IWASE Yusuke <iwase.yusu...@gmail.com>
---
  ryu/app/wsgi.py                            |  13 ----
  ryu/cmd/manager.py                         |   4 +-
  ryu/controller/controller.py               |  29 -------
  ryu/flags.py                               | 119 ++++++++++++++++++++++++++++-
  ryu/lib/ovs/bridge.py                      |   6 --
  ryu/services/protocols/ovsdb/manager.py    |  31 +-------
  ryu/services/protocols/vrrp/rpc_manager.py |  10 +--
  ryu/topology/switches.py                   |  14 ----
  8 files changed, 123 insertions(+), 103 deletions(-)

diff --git a/ryu/app/wsgi.py b/ryu/app/wsgi.py
index 0b98df9..5eead67 100644
--- a/ryu/app/wsgi.py
+++ b/ryu/app/wsgi.py
@@ -31,21 +31,8 @@ import webob.exc
  from webob.request import Request as webob_Request
  from webob.response import Response as webob_Response
-from ryu import cfg
  from ryu.lib import hub
-DEFAULT_WSGI_HOST = '0.0.0.0'
-DEFAULT_WSGI_PORT = 8080
-
-CONF = cfg.CONF
-CONF.register_cli_opts([
-    cfg.StrOpt(
-        'wsapi-host', default=DEFAULT_WSGI_HOST,
-        help='webapp listen host (default %s)' % DEFAULT_WSGI_HOST),
-    cfg.IntOpt(
-        'wsapi-port', default=DEFAULT_WSGI_PORT,
-        help='webapp listen port (default %s)' % DEFAULT_WSGI_PORT),
-])
HEX_PATTERN = r'0x[0-9a-z]+'
  DIGIT_PATTERN = r'[1-9][0-9]*'
diff --git a/ryu/cmd/manager.py b/ryu/cmd/manager.py
index 797d8d5..c801131 100755
--- a/ryu/cmd/manager.py
+++ b/ryu/cmd/manager.py
@@ -28,12 +28,10 @@ import logging
  from ryu import log
  log.early_init_log(logging.DEBUG)
-from ryu import flags
+from ryu import flags  # to load registered option definitions
  from ryu import version
  from ryu.app import wsgi
  from ryu.base.app_manager import AppManager
-from ryu.controller import controller
-from ryu.topology import switches
CONF = cfg.CONF
diff --git a/ryu/controller/controller.py b/ryu/controller/controller.py
index 62bca5f..0c6cef9 100644
--- a/ryu/controller/controller.py
+++ b/ryu/controller/controller.py
@@ -27,11 +27,9 @@ from ryu import cfg
  import logging
  from ryu.lib import hub
  from ryu.lib.hub import StreamServer
-import traceback
  import random
  import ssl
  from socket import IPPROTO_TCP, TCP_NODELAY, SHUT_RDWR, timeout as 
SocketTimeout
-import warnings
import ryu.base.app_manager @@ -48,34 +46,7 @@ from ryu.lib.dpid import dpid_to_str LOG = logging.getLogger('ryu.controller.controller') -DEFAULT_OFP_HOST = '0.0.0.0'
-
  CONF = cfg.CONF
-CONF.register_cli_opts([
-    cfg.StrOpt('ofp-listen-host', default=DEFAULT_OFP_HOST,
-               help='openflow listen host (default %s)' % DEFAULT_OFP_HOST),
-    cfg.IntOpt('ofp-tcp-listen-port', default=None,
-               help='openflow tcp listen port '
-                    '(default: %d)' % ofproto_common.OFP_TCP_PORT),
-    cfg.IntOpt('ofp-ssl-listen-port', default=None,
-               help='openflow ssl listen port '
-                    '(default: %d)' % ofproto_common.OFP_SSL_PORT),
-    cfg.StrOpt('ctl-privkey', default=None, help='controller private key'),
-    cfg.StrOpt('ctl-cert', default=None, help='controller certificate'),
-    cfg.StrOpt('ca-certs', default=None, help='CA certificates')
-])
-CONF.register_opts([
-    cfg.FloatOpt('socket-timeout',
-                 default=5.0,
-                 help='Time, in seconds, to await completion of socket 
operations.'),
-    cfg.FloatOpt('echo-request-interval',
-                 default=15.0,
-                 help='Time, in seconds, between sending echo requests to a 
datapath.'),
-    cfg.IntOpt('maximum-unreplied-echo-requests',
-               default=0,
-               min=0,
-               help='Maximum number of unreplied echo requests before datapath 
is disconnected.')
-])
class OpenFlowController(object):
diff --git a/ryu/flags.py b/ryu/flags.py
index 69eb3d2..40dc724 100644
--- a/ryu/flags.py
+++ b/ryu/flags.py
@@ -18,9 +18,74 @@ global flags
  """
from ryu import cfg
+from ryu.ofproto import ofproto_common
CONF = cfg.CONF +DEFAULT_OFP_HOST = '0.0.0.0'
+
+CONF.register_cli_opts([
+    cfg.StrOpt(
+        'ofp-listen-host', default=DEFAULT_OFP_HOST,
+        help='openflow listen host (default %s)' % DEFAULT_OFP_HOST),
+    cfg.IntOpt(
+        'ofp-tcp-listen-port', default=None,
+        help='openflow tcp listen port'
+             ' (default: %d)' % ofproto_common.OFP_TCP_PORT),
+    cfg.IntOpt(
+        'ofp-ssl-listen-port', default=None,
+        help='openflow ssl listen port'
+             ' (default: %d)' % ofproto_common.OFP_SSL_PORT),
+    cfg.StrOpt(
+        'ctl-privkey', default=None,
+        help='controller private key'),
+    cfg.StrOpt(
+        'ctl-cert', default=None,
+        help='controller certificate'),
+    cfg.StrOpt(
+        'ca-certs', default=None,
+        help='CA certificates'),
+    cfg.BoolOpt(
+        'observe-links', default=False,
+        help='observe link discovery events.'),
+    cfg.BoolOpt(
+        'install-lldp-flow', default=True,
+        help='link discovery: explicitly install flow entry'
+             ' to send lldp packet to controller'),
+    cfg.BoolOpt(
+        'explicit-drop', default=True,
+        help='link discovery: explicitly drop lldp packet in'),
+])  # group='default'
+
+# Note: The following option should not be exposed to CLI, because these
+# options are supposed not to be normally configured by command line.
+CONF.register_opts([
+    cfg.FloatOpt(
+        'socket-timeout', default=5.0,
+        help='Time, in seconds, to await completion of socket operations.'),
+    cfg.FloatOpt(
+        'echo-request-interval', default=15.0,
+        help='Time, in seconds, between sending echo requests to a datapath.'),
+    cfg.IntOpt(
+        'maximum-unreplied-echo-requests', default=0, min=0,
+        help='Maximum number of unreplied echo requests'
+             ' before datapath is disconnected.'),
+])  # group='default'
+
+
+DEFAULT_WSGI_HOST = '0.0.0.0'
+DEFAULT_WSGI_PORT = 8080
+
+CONF.register_cli_opts([
+    cfg.StrOpt(
+        'host', default=DEFAULT_WSGI_HOST,
+        help='webapp listen host (default %s)' % DEFAULT_WSGI_HOST),
+    cfg.IntOpt(
+        'port', default=DEFAULT_WSGI_PORT,
+        help='webapp listen port (default %s)' % DEFAULT_WSGI_PORT),
+], group='wsapi')
+
+
  CONF.register_cli_opts([
      # tests/switch/tester
      cfg.StrOpt('target', default='0000000000000001', help='target sw dp-id'),
@@ -59,6 +124,49 @@ CONF.register_cli_opts([
  ], group='test-switch')
+DEFAULT_OVSDB_TIMEOUT = 2
+
+CONF.register_cli_opts([
+    cfg.IntOpt(
+        'timeout', default=DEFAULT_OVSDB_TIMEOUT,
+        help='ovsdb timeout'),
+    cfg.StrOpt(
+        'address', default='0.0.0.0',
+        help='OVSDB address'),
+    cfg.IntOpt(
+        'port', default=6640,
+        help='OVSDB port'),
+    cfg.IntOpt(
+        'probe-interval',
+        help='OVSDB reconnect probe interval'),
+    cfg.IntOpt(
+        'min-backoff',
+        help='OVSDB reconnect minimum milliseconds'
+             ' between connection attempts'),
+    cfg.IntOpt(
+        'max-backoff',
+        help='OVSDB reconnect maximum milliseconds'
+             ' between connection attempts'),
+    cfg.StrOpt(
+        'mngr-privkey', default=None,
+        help='manager private key'),
+    cfg.StrOpt(
+        'mngr-cert', default=None,
+        help='manager certificate'),
+    cfg.ListOpt(
+        'whitelist', default=[],
+        help='Whitelist of address to allow to connect'),
+    cfg.ListOpt(
+        'schema-tables', default=[],
+        help='Tables in the OVSDB schema to configure'),
+    cfg.ListOpt(
+        'schema-exclude-columns', default=[],
+        help='Table columns in the OVSDB schema to filter out.'
+             ' Values should be in the format: <table>.<column>.'
+             ' Ex: Bridge.netflow,Interface.statistics'),
+], group='ovsdb')
+
+
  DEFAULT_RPC_PORT = 50002
  DEFAULT_RPC_HOST = '0.0.0.0'
@@ -69,10 +177,19 @@ CONF.register_cli_opts([
                 help='IP for RPC server (default: %s)' % DEFAULT_RPC_HOST),
      cfg.StrOpt('config-file', default=None,
                 help='The config file formatted in Python source file. '
-                    'Please refer to "bgp_sample_conf.py" for details.')
+                    'Please refer to "bgp_sample_conf.py" for details.'),
  ], group='bgp-app')
+DEFAULT_VRRP_RPC_PORT = 50004 # should be random
+
+CONF.register_cli_opts([
+    cfg.IntOpt(
+        'rpc-port', default=DEFAULT_VRRP_RPC_PORT,
+        help='port for vrrp rpc interface'),
+], group='vrrp')
+
+
  DEFAULT_ZSERV_HOST = '/var/run/quagga/zserv.api'
  DEFAULT_ZSERV_PORT = 2600
  DEFAULT_ZSERV_VERSION = 2  # Version of Ubuntu 16.04 LTS packaged Quagga
diff --git a/ryu/lib/ovs/bridge.py b/ryu/lib/ovs/bridge.py
index 337867e..55eb37e 100644
--- a/ryu/lib/ovs/bridge.py
+++ b/ryu/lib/ovs/bridge.py
@@ -21,18 +21,12 @@ slimmed down version of OVSBridge in quantum agent
  import functools
  import logging
-from ryu import cfg
  import ryu.exception as ryu_exc
  import ryu.lib.dpid as dpid_lib
  import ryu.lib.ovs.vsctl as ovs_vsctl
LOG = logging.getLogger(__name__) -CONF = cfg.CONF
-CONF.register_opts([
-    cfg.IntOpt('ovsdb-timeout', default=2, help='ovsdb timeout')
-])
-
class OVSBridgeNotFound(ryu_exc.RyuException):
      message = 'no bridge for datapath_id %(datapath_id)s'
diff --git a/ryu/services/protocols/ovsdb/manager.py 
b/ryu/services/protocols/ovsdb/manager.py
index 9822503..6c52df3 100644
--- a/ryu/services/protocols/ovsdb/manager.py
+++ b/ryu/services/protocols/ovsdb/manager.py
@@ -16,7 +16,6 @@
  import ssl
  import socket
-from ryu import cfg
  from ryu.base import app_manager
  from ryu.lib import hub
  from ryu.services.protocols.ovsdb import client
@@ -24,30 +23,6 @@ from ryu.services.protocols.ovsdb import event
  from ryu.controller import handler
-opts = (cfg.StrOpt('address', default='0.0.0.0', help='OVSDB address'),
-        cfg.IntOpt('port', default=6640, help='OVSDB port'),
-        cfg.IntOpt('probe-interval', help='OVSDB reconnect probe interval'),
-        cfg.IntOpt('min-backoff',
-                   help=('OVSDB reconnect minimum milliseconds between '
-                         'connection attemps')),
-        cfg.IntOpt('max-backoff',
-                   help=('OVSDB reconnect maximum milliseconds between '
-                         'connection attemps')),
-        cfg.StrOpt('mngr-privkey', default=None, help='manager private key'),
-        cfg.StrOpt('mngr-cert', default=None, help='manager certificate'),
-        cfg.ListOpt('whitelist', default=[],
-                    help='Whitelist of address to allow to connect'),
-        cfg.ListOpt('schema-tables', default=[],
-                    help='Tables in the OVSDB schema to configure'),
-        cfg.ListOpt('schema-exclude-columns', default=[],
-                    help='Table columns in the OVSDB schema to filter out.  '
-                         'Values should be in the format: <table>.<column>.'
-                         'Ex: Bridge.netflow,Interface.statistics')
-        )
-
-cfg.CONF.register_opts(opts, 'ovsdb')
-
-
  class OVSDB(app_manager.RyuApp):
      _EVENTS = [event.EventNewOVSDBConnection,
                 event.EventModifyRequest,
@@ -126,10 +101,10 @@ class OVSDB(app_manager.RyuApp):
          return self.send_event(client_name, ev)
def _start_remote(self, sock, client_address):
-        schema_tables = cfg.CONF.ovsdb.schema_tables
+        schema_tables = self.CONF.ovsdb.schema_tables
          schema_ex_col = {}
-        if cfg.CONF.ovsdb.schema_exclude_columns:
-            for c in cfg.CONF.ovsdb.schema_exclude_columns:
+        if self.CONF.ovsdb.schema_exclude_columns:
+            for c in self.CONF.ovsdb.schema_exclude_columns:
                  tbl, col = c.split('.')
                  if tbl in schema_ex_col:
                      schema_ex_col[tbl].append(col)
diff --git a/ryu/services/protocols/vrrp/rpc_manager.py 
b/ryu/services/protocols/vrrp/rpc_manager.py
index 6ac07b1..f78dd48 100644
--- a/ryu/services/protocols/vrrp/rpc_manager.py
+++ b/ryu/services/protocols/vrrp/rpc_manager.py
@@ -13,10 +13,8 @@
  # See the License for the specific language governing permissions and
  # limitations under the License.
-from ryu import cfg
-import socket
-
  import netaddr
+
  from ryu.base import app_manager
  from ryu.controller import handler
  from ryu.services.protocols.vrrp import event as vrrp_event
@@ -25,8 +23,6 @@ from ryu.lib import rpc
  from ryu.lib import hub
  from ryu.lib import mac
-VRRP_RPC_PORT = 50004 # random
-
class RPCError(Exception):
      pass
@@ -44,10 +40,6 @@ class Peer(object):
  class RpcVRRPManager(app_manager.RyuApp):
      def __init__(self, *args, **kwargs):
          super(RpcVRRPManager, self).__init__(*args, **kwargs)
-        self.CONF.register_opts([
-            cfg.IntOpt('vrrp-rpc-port', default=VRRP_RPC_PORT,
-                       help='port for vrrp rpc interface')])
-
          self._args = args
          self._kwargs = kwargs
          self._peers = []
diff --git a/ryu/topology/switches.py b/ryu/topology/switches.py
index 644a1fe..f0995e9 100644
--- a/ryu/topology/switches.py
+++ b/ryu/topology/switches.py
@@ -17,7 +17,6 @@ import logging
  import six
  import struct
  import time
-from ryu import cfg
from ryu.topology import event
  from ryu.base import app_manager
@@ -43,19 +42,6 @@ from ryu.ofproto import ofproto_v1_4
  LOG = logging.getLogger(__name__)
-CONF = cfg.CONF
-
-CONF.register_cli_opts([
-    cfg.BoolOpt('observe-links', default=False,
-                help='observe link discovery events.'),
-    cfg.BoolOpt('install-lldp-flow', default=True,
-                help='link discovery: explicitly install flow entry '
-                     'to send lldp packet to controller'),
-    cfg.BoolOpt('explicit-drop', default=True,
-                help='link discovery: explicitly drop lldp packet in')
-])
-
-
  class Port(object):
      # This is data class passed by EventPortXXX
      def __init__(self, dpid, ofproto, ofpport):
--
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Ryu-devel mailing list
Ryu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ryu-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Ryu-devel mailing list
Ryu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ryu-devel

Reply via email to