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: run partprobe/partx after zap and data partition creation #2648

Merged
8 commits merged into from Nov 19, 2014

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Oct 6, 2014

Not running partprobe after zapping a device can lead to the following:

  • ceph-disk prepare /dev/loop2
  • links are created in /dev/disk/by-partuuid
  • ceph-disk zap /dev/loop2
  • links are not removed from /dev/disk/by-partuuid
  • ceph-disk prepare /dev/loop2
  • some links are not created in /dev/disk/by-partuuid

This is assuming there is a bug in the way udev events are handled by
the operating system.

http://tracker.ceph.com/issues/9665
http://tracker.ceph.com/issues/9721

@ghost ghost added the bug fix label Oct 7, 2014

@ghost ghost changed the title from DNM: ceph-disk: run partprobe after zap to ceph-disk: run partprobe after zap Oct 7, 2014

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 7, 2014

@jecluis this is a minor bug fix with ... docker powered tests ;-)

ghost commented Oct 7, 2014

@jecluis this is a minor bug fix with ... docker powered tests ;-)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 8, 2014

@jecluis do you have other reservations about the docker helper ? I will move the initialization into a docker file as you suggested ( i.e. https://github.com/dachary/ceph/commit/50bc5b189e4f465a6bc2330054c9a7714265c560#diff-74a0b360984aea2adfc7358e2952c655R62 )

ghost commented Oct 8, 2014

@jecluis do you have other reservations about the docker helper ? I will move the initialization into a docker file as you suggested ( i.e. https://github.com/dachary/ceph/commit/50bc5b189e4f465a6bc2330054c9a7714265c560#diff-74a0b360984aea2adfc7358e2952c655R62 )

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 8, 2014

gitbuilder is green (it does not run the test because it does not have docker anywhere)

ghost commented Oct 8, 2014

gitbuilder is green (it does not run the test because it does not have docker anywhere)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 9, 2014

@jecluis I'm not sure how to do that in a Dockerfile

useradd -M --uid $(id -u) $USER

which is intended to create a user that has the same name and user id as the user running the docker command. Hint ?

ghost commented Oct 9, 2014

@jecluis I'm not sure how to do that in a Dockerfile

useradd -M --uid $(id -u) $USER

which is intended to create a user that has the same name and user id as the user running the docker command. Hint ?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 9, 2014

@jecluis found it:

cat ubuntu.docker | ID=$(id -u) envsubst | docker build --tag=ceph-ubuntu-14.04 -

ghost commented Oct 9, 2014

@jecluis found it:

cat ubuntu.docker | ID=$(id -u) envsubst | docker build --tag=ceph-ubuntu-14.04 -
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 9, 2014

@jecluis using docker build makes it a lot simpler indeed: 021a2a7

ghost commented Oct 9, 2014

@jecluis using docker build makes it a lot simpler indeed: 021a2a7

@theanalyst

This comment has been minimized.

Show comment
Hide comment
@theanalyst

theanalyst Oct 10, 2014

Member

the dockerfiles LGTM

Member

theanalyst commented Oct 10, 2014

the dockerfiles LGTM

@ghost ghost changed the title from ceph-disk: run partprobe after zap to ceph-disk: run partprobe/partx after zap and data partition creation Oct 10, 2014

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 16, 2014

rebased and repushed (no conflict)

ghost commented Oct 16, 2014

rebased and repushed (no conflict)

@ghost ghost added the core label Nov 5, 2014

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 8, 2014

@jecluis would you have time to look into this ? Low priority ;-)

ghost commented Nov 8, 2014

@jecluis would you have time to look into this ? Low priority ;-)

@loic-bot

This comment has been minimized.

Show comment
Hide comment
@loic-bot

loic-bot Nov 10, 2014

SUCCESS make check
origin https://github.com/dachary/ceph (fetch)
v0.56-13476-g55d1034

loic-bot commented Nov 10, 2014

SUCCESS make check
origin https://github.com/dachary/ceph (fetch)
v0.56-13476-g55d1034

@loic-bot

This comment has been minimized.

Show comment
Hide comment
@loic-bot

loic-bot Nov 10, 2014

SUCCESS make check
origin https://github.com/dachary/ceph (fetch)
af74fb9

loic-bot commented Nov 10, 2014

SUCCESS make check
origin https://github.com/dachary/ceph (fetch)
af74fb9

@liewegas liewegas self-assigned this Nov 11, 2014

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Nov 11, 2014

