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

Replaced config map usage with, omaps and volume ID generation #288

Closed

Conversation

ShyamsundarR
Copy link
Contributor

@ShyamsundarR ShyamsundarR commented Mar 26, 2019

Existing config maps are now replaced with rados omaps that help
store information regarding the requested volume names and the rbd
image names backing the same.

Further to detect cluster, pool and which image a volume ID refers
to, changes to volume ID encoding has been done as per provided
design specification in the stateless ceph-csi proposal.

Signed-off-by: ShyamsundarR srangana@redhat.com


This change is Reviewable

plnordquist and others added 30 commits December 10, 2018 11:25
Signed-off-by: Peter Nordquist <peter.nordquist@pnnl.gov>
Added Helm chart for RBD plugin
Without this check, the driver fails one of the E2E storage tests in
Kubernetes 1.13: provisioning a block volume is expected to fail in
https://github.com/kubernetes/kubernetes/blob/e689d515f77cda51898045d626ec4070e3328194/test/e2e/storage/testsuites/volumemode.go#L329-L330
Signed-off-by: Peter Nordquist <peter.nordquist@pnnl.gov>
Added docs for deploying rbd driver with Helm
Added snapshotter deployments files + instuctions to deploy
Signed-off-by: Huamin Chen <hchen@redhat.com>
fix cache persistent default
Signed-off-by: Huamin Chen <hchen@redhat.com>
fix cache persistent default for cephfs
pkg/rbd/rbd.go Outdated Show resolved Hide resolved
pkg/rbd/rbd_util.go Outdated Show resolved Hide resolved
pkg/rbd/rbd_util.go Outdated Show resolved Hide resolved
pkg/rbd/rbd.go Outdated Show resolved Hide resolved
pkg/rbd/rbd.go Outdated Show resolved Hide resolved
pkg/rbd/rbd_util.go Outdated Show resolved Hide resolved
pkg/util/volid.go Outdated Show resolved Hide resolved
pkg/rbd/rbd.go Show resolved Hide resolved
pkg/util/cephcmds.go Outdated Show resolved Hide resolved
pkg/util/cephcmds.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments acknowledged as a part of this update. If any comment is not responded to, means I am still thinking about the same, like these 2,
https://github.com/ceph/ceph-csi/pull/288/files#r270048735
https://github.com/ceph/ceph-csi/pull/288/files#r270043890

NOTE: I will wait till this weekend to refresh this PR with the comments addressed, giving myself and others time to add more review comments, such that I can hopefully address them in one go.

I do not see any major objection to the implementation as yet in the comments, if I assume wrong please raise the same explicitly.

cmd/rbd/main.go Show resolved Hide resolved
pkg/rbd/controllerserver.go Show resolved Hide resolved
pkg/rbd/controllerserver.go Show resolved Hide resolved
pkg/rbd/controllerserver.go Show resolved Hide resolved
pkg/rbd/rbd.go Outdated Show resolved Hide resolved
pkg/util/cephcmds.go Outdated Show resolved Hide resolved
pkg/util/cephcmds.go Outdated Show resolved Hide resolved
pkg/util/cephcmds.go Outdated Show resolved Hide resolved
pkg/util/volid.go Outdated Show resolved Hide resolved
ability to parse backward compatible encodings.
- ClusterID: Is a unique ID per cluster that the CSI instance is serving and is restricted to
lengths that can be accommodated in the encoding scheme.
- ImageName: Is the on-disk image name of the CSI volume that corresponds to this volume ID.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, started as a representation for volumes and the name got left behind. Will address naming and related comments in an update to this PR.

I would still like to retain this outside of rbd (i.e in util), as I foresee we would be reusing this in the CephFS plugin code as well, to generate and return Vol|SnapID for volumes from cephfs. objections to retaining these here?

@ShyamsundarR
Copy link
Contributor Author


pkg/rbd/rbd_util.go, line 271 at r1 (raw file):

Previously, dillaman (Jason Dillaman) wrote…

Fair enough. Long term efficiency, we really should just get the entire CSI off using the CLI (and associated parsing of CLI output and error codes) and instead directly use the golang wrapper API [1]

[1] https://github.com/ceph/go-ceph

Agreed, we should move to that at some point in time in the future.

@ShyamsundarR
Copy link
Contributor Author

@dillaman @Madhu-1 @gman0 Addressed review comments in a new commit, please review.

IMO, the code is complete, it does need a fresh PR against master now, but it also needs the deployment and documentation changes to be made. As I get those done, request a review of the code as a part of this PR itself, so that I can address any concerns in the next round of updates.

Items that are unresolved or not going to be resolved in this PR IMO are,

  • Naming of the PoolID field in CsiIdentifier structure in file volid.go file (thinking about that now, the file should be renamed csiid.go :-/ ) This will be done when we move to making cephfs stateless

  • 1:1 clusterID secret to storage class

    • This is a broader change, and IMO encoding the pool ID in the returned VolID, does not preclude moving the pool name to the secret in the future
    • Hence, for now, retaining the existing method from the in-tree and CSI implementation of specifying the pool name in the StorageClass

Madhu-1 and others added 15 commits April 4, 2019 11:11
currently we are deploying external-attacher
as a seperate statefulset, which leads to
attacher communicating with the node provisoner
daemonset, This PR deploys external-attacher
as a sidecar container inside provisioner
statefulset, so that external-provisioner
always communicates with the plugin responsible
for the provision controller capcabilities.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
…isioner pod

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
… pod

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
…rovisioner.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
adding the condition will help us
to easily remove the attacher later.
or even we can add else condition
if we have an alternate to attacher.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
if attacher is not enabled, we need to
create the csidriver CRD with spec
to make attachRequired as false to
skip volume attach check in kube.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Add link to go-report card and
travis build status in readme file.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Replaces the references to Kubernetes Authors in the copyright
add go-report card and travis status
Madhu-1 and others added 3 commits April 5, 2019 12:43
Deploy csi-attacher as sidecar container in provisioner statefulset
Existing config maps are now replaced with rados omaps that help
store information regarding the requested volume names and the rbd
image names backing the same.

Further to detect cluster, pool and which image a volume ID refers
to, changes to volume ID encoding has been done as per provided
design specification in the stateless ceph-csi proposal.

Signed-off-by: ShyamsundarR <srangana@redhat.com>
Retaining this as a separate commit, to eas reviews of the
commit by itself, and ensure review comments are handled.

When the entire set of commits are ready, this would be
merged with the previous commit.

Signed-off-by: ShyamsundarR <srangana@redhat.com>
@ShyamsundarR ShyamsundarR force-pushed the stateless-removeconfigmap branch from 5e8f6ee to da455ee Compare April 8, 2019 11:25
@ShyamsundarR
Copy link
Contributor Author

This is closed, as the PR is moved to master #312 please continue the reviews there.

Madhu-1 pushed a commit to Madhu-1/ceph-csi that referenced this pull request Jun 20, 2024
Syncing latest changes from upstream devel for ceph-csi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.