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_fuse: pid_file default to empty #12628

Merged
merged 4 commits into from Jan 6, 2017

Conversation

Projects
None yet
4 participants
@smithfarm
Contributor

smithfarm commented Dec 22, 2016

Fixes: http://tracker.ceph.com/issues/18309
Signed-off-by: Nathan Cutler ncutler@suse.com

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Dec 22, 2016

For reference:

@@ -78,7 +78,10 @@ int main(int argc, const char **argv, const char *envp[]) {
}
env_to_vec(args);
auto cct = global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT,
std::vector<const char*> def_args;
def_args.push_back("--pid-file="); // pid_file default to empty

This comment has been minimized.

@tchaikov

tchaikov Dec 22, 2016

Contributor

nit, could just put

std::vector<const char*> def_args{"--pid-file="};
@tchaikov

apart from the nit, lgtm. and i love this approach more =)

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Dec 22, 2016

This should make fs/recovery/{begin.yaml clusters/4-remote-clients.yaml fs/xfs.yaml mount/fuse.yaml objectstore/filestore.yaml overrides/{debug.yaml frag_enable.yaml fuse-unmounted.yaml whitelist_wrongly_marked_down.yaml} tasks/volume-client.yaml} succeed.

Pushed wip-18309-alt to ceph-ci for build.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Dec 22, 2016

Changelog:

  • standardize on std::vector even though there is a using namespace std at the top of the file
  • set def_args via {} as suggested by @tchaikov
  • add commit to remove dead code following usage() call
@liewegas

lgtm

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Dec 22, 2016

teuthology-suite -s fs -c wip-18309-alt --filter "recovery/{begin.yaml clusters/4-remote-clients.yaml fs/xfs.yaml mount/fuse.yaml objectstore/filestore.yaml overrides/{debug.yaml frag_enable.yaml fuse-unmounted.yaml whitelist_wrongly_marked_down.yaml} tasks/volume-client.yaml}" --priority 101 --email ncutler@suse.cz -m smithi (with patch - should succeed)

fail http://pulpito.front.sepia.ceph.com:80/smithfarm-2016-12-23_16:05:43-fs-wip-18309-alt---basic-smithi/

teuthology-suite -c master --ceph-repo https://github.com/ceph/ceph.git --suite-repo https://github.com/ceph/ceph.git -s fs --filter "recovery/{begin.yaml clusters/4-remote-clients.yaml fs/xfs.yaml mount/fuse.yaml objectstore/filestore.yaml overrides/{debug.yaml frag_enable.yaml fuse-unmounted.yaml whitelist_wrongly_marked_down.yaml} tasks/volume-client.yaml}" --priority 101 --email ncutler@suse.cz -m smithi -n 10 (without patch, should fail)

fail http://pulpito.front.sepia.ceph.com:80/smithfarm-2016-12-23_16:08:27-fs-master---basic-smithi/

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Dec 23, 2016

Both tests appear to fail because of the pidfile issue.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jan 4, 2017

@smithfarm weird. I had a quick look through arg parsing code and I can't see why the --pidfile= is being ignored.

@tchaikov tchaikov self-assigned this Jan 4, 2017

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jan 5, 2017

Ah, this is failing because the teuthology ceph.conf.template has pid file = /var/run/ceph/$cluster-$name.pid in [global]

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 5, 2017

So we need to add something like

overrides:
  ceph:
    conf:
      global:
        pid file = ""

to the test definition?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 5, 2017

(I suspected that pid file name was coming from the test, but I grepping for it in qa/ turned up nothing, now I know why)

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 5, 2017

@jcsp Something like in the commit I just added?

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jan 5, 2017

@smithfarm yeah, but we probably want to set the override for client rather than global to avoid collaterally disabling pidfiles for daemons in the tests.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jan 5, 2017