Member

@dachary ping me tomorrow? i'd like get docker on rex002 and then test

Member

liewegas commented Nov 11, 2014

@dachary ping me tomorrow? i'd like get docker on rex002 and then test

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 11, 2014

rebased and repushed

ghost commented Nov 11, 2014

rebased and repushed

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 11, 2014

@liewegas docker is installed on rex002 and your user is in the docker group, docker ps should work. Make sure you --enable-docker

ghost commented Nov 11, 2014

@liewegas docker is installed on rex002 and your user is in the docker group, docker ps should work. Make sure you --enable-docker

@loic-bot

This comment has been minimized.

Show comment
Hide comment

Loic Dachary added some commits Oct 5, 2014

doc: update debian compilation dependencies
Using the content of debian/control.

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
ceph-disk: implement init=none for block devices
Signed-off-by: Loic Dachary <loic-201408@dachary.org>
tests: helper to run unit / function tests in docker
For instance

   test/docker-test.sh --os-type ubuntu --os-version 14.04 \
        test/ceph-disk.sh

runs test/ceph-disk.sh in a ubuntu 14.04 docker container. Once the
container is populated and ceph compiled, running a test script roughly
requires entering the container and running make TESTS=tests/foo.sh check

* docker build ceph-ubuntu-14.04 using ubuntu.dockerfile as a Dockerfile
* it will run apt-get install ceph compilation / run dependencies
* git clone the-local-clone ceph-ubuntu-14.04
* docker run ceph-ubuntu-14.04 make -j4 in the ceph-ubuntu-14.04 clone
* docker run test/ceph-disk.sh

test/docker-test.sh is the command line interface for
test/docker-test-helper.sh which can be invoked from shell scripts.
test/ubuntu.dockerfile and test/ubuntu.dockerfile are regular
Dockerfiles which allow substitution of environment variables.

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
ceph-disk: test prepare / activate on a device
This indirectly tests that partprobe is called after zap because it
would fail to map the partitions to /dev/disk/by-partuuid otherwise.

It also indirectly test the implementation of init=none when using a
block device because the test would fail to put an object into the rbd
pool using the device otherwise.

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
autotools: add --enable-docker
Docker based tests should be explicit instead of auto-detected. It is
good that they do not run if docker is not available. It would be bad if
they run when the developer does not expect them to create docker
containers.

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
ceph-disk: run partprobe after zap
Not running partprobe after zapping a device can lead to the following:

* ceph-disk prepare /dev/loop2
* links are created in /dev/disk/by-partuuid
* ceph-disk zap /dev/loop2
* links are not removed from /dev/disk/by-partuuid
* ceph-disk prepare /dev/loop2
* some links are not created in /dev/disk/by-partuuid

This is assuming there is a bug in the way udev events are handled by
the operating system.

http://tracker.ceph.com/issues/9665 Fixes: #9665

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
ceph-disk: encapsulate partprobe / partx calls
Add the update_partition function to reduce code duplication.
The action is made an argument although it always is -a because it will
be -d when deleting a partition.

Use the update_partition function in prepare_journal_dev

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
ceph-disk: use update_partition in prepare_dev and main_prepare
In the case of prepare_dev the partx alternative was missing and is not
added because update_partition does it.

http://tracker.ceph.com/issues/9721 Fixes: #9721

Signed-off-by: Loic Dachary <loic-201408@dachary.org>
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 18, 2014

rebased and repushed

ghost commented Nov 18, 2014

rebased and repushed

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Nov 19, 2014

Member

@dachary These changes all look good to me. If you're comfortable with the testing, I say merge it!

Member

liewegas commented Nov 19, 2014

@dachary These changes all look good to me. If you're comfortable with the testing, I say merge it!

ghost pushed a commit that referenced this pull request Nov 19, 2014

Loic Dachary
Merge pull request #2648 from dachary/wip-9665-ceph-disk-partprobe
ceph-disk: run partprobe/partx after zap and data partition creation

Reviewed-by: Sage Weil <sage@redhat.com>

@ghost ghost merged commit 4f37c50 into ceph:master Nov 19, 2014

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 19, 2014

@liewegas I'm not worried about testing since it's deactivated by default. It opens an era of docker based functional tests ;-)

ghost commented Nov 19, 2014

@liewegas I'm not worried about testing since it's deactivated by default. It opens an era of docker based functional tests ;-)

This issue was closed.

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