Skip to content

WIP: nixos/ceph: port tests to python#73190

Merged
flokli merged 4 commits intoNixOS:masterfrom
flokli:ceph-tests-python
Nov 21, 2019
Merged

WIP: nixos/ceph: port tests to python#73190
flokli merged 4 commits intoNixOS:masterfrom
flokli:ceph-tests-python

Conversation

@flokli
Copy link
Copy Markdown
Member

@flokli flokli commented Nov 11, 2019

Motivation for this change

#72828

Contains commits from #72603 and #73189 and should be rebased on top of master once these are merged.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @tfc @worldofpeace

@flokli flokli requested review from johanot and lejonet November 11, 2019 00:35
@flokli flokli changed the title ceph: port tests to python nixos/ceph: port tests to python Nov 11, 2019
@flokli flokli changed the title nixos/ceph: port tests to python WIP: nixos/ceph: port tests to python Nov 11, 2019
@tfc tfc mentioned this pull request Nov 11, 2019
Closed
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 11, 2019
@ofborg ofborg bot removed the 8.has: module (update) This PR changes an existing module in `nixos/` label Nov 11, 2019
@flokli
Copy link
Copy Markdown
Member Author

flokli commented Nov 11, 2019

@Mic92 this contains #73189, but it seems the python test driver spends 100% of its cpu time with recvfrom syscalls when running the ceph-multi-node test… Any idea what's going on?

@flokli
Copy link
Copy Markdown
Member Author

flokli commented Nov 11, 2019

Turns out, it's wasting cpu cycles in the wait_for_monitor_prompt method, because for some reason, there's nothing received.

@flokli flokli mentioned this pull request Nov 11, 2019
Merged
10 tasks
lejonet
lejonet approved these changes Nov 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lejonet lejonet left a comment

Choose a reason for hiding this comment

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

From what I can see, it doesn't change any of the test cases, merely just changes the syntax of calling.

The only comment I have is that in the lines where there is more than one command in a row, and thus a comma-separated list is given to succeed() for example, there seems to be some extra whitespace that gets added. Is that intentionally or just your editor interprenting tabs as 4 spaces or something? I don't really care if its 2, 4, 8 or more spaces, the indentation is coherent and that is the important part. Just wanted to highlight it to see if it was an intended or unintended change.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Nov 13, 2019 •
edited
Loading

4 spaces is what the python community does for code, so I am fine with that.

@lejonet
Copy link
Copy Markdown
Contributor

lejonet commented Nov 13, 2019

4 spaces is what the python community does for code, so I am fine with that.

Yeah, I'm completely fine with it too, just wanted to point it out, in case it wasn't intentional :)

@flokli
Copy link
Copy Markdown
Member Author

flokli commented Nov 17, 2019

@tfc @lejonet after some investigation, it turns out the python test driver doesn't properly handle emptyDiskImages. I opened #73559 to track this.

@flokli flokli added the 9.needs: upstream fix This PR needs upstream to change something label Nov 17, 2019
@flokli
Copy link
Copy Markdown
Member Author

flokli commented Nov 20, 2019

Got the test to work - they uncovered two issues in the python test driver, which didn't behave exactly like the perl one (passing of environment to the script executing qemu, handling in the wait_for_monitor_prompt method).

Thanks to @tfc, these are fixed now. Rebased on latest master and cherry-picked them in. PTAL.

@flokli
Copy link
Copy Markdown
Member Author

flokli commented Nov 20, 2019

@GrahamcOfBorg test ceph-single-node ceph-multi-node

@tfc
Copy link
Copy Markdown
Contributor

tfc commented Nov 21, 2019

@domenkozar @adisbladis

@flokli flokli merged commit 758efb9 into NixOS:master Nov 21, 2019
@flokli flokli deleted the ceph-tests-python branch November 21, 2019 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tfc tfc tfc approved these changes

@johanot johanot Awaiting requested review from johanot

+1 more reviewer

@lejonet lejonet lejonet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 9.needs: upstream fix This PR needs upstream to change something 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@flokli @Mic92 @lejonet @tfc