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

ceph-disk: FreeBSD changes to get it working and passing tests #12086

Merged
merged 1 commit into from Feb 8, 2017

Conversation

Projects
None yet
4 participants
@wjwithagen
Contributor

wjwithagen commented Nov 20, 2016

  • It currently only supports running with filestore
  • Testing is executed while running on a ZFS partition
  • All disktypes and naming is different on FreeBSD
  • Partitioning and tools are not workable on FreeBSD

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@ghost ghost added core feature labels Nov 23, 2016

@wjwithagen wjwithagen requested a review from Jan 25, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Feb 1, 2017

@liewegas
Hi,

Any suggestions how to push this one forward?

@liewegas

lgtm!

@liewegas liewegas added the needs-qa label Feb 1, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Feb 1, 2017

@liewegas
Thanx, lets see wat QA brings.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Feb 4, 2017

@liewegas
Sorry, I have added some more code, to auto start/stop on activate.
Did not see you scheduled QA already.

@yuriw

This comment has been minimized.

Contributor

yuriw commented Feb 5, 2017

test this please

@@ -799,8 +819,12 @@ def is_mounted(dev):
"""
Check if the given device is mounted.
"""
if FREEBSD:
PROCDIR = '/compat/linux/proc'

This comment has been minimized.

@tchaikov

This comment has been minimized.

@wjwithagen

wjwithagen Feb 6, 2017

Contributor

@tchaikov
I think an odl and a newwer commit got mixed up. Will clean up.

FREEBSD = False
if platform.system() == 'FreeBSD':
FREEBSD = True
PROCDIR = '/compat/linux/proc'

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

nit. could move the

FREEBSD = False
PROCDIR = '/proc'

to the "else" branch. so the structure of the code is more balanced and compact.

echo "Command: $cmnd is a non existing command"
return 1
;;
*)

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

If the command times out, and --preserve-status is not set, then exit with status 124.

the timeout from coreutils returns 124 on timeout.

This comment has been minimized.

@wjwithagen

wjwithagen Feb 6, 2017

Contributor

@tchaikov
Removed timeout_diagnose, as requested.

umount $mounted
done
rm -fr $dir
}
function timeout_diagnose() {

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

this function helps if we are debugging the test, but it should be not used in the test, i think. if the test itself is sane, timeout here should not return 125,126,127 because we are passing the wrong argument to it, or the launched executable does not exists. the only possibility is that the "command" returned this status code.

This comment has been minimized.

@wjwithagen

wjwithagen Feb 6, 2017

Contributor

@tchaikov
Are you suggesting to take it out all the way?

Reason I left it in, is that I do run into these timeout problems that I woudl like to be diagnosed correctly.
Hence the full case statement. I'll add 124 explicitly, since that is what FreeBSD also does, if we decide to keep it.

Removing that from the tests, will result in somebody else concluding that this routine is not used.
And the submit a PR for removal.

This comment has been minimized.

@tchaikov

tchaikov Feb 6, 2017

Contributor

Are you suggesting to take it out all the way?

@wjwithagen yes.

Reason I left it in, is that I do run into these timeout problems that I woudl like to be diagnosed correctly.

but again, if they were the problems of the test itself, we should fix the test first.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Feb 6, 2017

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes, in this case, it would be "ceph-disk: " but not "FreeBSD: ".

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Feb 6, 2017

@tchaikov
I know about selecting the title of the PR carefully. But it is sometimes a good question on what to put there. Not with this case: your suggestion is the correct one. But I do struggle to pick the correct leaders. sometimes.

@wjwithagen wjwithagen changed the title from FreeBSD: changes to ceph-disk to get it working and passing tests to ceph-disk: FreeBSD changes to get it working and passing tests Feb 6, 2017

FreeBSD: changes to ceph-disk to get it working and passing tests
 - It currently only supports running with filestore
 - Testing is executed while running on a ZFS partition
 - All disktypes and naming is different on FreeBSD
 - Partitioning and tools are not workable on FreeBSD
 - add diagnostic timeout

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Feb 7, 2017

@dachary could you take a look?

@tchaikov tchaikov self-assigned this Feb 7, 2017

@tchaikov tchaikov merged commit df540ba into ceph:master Feb 8, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@tchaikov

This comment has been minimized.

@wjwithagen wjwithagen deleted the wjwithagen:wip-wjw-freebsd-ceph-disk branch Feb 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment