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

luminous: rbd: librbd: image create request should validate data pool for self-managed snapshot support #24390

Merged
merged 1 commit into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@smithfarm
Contributor

smithfarm commented Oct 3, 2018

@smithfarm smithfarm self-assigned this Oct 3, 2018

@smithfarm smithfarm added this to the luminous milestone Oct 3, 2018

@smithfarm smithfarm added bug fix core rbd and removed core labels Oct 3, 2018

@smithfarm smithfarm requested review from dillaman and trociny Oct 3, 2018

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

jenkins test docs

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/image/CreateRequest.cc: In member function ‘void librbd::image::CreateRequest<ImageCtxT>::validate_data_pool()’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/image/CreateRequest.cc:245:19: error: ‘m_io_ctx’ was not declared in this scope
   m_data_io_ctx = m_io_ctx;
                   ^
src/librbd/CMakeFiles/rbd_internal.dir/build.make:734: recipe for target 'src/librbd/CMakeFiles/rbd_internal.dir/image/CreateRequest.cc.o' failed
make[3]: *** [src/librbd/CMakeFiles/rbd_internal.dir/image/CreateRequest.cc.o] Error 1

@smithfarm smithfarm force-pushed the smithfarm:wip-24946-luminous branch from e51647f to ecc80a4 Oct 3, 2018

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/image/CreateRequest.cc: In member function ‘void librbd::image::CreateRequest<ImageCtxT>::validate_data_pool()’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/image/CreateRequest.cc:255:41: error: ‘class librados::IoCtx’ has no member named ‘get_namespace’
     m_data_io_ctx.set_namespace(m_ioctx.get_namespace());
                                         ^
src/librbd/CMakeFiles/rbd_internal.dir/build.make:734: recipe for target 'src/librbd/CMakeFiles/rbd_internal.dir/image/CreateRequest.cc.o' failed
make[3]: *** [src/librbd/CMakeFiles/rbd_internal.dir/image/CreateRequest.cc.o] Error 1

UPDATE: fixing by simply dropping this line, since (apparently) luminous IoCtx does not have get_namespace/set_namespace.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

The problem here is that, in master, m_io_ctx is declared in src/librbd/image/CreateRequest.h like this:

IoCtx m_io_ctx;

while in luminous the m_ioctx declaration looks like this:

IoCtx &m_ioctx;
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

To preserve that line with get_namespace/set_namespace, my guess is the commit in this PR would need to be cherry-picked after 9c59e68, yet that one appears to depend on 0fca6ea.

So dropping the line would appear to be the better course of action.

@smithfarm smithfarm force-pushed the smithfarm:wip-24946-luminous branch from ecc80a4 to b4d221c Oct 3, 2018

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

jenkins test docs

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

@trociny @dillaman I got it to compile, but it blows up in the unit tests. Any suggestions welcome, including closing the PR :-)

@smithfarm smithfarm added the DNM label Oct 3, 2018

bl.append("overwrite validated");
if (r >= 0 && m_outbl.contents_equal(bl)) {
add_image_to_directory();

This comment has been minimized.

@trociny

trociny Oct 3, 2018

Contributor

@smithfarm Note, in luminus CreateRequest state machine is a little different than in the master (see the diagram in CreateRequest.h). In luminous the next state after VALIDATE_DATA_POOL is CREATE_ID_OBJECT, so you need to call create_id_object() here instead.

@smithfarm smithfarm force-pushed the smithfarm:wip-24946-luminous branch from b4d221c to 94098fb Oct 3, 2018

librbd: validate data pool for self-managed snapshot support
Fixes: https://tracker.ceph.com/issues/24675
Signed-off-by: Mykola Golub <mgolub@suse.com>
(cherry picked from commit 08ea7d6)

Conflicts:
	src/librbd/image/CreateRequest.cc
	src/librbd/image/CreateRequest.h
- luminous uses m_ioctx where master has m_io_ctx
- luminous IoCtx does not have get_namespace/set_namespace
- in luminuos CreateRequest state machine is a little different than in
the master (see the diagram in CreateRequest.h). In luminous the next
state after VALIDATE_DATA_POOL is CREATE_ID_OBJECT, so call
create_id_object() instead of add_image_to_directory().

@smithfarm smithfarm force-pushed the smithfarm:wip-24946-luminous branch from 94098fb to b579df9 Oct 3, 2018

@smithfarm smithfarm changed the title from luminous: image create request should validate data pool for self-managed snapshot support to luminous: librbd: image create request should validate data pool for self-managed snapshot support Oct 3, 2018

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Oct 3, 2018

Thanks, @trociny, it passes make check now. Ready for review :-)

@trociny

trociny approved these changes Oct 3, 2018

LGTM

@yuriw

This comment has been minimized.

Contributor

yuriw commented Oct 3, 2018

@yuriw yuriw merged commit ec337fe into ceph:luminous Oct 4, 2018

4 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

@smithfarm smithfarm deleted the smithfarm:wip-24946-luminous branch Oct 4, 2018

@smithfarm smithfarm changed the title from luminous: librbd: image create request should validate data pool for self-managed snapshot support to luminous: rbd: librbd: image create request should validate data pool for self-managed snapshot support Oct 26, 2018

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