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

Jump to bottom

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

Merged
merged 4 commits into from Nov 21, 2019
Merged

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

merged 4 commits into from Nov 21, 2019

Conversation

flokli
Copy link
Contributor

@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 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
@flokli
Copy link
Contributor 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
Contributor 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
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
Member

Mic92 commented Nov 13, 2019 •
edited

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

@lejonet
Copy link
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
Contributor 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
Copy link
Contributor 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
Contributor Author

flokli commented Nov 20, 2019

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

@tfc
Copy link
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

@lejonet lejonet lejonet approved these changes

@johanot johanot Awaiting requested review from johanot

Assignees
No one assigned
Labels
6.topic: nixos 9.needs: upstream fix 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flokli @Mic92 @lejonet @tfc