Much more progress on Windows support in ducktape #155
Conversation
|
Test PASSed. |
|
Test PASSed. |
|
Test FAILed. |
1 similar comment
|
Test FAILed. |
|
retest |
|
retest this please |
|
Test FAILed. |
|
Tests are failing because jenkins doesn't have the |
|
Test FAILed. |
|
I just created a PR in muckrake that will be merged alongside this PR, eventually: https://github.com/confluentinc/muckrake/pull/200 |
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
| @@ -48,7 +48,7 @@ def __init__(self, cluster_json=None, *args, **kwargs): | |||
| { | |||
| "externally_routable_ip": "192.168.50.151", | |||
|
|
|||
| "ssh_config": { | |||
| "remote_command_config": { | |||
There was a problem hiding this comment.
I like the renaming, but it's kind of annoying compatibility-wise. Some other folks use the JSON cluster directly (rather than via Vagrant). It might make sense to leave the format as is even if it's slightly inaccurate.
There was a problem hiding this comment.
I'm fine with that, especially now that windows can run SSH commands (because it has an SSH server). I'll go ahead and move everything back to ssh_config.
| @@ -70,6 +70,8 @@ def the_test(x): | |||
|
|
|||
| """ | |||
|
|
|||
| # TODO: should I change this to also support node_spec in addition to num_nodes? | |||
There was a problem hiding this comment.
I think we would have to for the test runner to work properly. Without that support, it can't schedule new tests in parallel since it'll need to take into account which tests need a windows box and how many are available.
There was a problem hiding this comment.
Cool, I'll take a stab at this.
There was a problem hiding this comment.
I didn't have a chance to address this.
| @@ -108,13 +106,13 @@ def setup_node_spec(num_nodes=None, node_spec=None): | |||
| else: | |||
| try: | |||
| for os_type in RemoteAccount.SUPPORTED_OS_TYPES: | |||
| if not node_spec[os_type]: | |||
| if os_type not in node_spec: | |||
There was a problem hiding this comment.
Thinking about this more, this might be annoying since you have to specify 0 or switch between num_nodes and node_spec if you don't want windows machines. Do we actually need to enforce that all OSes are in the node_spec.
There was a problem hiding this comment.
We don't have to. We can assume 0. I like that -- it's more flexible. I'll make that change.
| Return None if undefined. | ||
| """ | ||
|
|
||
| # TODO: closely code review this, please. I can't find anything in ducktape or muckrake that |
There was a problem hiding this comment.
The expected_num_nodes has limited use these days. We ended up going the annotation route eventually partly because variants of the same test can end up using a different # of nodes. We should probably even just get rid of it entirely, so adding to it with expected_node_spec may not make sense.
There was a problem hiding this comment.
Sounds good. I'll remove that part of this code.
…e because a transient vagrant up error.
|
@ewencp: I pushed a new commit that addresses all of your feedback except the one remaining open comment. ducktape tests are passing but I didn't get a chance to test in muckrake because I'm having a transient vagrant up issue. So keep in mind that my newest commit may have a bug in it that would be revealed by running muckrake. |
|
Test FAILed. |
|
Test FAILed. |
|
retest this please |
|
Test FAILed. |
|
retest this please Seems to be a conflict with an existing installed version of requests. Updated the Jenkins jobs to use a virtualenv. |
|
Test FAILed. |
|
retest this please This time, with virtualenv installed. |
|
Test FAILed. |
|
retest this please |
|
Test FAILed. |
|
retest this please |
|
Test FAILed. |
|
retest this please |
|
Test FAILed. |
|
retest this please |
|
Test PASSed. |
|
@ewencp: I think this thing is ready to merge. I addressed the last open comment, tests are passing, and my muckrake changes in https://github.com/confluentinc/muckrake/pull/200 are passing, too. |
|
Test PASSed. |
@ewencp: I've made a lot of progress on the windows changes to ducktape. These changes support my very simple windows-based service: https://github.com/alexlod/muckrake/blob/windows-finished/muckrake/services/dot_net_client_runner.py and a very simple test, too: https://github.com/alexlod/muckrake/blob/windows-finished/muckrake/tests/dot_net_client_test.py
I have a branch builder running right now that is testing these changes. Take a look at
system-test-confluent-platform-branch-builderbuild number136.While you're reviewing this and giving feedback, I'll continue building out the service and test above. Once those are done and this PR merged, as far as I can tell, my whole .NET client system testing work is done until the .NET client is in a testable state (including the pluggable client work being done).
Thanks ahead of time, Ewen!