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

cephadm: fix port_in_use when IPv6 is disabled #39421

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

p-se
Copy link
Contributor

@p-se p-se commented Feb 11, 2021

Do not return "port is in use" when the protocol family tested is not
supported (due to being deactivated).

Differentiates between

OSError: [Errno 97] Address family not supported by protocol

and

OSError: [Errno 98] Address already in use

Signed-off-by: Patrick Seidensal pseidensal@suse.com

Checklist

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

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Copy link
Contributor

@sebastian-philipp sebastian-philipp left a comment

Choose a reason for hiding this comment

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

lgrm, but unrelated whitespace changes

src/cephadm/cephadm Outdated Show resolved Hide resolved
@p-se p-se force-pushed the wip-pse-cephadm-port-in-use branch from eecef03 to 6c0e059 Compare February 12, 2021 09:54
@p-se p-se marked this pull request as ready for review February 12, 2021 10:00
@p-se p-se requested a review from a team as a code owner February 12, 2021 10:00
@p-se p-se force-pushed the wip-pse-cephadm-port-in-use branch 3 times, most recently from 8ecdee9 to 512f899 Compare February 12, 2021 13:14
Copy link
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

LGTM

@p-se
Copy link
Contributor Author

p-se commented Feb 15, 2021

jenkins retest this please

@p-se
Copy link
Contributor Author

p-se commented Feb 15, 2021

jenkins test make check

@p-se
Copy link
Contributor Author

p-se commented Feb 15, 2021

Building remotely on irvingi03

tests/test_cephadm.py:529: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
cephadm:6554: in port_active
    return port_in_use(self.ctx, self.port)
cephadm:966: in port_in_use
    (socket.AF_INET6, '::')
cephadm:964: in <genexpr>
    return any(_port_in_use(af, address) for af, address in (
cephadm:961: in _port_in_use
    raise e
cephadm:954: in _port_in_use
    attempt_bind(ctx, s, address, port_num)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

ctx = <cephadm.CephadmContext object at 0x7f14bb673dd8>
s = <socket.socket [closed] fd=-1, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0>
address = '0.0.0.0', port = 9443

    def attempt_bind(ctx, s, address, port):
        # type: (CephadmContext, socket.socket, str, int) -> None
        try:
            s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
            s.bind((address, port))
        except (socket.error, OSError) as e:  # py2 and py3
            msg = 'Cannot bind to IP %s port %d: %s' % (address, port, e)
            logger.warning(msg)
            if e.errno == errno.EADDRINUSE:
>               raise OSError(msg)
E               OSError: Cannot bind to IP 0.0.0.0 port 9443: [Errno 98] Address already in use

cephadm:938: OSError

However, running the tests locally seems to work:

user@home ~/src/ceph/src/pybind/mgr ±wip-pse-cephadm-port-in-use » ../../script/run_tox.sh mgr -- cephadm | tail 
flake8 installed: DEPRECATION: --find-links option in pip freeze is deprecated. pip 21.2 will remove support for this functionality. You can find discussion regarding this at https://github.com/pypa/pip/issues/9069.,-f /home/user/src/ceph/src/pybind/mgr/wheelhouse,flake8==3.8.4,mccabe==0.6.1,pycodestyle==2.6.0,pyflakes==2.2.0
flake8 run-test-pre: PYTHONHASHSEED='3595092527'
flake8 run-test: commands[0] | flake8 --config=tox.ini cephadm cephadm
___________________________________ summary ____________________________________
  py3: commands succeeded
  mypy: commands succeeded
  test: commands succeeded
  fix: commands succeeded
  flake8: commands succeeded
  congratulations :)
user@home ~/src/ceph/src/pybind/mgr ±wip-pse-cephadm-port-in-use » 

@p-se
Copy link
Contributor Author

p-se commented Feb 16, 2021

jenkins test make check

@sebastian-philipp
Copy link
Contributor

Building remotely on irvingi03

I think we need to mock socket.bind. Maybe

@mock.patch("socket.bind")
    def test_port_active(self, _, exporter):
        assert exporter.port_active == True

@p-se
Copy link
Contributor Author

p-se commented Feb 16, 2021

Building remotely on irvingi03

I think we need to mock socket.bind. Maybe

@mock.patch("socket.bind")
    def test_port_active(self, _, exporter):
        assert exporter.port_active == True

What I haven't understood yet is why this test, all of a sudden, starts making problems and hasn't done so before. I wonder if that's due to the machine or the test (maybe being canceled at the wrong moment?). But in any case, adapting the test should get us going.

@sebastian-philipp
Copy link
Contributor

Building remotely on irvingi03

I think we need to mock socket.bind. Maybe

@mock.patch("socket.bind")
    def test_port_active(self, _, exporter):
        assert exporter.port_active == True

What I haven't understood yet is why this test, all of a sudden, starts making problems and hasn't done so before. I wonder if that's due to the machine or the test (maybe being canceled at the wrong moment?). But in any case, adapting the test should get us going.

cause we are no longer hiding those exceptions from the caller. which is a good thing, but now some tests did call that function and are now throwing errors.

@p-se p-se force-pushed the wip-pse-cephadm-port-in-use branch from 512f899 to 03d2262 Compare February 18, 2021 10:01
src/cephadm/cephadm Outdated Show resolved Hide resolved
Do not return "port is in use" when the protocol family tested is not
supported (due to being deactivated).

Fixes: https://tracker.ceph.com/issues/49273

Signed-off-by: Patrick Seidensal <pseidensal@suse.com>
@p-se p-se force-pushed the wip-pse-cephadm-port-in-use branch from 03d2262 to a0ffcec Compare February 18, 2021 22:39
@p-se
Copy link
Contributor Author

p-se commented Feb 18, 2021

Building remotely on irvingi03

I think we need to mock socket.bind. Maybe

@mock.patch("socket.bind")
    def test_port_active(self, _, exporter):
        assert exporter.port_active == True

What I haven't understood yet is why this test, all of a sudden, starts making problems and hasn't done so before. I wonder if that's due to the machine or the test (maybe being canceled at the wrong moment?). But in any case, adapting the test should get us going.

cause we are no longer hiding those exceptions from the caller. which is a good thing, but now some tests did call that function and are now throwing errors.

Turns out the code was faulty. Fixed it and created some tests to ensure it works as expected.

@sebastian-philipp
Copy link
Contributor

p-se requested a review from sebastian-philipp 1 hour ago

I'm waiting for https://pulpito.ceph.com/swagner-2021-02-22_13:54:50-rados:cephadm-wip-swagner4-testing-2021-02-22-1119-distro-basic-smithi/ to be scheduled.

@smithfarm
Copy link
Contributor

NOTE: this was backported to octopus via #41602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants