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.

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

mgr: use ipv4 default when ipv6 was disabled #28246

Merged
merged 1 commit into from Jun 11, 2019

Conversation

kungf
Copy link

@kungf kungf commented May 27, 2019

Fixes: https://tracker.ceph.com/issues/40023
Signed-off-by: kungf yang.wang@easystack.cn

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

@votdev
Copy link
Member

votdev commented May 27, 2019

I do not like the usage of the ip command, isn't it possible to check that using a different solution, e.g. by using the procfs information, e.g. like this code.

@@ -25,6 +26,12 @@

from .services.sso import load_sso_db

ip_addrs = subprocess.check_output(["ip","addr"])
Copy link
Member

Choose a reason for hiding this comment

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

I vote to use a different solution, e.g. by using procfs.

Copy link
Author

Choose a reason for hiding this comment

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

hi, @votdev what's the advantage of procfs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm agreeing with Volker here that calling an external program is not my preferred solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this is the wrong place for this if statement, because it's part of the dashboard configuration. Instead it should be located somewhere in _configure()

Copy link
Contributor

Choose a reason for hiding this comment

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

A Python "universal" solution would be preferable here.
Otherwise part of it will degrade into a `if (os_platform == .....) ... else .... " version

@@ -25,6 +26,12 @@

from .services.sso import load_sso_db

ip_addrs = subprocess.check_output(["ip","addr"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this is the wrong place for this if statement, because it's part of the dashboard configuration. Instead it should be located somewhere in _configure()

@sebastian-philipp
Copy link
Contributor

Relates to #26635

@kungf
Copy link
Author

kungf commented May 27, 2019

#26635 may need be reverted, wait_for_occupied_port check is right to check the port.

jan--f
jan--f previously requested changes May 27, 2019
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

Additionally to what other said: Imho this hsould be part of the MgrModule implementation instead of adding this code to every module extending MgrModule

@kungf
Copy link
Author

kungf commented May 27, 2019

hi, @votdev @sebastian-philipp @jan--f thanks for your reviews, code modified, please review again

src/pybind/mgr/mgr_module.py Outdated Show resolved Hide resolved
src/pybind/mgr/mgr_module.py Outdated Show resolved Hide resolved
@LenzGr LenzGr changed the title mgr: use ipv4 default when ipv6 was disabled mgr/dashboard: use ipv4 default when ipv6 was disabled May 27, 2019
@LenzGr LenzGr changed the title mgr/dashboard: use ipv4 default when ipv6 was disabled mgr: use ipv4 default when ipv6 was disabled May 27, 2019
@votdev
Copy link
Member

votdev commented May 27, 2019

Maybe something like this will work? Have not tested how it behaves on a IPv4 only system.

def is_ipv6_enabled():
    import socket
    try:
        socket.AF_INET6
        socket.IPPROTO_IPV6
        socket.IPV6_V6ONLY
        return True
    except AttributeError:
        return False

src/pybind/mgr/mgr_module.py Outdated Show resolved Hide resolved
src/pybind/mgr/mgr_module.py Outdated Show resolved Hide resolved
src/pybind/mgr/mgr_module.py Outdated Show resolved Hide resolved
@kungf kungf force-pushed the mgr_default_addr branch 2 times, most recently from 76a4b9c to 7d81117 Compare May 27, 2019 12:36
@kungf
Copy link
Author

kungf commented May 27, 2019

def is_ipv6_enabled():
    import socket
    try:
        socket.AF_INET6
        socket.IPPROTO_IPV6
        socket.IPV6_V6ONLY
        return True
    except AttributeError:
        return False

@votdev not work too

@tchaikov tchaikov dismissed their stale review May 27, 2019 13:02

comment addressed

@kungf
Copy link
Author

kungf commented May 28, 2019

Hi, reviewers, now we have two function to check whether ipv6 enabled, what's your proposition,socket or netifaces, i think netifaces is more simple,no need to create a socket, but it requires new dependence

def is_ipv6_enabled():                                                                                                                                                                                             
    """ Returns True if the system can bind an IPv6 address. """                 
    sock = None                                                                  
    has_ipv6 = False                                                             
                                                                                 
    if socket.has_ipv6:                                                          
        try:                                                                     
            sock = socket.socket(socket.AF_INET6)                                    
            sock.bind(("::1", 0))                                                    
            has_ipv6 = True                                                          
        except Exception:                                                            
            pass                                                                     
                                                                                     
    if sock:                                                                         
        sock.close()                                                                 
    return has_ipv6
def is_ipv6_enabled():                                                           
    """                                                                          
    Check whether IPv6 is enabled.                                               
    :return: Return True if IPv6 is enabled, otherwise False.                    
    :rtype: bool                                                                 
    """                                                                              
    interfaces = netifaces.interfaces()                                          
    for i in interfaces:                                                         
        if netifaces.ifaddresses(i).has_key(netifaces.AF_INET6):                 
            return True;                                                         
    return False

@sebastian-philipp
Copy link
Contributor

I'm ok with both of them. ( @smithfarm is netifaces already a transitive dependency? )

@tchaikov
Copy link
Contributor

tchaikov commented May 28, 2019

i am in favor of the first solution. and the reason is that it has less dependencies. and we can improve it a little bit:

import contextlib
# ...
def is_ipv6_enabled():
    try:
        sock = socket.socket(socket.AF_INET6)
        with contextlib.closing(sock):
            sock.bind(("::1", 0))
            return True
    except (AttributeError, socket.error) as e:
           return False

changelog:

  • the function is self-explained. no need extra doc string.
  • their is chance that the constant of socket.AF_INET6 is not defined per python document[0], so AttributeError is caught as well
  • use context manager to simplify the exception handling

[0]

These constants represent the address (and protocol) families, used for the first argument to socket(). If the AF_UNIX constant is not defined then this protocol is unsupported.

@smithfarm
Copy link
Contributor

smithfarm commented May 28, 2019

is netifaces already a transitive dependency?

I grepped the Ceph codebase for netifaces and came up with nothing, so I would be careful with it...

UPDATE: On a single-node nautilus cluster installed from RPMs, there is no netifaces (python-netifaces, python3-netifaces) at all, so I'd say the short answer is: "no".

@kungf
Copy link
Author

kungf commented May 28, 2019

hi, reviewers, according your advises, ipv6 check with socket was commit, please review, thanks

@kungf kungf force-pushed the mgr_default_addr branch 2 times, most recently from 3c4281b to 350f764 Compare May 29, 2019 08:08
@kungf
Copy link
Author

kungf commented May 29, 2019

hi reviewers need review again,ths!

@votdev
Copy link
Member

votdev commented May 29, 2019

jenkins retest this please

votdev
votdev previously requested changes May 29, 2019
src/pybind/mgr/prometheus/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/module.py Outdated Show resolved Hide resolved
Fixes: https://tracker.ceph.com/issues/40023
Signed-off-by: kungf <yang.wang@easystack.cn>
@tchaikov
Copy link
Contributor

tchaikov commented Jun 5, 2019

jenkins test dashboard

@tchaikov
Copy link
Contributor

tchaikov commented Jun 5, 2019

@jan--f and @votdev i think the author has address your comments in his latest change, could you please take another look?

@tchaikov tchaikov self-assigned this Jun 5, 2019
@tchaikov tchaikov dismissed stale reviews from votdev and jan--f June 11, 2019 11:45

comments addressed

@tchaikov tchaikov merged commit 748c294 into ceph:master Jun 11, 2019
@kungf kungf deleted the mgr_default_addr branch June 12, 2019 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants