Skip to content

Conversation

@portante
Copy link
Member

@portante portante commented Mar 29, 2021

We needed a way to specify what IP address and port to use when the Redis server and Tool Data Sink (really its WSGI server) bind for their respective services, while also being able to specify what address and port the Tool Meister(s) should connect to in environments where those address and ports are different (software-defined-networking, or NAT'd, or some other reason).

We solve this need by adding a new option, --orchestrate, along with a new --tool-data-sink option, and adjust the behaviors a bit.

The new --orchestrate option accepts two keywords, create and existing, which allow the caller to direct tool-meister-start to either be responsible for creating the Redis server, Tool Data Sink, and Tool Meister instances, or use existing instances. The --redis-server (or the PBENCH_REDIS_SERVER environment variable) and --tool-data-sink (or the PBENCH_TOOL_DATA_SINK environment variable) options give the caller the ability to specify the IP/port combinations to use in either operating case.

When --orchestrate=existing is specified, the --redis-server and --tool-data-sink options (or environment variables) must be specified.

The format for specifying the IP/ports to use via the --redis-server and --tool-data-sink options can be one of the following:

  • <host ip>:<port>: used both for listening and connections
  • <listen host ip>:<port>;<connection host ip>:<port>: the 1st host/port pair is used for listening for connections, the 2nd host/port pair is used to specify how to make connections to the instances

The defaults are:

  • Redis: bind & connect, ${_pbench_full_hostname}:17001
    • Also binds to localhost
  • TDS: bind & connect, ${_pbench_full_hostname}:8080

Prior to this change, pbench-tool-meister-start had one very long main method. This refactoring breaks out the orchestration pieces required to start the Redis server, Tool Data Sink, and the Tool Meisters, into their own methods and classes, reducing main to something a bit more reasonable.

As part of this work, a new BaseServer abstraction, sub-classed for the RedisServer and ToolDataSink, now handles how the Redis server and Tool Data Sink instance tracking works, either when they are created by tool-meister-start, or created ahead of time by the caller.

@portante portante added enhancement Agent tools Of and related to the operation and behavior of various tools (iostat, sar, etc.) labels Mar 29, 2021
@portante portante added this to the v0.71 milestone Mar 29, 2021
@portante portante self-assigned this Mar 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 2 alerts when merging 0903571 into cf66c53 - view on LGTM.com

new alerts:

  • 1 for Missing call to `__init__` during object initialization
  • 1 for Wrong number of arguments in a call

@portante portante force-pushed the tm-fix-bind-hosts branch 2 times, most recently from 9b7d998 to 920907d Compare March 31, 2021 01:49
@portante portante marked this pull request as ready for review March 31, 2021 01:50
@portante
Copy link
Member Author

portante commented Mar 31, 2021

@RobertKrawitz, okay here is the fixed code to handle specifying the listen (binding) vs connection IP/ports for both the Tool Data Sink and the Redis Server as created by pbench-tool-meister-start.

Please take a moment to review the PR description where we describe how to use PBENCH_REDIS_SERVER_CFG and PBENCH_TOOL_DATA_SINK_CFG contents.

@portante
Copy link
Member Author

I need to test this in a real container, still, but it is a start.

@portante portante force-pushed the tm-fix-bind-hosts branch from 920907d to 94f1a73 Compare March 31, 2021 18:01
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

Light first pass: typos and some questions. Need to go through main() in more detail though.

Copy link
Member

@Maxusmusti Maxusmusti left a comment

Choose a reason for hiding this comment

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

Looks awesome so far, just a couple of design questions

Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

More to come....

@portante portante force-pushed the tm-fix-bind-hosts branch from 37fb854 to 3c14a84 Compare April 5, 2021 23:58
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Done! (For this round....)

@portante
Copy link
Member Author

portante commented Apr 8, 2021

Okay, since some of the code-review comments were asking about validating host names and IP addresses, and we had issue #2201 come up during this window of work, I decided to add a commit to solve #2201 first. It is now the first commit before any other changes.

I'll follow that up with another refactoring so that the pbench-tool-meister-start host name processing can leverage it as well.

@portante portante force-pushed the tm-fix-bind-hosts branch 4 times, most recently from f7a4b8f to 9641f62 Compare April 8, 2021 15:53
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

The levels of validate-hostname / validate_hostname are a bit convoluted, but I like the idea.

I see Webb pointed out a few problems that need to be resolved; but pending that, it's looking good.

@portante portante marked this pull request as ready for review April 12, 2021 14:47
@portante
Copy link
Member Author

@Maxusmusti, @ndokos, this PR still have 2 changes requested from the two of you. Is that still the case?

@portante portante force-pushed the tm-fix-bind-hosts branch from a0cfe54 to 8fdac35 Compare April 12, 2021 17:41
webbnh
webbnh previously approved these changes Apr 12, 2021
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

I assume that you are going to open an issue for the IPv6 address-with-port format, rather than add it to this PR, so, modulo the documentation nit below (and Nick's concerns), I think this is good to go.

Copy link
Member Author

@portante portante left a comment

Choose a reason for hiding this comment

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

Okay, ready for another pass. I had to rebase anyways, and these changes were straightforward to make.

@portante portante requested review from ndokos and webbnh April 12, 2021 23:33
dbutenhof
dbutenhof previously approved these changes Apr 13, 2021
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Yeah, we need to clean up the IPv6 addressing, but since we're not using IPv6 I guess that can be deferred. (Do we even have IPv6 in RDU2 or BOS labs?)

Maxusmusti
Maxusmusti previously approved these changes Apr 13, 2021
Copy link
Member

@Maxusmusti Maxusmusti left a comment

Choose a reason for hiding this comment

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

Passed usual local/remote node testing with expected results

We needed a way to specify what IP address and port to use when the
Redis server and Tool Data Sink (really its WSGI server) bind for their
respective services, while also being able to specify what address and
port the Tool Meister(s) should connect to in environments where those
address and ports are different (software-defined-networking, or NAT'd,
or some other reason).

We solve this need by adding a new option, `--orchestrate`, along with
a new `--tool-data-sink` option, and adjust the behaviors a bit.

The new `--orchestrate` option accepts two keywords, `create` and
`existing`, which allow the caller to direct tool-meister-start to
either be responsible for creating the Redis server, Tool Data Sink,
and Tool Meister instances, or use existing instances.  The
`--redis-server` (or the `PBENCH_REDIS_SERVER` environment variable)
and `--tool-data-sink` (or the `PBENCH_TOOL_DATA_SINK` environment
variable) options give the caller the ability to specify the IP/port
combinations to use in either operating case.

When `--orchestrate=existing` is specified, the `--redis-server` and
`--tool-data-sink` options (or environment variables) must be specified.

The format for specifying the IP/ports to use via the `--redis-server`
and `--tool-data-sink` options can be one of the following:

  * `<host ip>:<port>`: used both for listening and connections
  * `<listen host ip>:<port>;<connection host ip>:<port>`: the 1st
    host/port pair is used for listening for connections, the 2nd
    host/port pair is used to specify how to make connections to
    the instances

The defaults are:
  * Redis: bind & connect, `${_pbench_full_hostname}:17001`
    * Also binds to `localhost`
  * TDS:   bind & connect, `${_pbench_full_hostname}:8080`

Prior to this change, `pbench-tool-meister-start` had one very long
`main` method.  This refactoring breaks out the orchestration pieces
required to start the Redis server, Tool Data Sink, and the Tool
Meisters, into their own methods and classes, reducing `main` to
something a bit more reasonable.

As part of this work, a new `BaseServer` abstraction, sub-classed for
the `RedisServer` and `ToolDataSink`, now handles how the Redis server
and Tool Data Sink instance tracking works, either when they are
created by tool-meister-start, or created ahead of time by the caller.
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks great, let's ship it! (Please don't make me review the entire thing again....)

What? Oh, them?...no, pay no attention to those comments.....

Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I say go.

@portante portante merged commit c9dd0e9 into distributed-system-analysis:main Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment