-
Notifications
You must be signed in to change notification settings - Fork 559
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
rbd: add backend support for VolumeGroup operations #4719
rbd: add backend support for VolumeGroup operations #4719
Conversation
b71a67e
to
beaf170
Compare
librbd "github.com/ceph/go-ceph/rbd" | ||
|
||
"github.com/ceph/ceph-csi/internal/rbd/types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rearrange the order here? cephcsi import should come first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this guideline is not followed everywere. I think and Golang suggests to have imports of local packages at the bottom, that seems to be a coding convention many other Go projects use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/ceph/ceph-csi/blob/devel/docs/coding.md#imports this is one we have, lets stick to it and also update other places if its not followed as followup cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4721. I don't think we should strictly enforce it for existing imports, but it would be nice to use this order in new files.
dc80fab
to
7a758aa
Compare
7a758aa
to
a34d7fe
Compare
a34d7fe
to
eca8a7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let me know once its tested, i will do final review and approve it.
eca8a7a
to
5c8bdb0
Compare
6dec5e0
to
e214d05
Compare
e214d05
to
ccd5536
Compare
LGTM |
1bc0ca0
to
75ccdcc
Compare
5746225
to
f552d4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nits.
There's 3 commits named rbd: implement the VolumeGroup interface
that I think can be squashed together.
f552d4d
to
3362475
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks LGTM
@Mergifyio rebase |
Add support for adding and removing the RBD-image from a group. Signed-off-by: Niels de Vos <[email protected]>
Signed-off-by: Niels de Vos <[email protected]>
Signed-off-by: Niels de Vos <[email protected]>
Signed-off-by: Niels de Vos <[email protected]>
Signed-off-by: Niels de Vos <[email protected]>
Add extra error checking to make sure trying to create an existing volume group does not result in a failure. The same counts for deleting a non-existing volume group, and adding/removing volumes to/from the volume group. Signed-off-by: Niels de Vos <[email protected]>
A RBD image can only be part of a single group. While an image is added to a group, check if the image is already part of a group, and return an error in case it is. Signed-off-by: Niels de Vos <[email protected]>
✅ Branch has been successfully rebased |
3362475
to
1825c27
Compare
@Mergifyio queue |
🛑 The pull request has been removed from the queue
|
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/mini-e2e/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.29 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/k8s-e2e-external-storage/1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.30 |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/mini-e2e/k8s-1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e/k8s-1.28 |
@Mergifyio requeue The multi-arch-build CI job failed while installing some packages. It has been restarted and has passed the section that previously failed. |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Describe what this PR does
Implementation of the VolumeGroup backend. It contains a clear separation of the VolumeGroup type and functions and CSI-Addons responsibilities.
Is there anything that requires special attention
Tested with a small application that calls the CSI-Addons procedures directly. The application was modified several times to test different scenarios. At the moment I am confident that the basic functionality works as expected.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>
: retest the<job-name>
after unrelatedfailure (please report the failure too!)