This would still cause issues for administrators if they had set their own global pid file setting the way teuthology does, but I think I'm okay with that -- the alternative would be squashing any existing pid_file setting from ceph_fuse, which would remove the admin's ability to optionally have the fuse client generate a pid file (not sure why they'd want to, but doesn't seem completely inconceivable)

@jcsp jcsp assigned smithfarm and unassigned tchaikov and jcsp Jan 5, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 5, 2017

@jcsp The test in question has four clients, client.[0123], but I'm guessing that

overrides:
  ceph:
    conf:
      client:
        pid file: ""

will cover them all (?) ...

smithfarm added some commits Dec 22, 2016

ceph_fuse: pid_file default to empty
Fixes: http://tracker.ceph.com/issues/18309
Signed-off-by: Nathan Cutler <ncutler@suse.com>
fuse: remove dead code after usage() call
usage() calls generic_client_usage(), which calls exit(-1), which never
returns.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 5, 2017

Hm, the no-client-pidfile.yaml is so general, maybe qa/overrides is a better place for it.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 5, 2017

teuthology-suite -s fs -c wip-18309-alt --filter "recovery/{begin.yaml clusters/4-remote-clients.yaml fs/xfs.yaml mount/fuse.yaml objectstore/filestore.yaml overrides/{debug.yaml frag_enable.yaml fuse-unmounted.yaml no-client-pidfile.yaml whitelist_wrongly_marked_down.yaml} tasks/volume-client.yaml}" --priority 101 --email ncutler@suse.cz -m smithi (with patch - should succeed)

pass http://pulpito.ceph.com:80/smithfarm-2017-01-06_10:17:36-fs-wip-18309-alt---basic-smithi/

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 6, 2017

teuthology-suite -s fs -c wip-18309-alt --filter "recovery/{begin.yaml clusters/4-remote-clients.yaml fs/xfs.yaml mount/fuse.yaml objectstore/filestore.yaml overrides/{debug.yaml frag_enable.yaml fuse-unmounted.yaml no_client_pidfile.yaml whitelist_wrongly_marked_down.yaml} tasks/volume-client.yaml}" --priority 101 --email ncutler@suse.cz -m smithi (with patch - should succeed)

pass http://pulpito.front.sepia.ceph.com:80/kchai-2017-01-06_12:31:35-fs-wip-18309-alt---basic-mira/

tests: override yaml to set client pid file to empty string
Due to http://tracker.ceph.com/issues/18309 the pid file for fuse clients
should always be set to the empty string. (Teuthology's default ceph.conf
sets it to /var/run/ceph/$cluster-$name.pid)

This commit adds a reusable yaml facet for this purpose.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
tests: add no_client_pidfile override to fs/recovery tests
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 6, 2017

retest this please

Submodule path 'src/erasure-code/jerasure/gf-complete': checked out 'd61b4d377a6a3f202de8a3541abf6383313adab7'
Cloning into 'src/erasure-code/jerasure/jerasure'...
fatal: unable to access 'https://github.com/ceph/jerasure.git/': Could not resolve host: github.com
Clone of 'https://github.com/ceph/jerasure.git' into submodule path 'src/erasure-code/jerasure/jerasure' failed
...
-- Performing Test HAVE_INTEL_SSE4_2 - Success
CMake Error at src/CMakeLists.txt:568 (add_subdirectory):
  The source directory

    /home/jenkins-build/build/workspace/ceph-pull-requests/src/lua

  does not contain a CMakeLists.txt file.

seems the submodules are not fully sync'ed due to networking issues.

@tchaikov

This comment has been minimized.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 6, 2017

@tchaikov Thanks! If you and @jcsp are satisfied with the last two commits, I guess it's good to merge.

@tchaikov tchaikov merged commit 0e0b7a0 into ceph:master Jan 6, 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
@jcsp

This comment has been minimized.

Contributor

jcsp commented Jan 6, 2017

👍 thanks @smithfarm

@smithfarm smithfarm deleted the SUSE:wip-18309-alt branch Jan 6, 2017

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