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

librbd: disallow creation of v1 image format #20460

Merged
merged 4 commits into from Apr 3, 2018

Conversation

Projects
None yet
3 participants
@colletj
Copy link
Contributor

commented Feb 16, 2018

V1 image format has been deprecated since the jewel release, so
forbit it. However some internal test cases still need v1 images
so allow their creation if the RBD_FORCE_ALLOW_V1 env var is set.

Signed-off-by: Julien Collet julien.collet@cern.ch

@colletj

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2018

@dillaman This is a work-in-progress, is this what you had in mind ?

@batrick batrick added the rbd label Feb 16, 2018

@dillaman
Copy link
Contributor

left a comment

Yup -- basically something like this and then update the unit tests / integration tests to function again

@@ -869,7 +869,13 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2)
}

if (old_format) {
r = create_v1(io_ctx, image_name.c_str(), size, order);
if ( !getenv("RBD_FORCE_ALLOW_V1") ) {
lderr(cct) << "Format 1 image creation deprecated. " << dendl;

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 19, 2018

Contributor

Nit: deprecated -> unsupported

@colletj colletj force-pushed the colletj:v1_image_creation_disallow branch from b78f005 to 4d67c90 Feb 20, 2018

@dillaman
Copy link
Contributor

left a comment

I would just change src/test/librbd/test_main.cc to include a setenv. There is also the python bindings test case in src/test/pybind/test_rbd.py and lots of standalone integration tests under qa/workunits/rbd that attempt to create v1 images.

if ( !getenv("RBD_FORCE_ALLOW_V1") ) {
lderr(cct) << "Format 1 image creation unsupported. " << dendl;
return -EINVAL;
} else {

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 22, 2018

Contributor

Nit: you can drop the else clause since the previous clause will return.

@colletj

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

Okay I'll have a look, thanks for pointing this out.

@@ -124,25 +124,25 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2)
} // anonymous namespace

int detect_format(IoCtx &io_ctx, const string &name,
bool *old_format, uint64_t *size)
bool *old_format, uint64_t *size)

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 28, 2018

Contributor

Nit: try to avoid changing tabs to spaces -- unless the lines are directly affected by the PR. While I prefer spaces, technically tabs are allowed in the project.

librbd: disallow creation of v1 image format
V1 image format has been deprecated since the jewel release, so
forbit it. However some internal test cases still need v1 images
so allow their creation if the RBD_FORCE_ALLOW_V1 env var is set.

Signed-off-by: Julien Collet <julien.collet@cern.ch>

@colletj colletj force-pushed the colletj:v1_image_creation_disallow branch from 264cb00 to a46b0b9 Mar 1, 2018

@colletj colletj changed the title DNM: librbd: disallow creation of v1 image format librbd: disallow creation of v1 image format Mar 1, 2018

@dillaman dillaman added the cleanup label Mar 5, 2018

@@ -14,4 +14,6 @@ do
RBD_FEATURES=$i unittest_librbd
done

unset RBD_FORCE_ALLOW_V1

This comment has been minimized.

Copy link
@dillaman

dillaman Mar 5, 2018

Contributor

Nit: why is this needed?

This comment has been minimized.

Copy link
@colletj

colletj Mar 6, 2018

Author Contributor

It seems it's not -- I fixed it.
Thanks for your feedback

test/librbd: use v1 images in the rbd unit tests
Update the test/qa scripts to support the v1 creation disallow fix (using
the RBD_FORCE_ALLOW_V1 environment variable).

Signed-off-by: Julien Collet <julien.collet@cern.ch>

@colletj colletj force-pushed the colletj:v1_image_creation_disallow branch from a46b0b9 to 18d9a74 Mar 6, 2018

@dillaman
Copy link
Contributor

left a comment

lgtm

librbd/qa: Permit V1 images in qa tests
Signed-off-by: Julien COLLET <julien.collet@cern.ch>
@dillaman
Copy link
Contributor

left a comment

lgtm

@dillaman

This comment has been minimized.

colletj referenced this pull request in colletj/ceph Mar 27, 2018

Fix cli-integration test
Signed-off-by: Julien COLLET <julien.collet@cern.ch>
@colletj

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@dillaman, PR updated. Do you think you could relaunch the tests, please ?

@dillaman dillaman added the needs-qa label Mar 29, 2018

@dillaman

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2018

@colletj Still having an issue w/ the cram test -- I think the semi-colon needs to be removed between the environment variable and the rbd CLI.

http://qa-proxy.ceph.com/teuthology/jdillaman-2018-03-29_21:54:26-rbd-wip-jd-testing-distro-basic-smithi/2334466/teuthology.log

librbd/cli-integration: Permit V1 image creation
Signed-off-by: Julien COLLET <julien.collet@cern.ch>

@colletj colletj force-pushed the colletj:v1_image_creation_disallow branch from 53d8d15 to bd750f8 Mar 30, 2018

@colletj

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

@dillaman Thanks for the feedback, I fixed it. Could you give it another try, please ?

@dillaman

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

Manually merged to master

@dillaman dillaman closed this Apr 3, 2018

@dillaman dillaman merged commit bd750f8 into ceph:master Apr 3, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

dillaman added a commit that referenced this pull request Apr 3, 2018

Merge pull request #20460 from colletj/v1_image_creation_disallow
librbd: disallow creation of v1 image format

Reviewed-by: Jason Dillaman <dillaman@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.