Cleanup after lbaas management script #30

matelakat · 2017-04-10T13:44:24Z

We agreed to clean up after the lbaasv2 management script has been merged, so that's the goal of this PR.

TODOS

Fix testing and cover all files with flake8
Unify classes - so RemoteRouterNSCleanup and RemoteLbaasV2Cleanup's public functions look alike, refactor them so to re-use network namespace cleanup methods.
Extract Host and methods to connect to SSH
Create Namespace class for managing namespaces living on a Host

Remaining issues

Still, socket.timeout sneaks in to exception handling - I suggest we make top-level methods swallow the exceptions, it doesn't make sense to handle timeout separately - also it doesn't handle exceptions that might occur during connection. This needs to be specified what behavior do we want
timeouts. Now it became clear that we are talking about 2 timeouts. One is to run a command, and the other is to connect to a host. We need to make sure that for blocking operations the timeout is sufficient. Also I am not sure if the timeout in exec_command is the one we need. That needs to be tested separately.
lbaasv2 movement scripts are not tested properly, we need to cover them. Now that we have better abstractions, it should be easier to test them.

Given that now we have multiple scripts, it makes sense to introduce a test runner. nosetests seems to support our weird file namings, so using that. It picked up signal_tester as a testcase, so that has been renamed. Also amended travis, and added noqa for some flake8 violations.

rhafer

Commit message of c0f5110 seem wrong: "Move the responsibility of connection to delete_remote_namespace" should be "... to delete_router_namespace"

Move the responsibility of connection to delete_router_namespace thus making delete_remote_namespace re-usable by the lbaasv2 cleanup classes.

The LbaasV2 cleanup was performing the same operations as RemoteNodeCleanup, so let's call that instead of repeating the lines. Also changing the log level to info. This means that info messages will be generated on router movement.

Separate out the responsibilities for connecting to a remote host, and create the Host abstraction that could run commands on a host. Also add some test helpers to ease creating a mock ssh connection instance. With this change, the responsibilites of RemoteNodeCleanup became smaller, and ssh related code could be re-used later. Also the internal ssh_client is hidden, so RemoteLbaasV2Cleanup does not need to know how the connection happens

As the existing logging statements all print out host, being able to stringify SSHHost makes sense - this way logging statements can say something like: with connect_to_host('localhost', timeout=10) as host: LOG.info('connected to %s', host)

connected_to_host sets the timeout for the run operation, thus it is not needed to specify it on each call. This makes _simple_ssh_command obsolete, thus removing it.

Since host implements str, we don't need to refer to target_host anymore in log messages.

Make it easier for mocking out an exec_command call of the ssh_client by providing a helper to make tests more readable.

Each uses were doing the same thing, instantiating a class and then calling a single method. Extracting it to one single method instead so that the internals could be refactored for a cleaner design and easier testability.

Introduce a Namespace class for managing namespaces and move methods from RemoteNodeCleanup to it. This eliminates the need for classes, and the class hierarchy as well.

matelakat · 2017-05-08T14:39:40Z

Commit message of c0f5110 seem wrong: "Move the responsibility of connection to delete_remote_namespace" should be "... to delete_router_namespace"

should be fixed now.

rhafer

Nice refactoring! Thanks a lot for taking the time to walk me through it on mumble. I really appreciate it.

As for the open questions, you raised in the initial comment. I think it's ok to address these things later. And I especially agree about the one about more testing.

As discussed in mumble, I don't think the socket.timeout thing is much of an issue though.

matelakat added the wip label Apr 10, 2017

matelakat removed the wip label Apr 19, 2017

matelakat requested review from rhafer and aspiers April 19, 2017 08:56

rhafer suggested changes May 8, 2017

View reviewed changes

Mate Lakat added 10 commits May 8, 2017 16:37

Refactor: move connection to delete_router_namespace

60dcc1e

Move the responsibility of connection to delete_router_namespace thus making delete_remote_namespace re-usable by the lbaasv2 cleanup classes.

Refactor: re-use delete_remote_namespace

a6b50e0

The LbaasV2 cleanup was performing the same operations as RemoteNodeCleanup, so let's call that instead of repeating the lines. Also changing the log level to info. This means that info messages will be generated on router movement.

str(SSHHost) returns hostname

ceed224

As the existing logging statements all print out host, being able to stringify SSHHost makes sense - this way logging statements can say something like: with connect_to_host('localhost', timeout=10) as host: LOG.info('connected to %s', host)

Add test: timeout is passed down to connect()

d6e7a89

Refactor - enable to set run_timeout on host

cb7c0d7

connected_to_host sets the timeout for the run operation, thus it is not needed to specify it on each call. This makes _simple_ssh_command obsolete, thus removing it.

refactor: Use host in log messages

a76e005

Since host implements str, we don't need to refer to target_host anymore in log messages.

Refactor - extract method

0510718

Make it easier for mocking out an exec_command call of the ssh_client by providing a helper to make tests more readable.

Refactor: extract destroy_router_namespace

3f3b497

Each uses were doing the same thing, instantiating a class and then calling a single method. Extracting it to one single method instead so that the internals could be refactored for a cleaner design and easier testability.

Refactor - remove Cleanup classes

8bcb2d8

Introduce a Namespace class for managing namespaces and move methods from RemoteNodeCleanup to it. This eliminates the need for classes, and the class hierarchy as well.

matelakat requested a review from rhafer May 8, 2017 14:39

rhafer approved these changes May 8, 2017

View reviewed changes

matelakat mentioned this pull request May 11, 2017

Check agent liveness while migrating #31

Merged

JanZerebecki added the wip label Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup after lbaas management script #30

Cleanup after lbaas management script #30

matelakat commented Apr 10, 2017 •

edited

Loading

rhafer left a comment

matelakat commented May 8, 2017

rhafer left a comment

Cleanup after lbaas management script #30

Are you sure you want to change the base?

Cleanup after lbaas management script #30

Conversation

matelakat commented Apr 10, 2017 • edited Loading

TODOS

Remaining issues

rhafer left a comment

Choose a reason for hiding this comment

matelakat commented May 8, 2017

rhafer left a comment

Choose a reason for hiding this comment

matelakat commented Apr 10, 2017 •

edited

Loading