[WIP] collect metadata in k8s cm #102
Conversation
|
Thanks for the work! The general idea seems ok. For the first round of comments: Non-atomic operations on a shared ConfigMap introduce race conditionsThere's a race condition in read->modify->write when adding/deleting objects in a shared ConfigMap resulting in loosing the result of either of those operations. Unrelated to the race condition but we should also take the size limit of a single CM object into the account - etcd is limiting us at 1MB per object which seems enough but since this implementation is using a single CM for all volumes it manages, maybe this is something to look out for. Reusing metadata persistence code in both driversThis one is just for refactoring so that we don't have the same thing implemented twice. Perhaps we could add a new package under type CachePersister interface {
Get(..., data interface{}) error
Update(..., data interface{}) error
Delete(...) error
}Be more explicit about the type of metadata storageInstead of having a bool @rootfs thoughts? |
|
Thanks for the feedback @gman0 !
I haven't thought about it... Do you have an idea how to handle both of these cases? |
|
@mickymiek for a quick start, you can make one cm per volume. In the cm, add a special label so when we list all volumes, we can list by label. You can find an example here |
|
cc @rohan47 |
| return nil | ||
| } | ||
|
|
||
| func (k8scm *K8sCMCache) Create(identifier string, data interface{}) error { |
There was a problem hiding this comment.
What are the use-cases for overwriting an existing cache entry? IMHO this is out-of-scope for Create and I'd say if an entry with this identifier already exists just exit with success, otherwise create a new CM object (and return success in case of an "ALREADY EXISTS" error). That way you also get rid of locking explicitly.
There was a problem hiding this comment.
Still a pending issue. Is there a reason why the CM is being updated here? If so please post a link to the code, maybe I'm missing something. This method should only:
- Try to
Getthe CM object (this is to handle idempotency)
1.1 If it already exists,return nilas to say everything is ok - Otherwise,
Createa new CM object
2.2 If it already exists, also return success
|
Also, please run |
|
Updated, PTAL @gman0! |
|
Looks better, I'll post more comments on weekend. |
| } | ||
|
|
||
| var ( | ||
| controllerCacheMap = make(map[volumeID]*controllerCacheEntry) |
There was a problem hiding this comment.
Where is this actually used?
| @@ -125,9 +155,10 @@ func (cs *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol | |||
| defer func() { | |||
There was a problem hiding this comment.
As we discussed offline, this whole "cache entry reinsert" block can be removed.
| if err := cs.MetadataStore.Get(string(volId), ce); err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| if err := cs.MetadataStore.Delete(string(volId)); err != nil { |
There was a problem hiding this comment.
Delete the cache entry as the last step of this procedure (i.e. just before returning)
| } | ||
|
|
||
| const ( | ||
| oneGB = 1073741824 |
There was a problem hiding this comment.
Leftovers from the merge conflicts ;)
| cmLabel = "csi-metadata" | ||
| cmDataKey = "content" | ||
|
|
||
| csiMetadataLabelAttr = "com.ceph.ceph-csi/persister" |
There was a problem hiding this comment.
Just a nit: perhaps metadata would be more descriptive here than persister?
| if err != nil { | ||
| cm, err = k8scm.createMetadataCM(identifier) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "already exists") { |
There was a problem hiding this comment.
The correct way to check for various kinds of errors is with apierrors pkg, see this for example https://github.com/kubernetes/kubernetes/blob/7f23a743e8c23ac6489340bbb34fa6f1d392db9d/test/utils/create_resources.go#L191
There was a problem hiding this comment.
Also, existing CM is not an error
|
|
||
| **Available environmental variables:** | ||
| `HOST_ROOTFS`: rbdplugin searches `/proc` directory under the directory set by `HOST_ROOTFS`. | ||
| `KUBERNETES_CONFIG_PATH`: if you use `k8s_configmap` as metadata store, specify the path of your k8s config file (if not specified, the plugin will assume you're running it inside a k8s cluster and find the config itself). |
There was a problem hiding this comment.
Add this to cephfs docs as well
| if err != nil { | ||
| return errors.Wrapf(err, "k8s-cm-cache: couldn't delete metadata configmap %s", identifier) | ||
| } | ||
| glog.Infof("k8s-cm-cache: successfully deleted metadata configmap %s", identifier) |
There was a problem hiding this comment.
If Delete prints a log msg, so should Create
| if err = encoder.Encode(data); err != nil { | ||
| return errors.Wrapf(err, "node-cache: failed to encode metadata for file: %s\n", file) | ||
| } | ||
| glog.Infof("node-cache: successfully saved metadata into file: %s\n", file) |
This PR gives an options to persist RBD and CephFS metadata (currently stored on node) in a k8s configmap.
Related task: #66