DeviceManager/lvm: Remove device from cache on delete

[avalluri]

LVM device manager caches the devices that it creates, but not cleaning up those
device references on delete. This change fixes this issue.

[okartau]

caches the devices that it creates, but not cleaning up those

yes, I noticed this when idempotency-enabled testing in #393 started to fail after uuid->hash change. Problem remained hidden before because we always used fresh uuid so old entries did not surface in checking. With repeatable hash as VolumeID, we hit the previous entry in the cache.

But about LV entry cache: do we really need this? Is it meant for performance?
We have state of volumes now (at least) at 3 levels: Controller has set Volume registry, Node has own, and LVM layer has one more of it's own. With every such state layer, we increase the risk that something gets out of sync. PMEM-CSI driver is meant for long run and bad things may happen, so we should minimize risk of getting states out of sync.
I don't think we need to value perf. gain of cache over reliability risks.
Also, similar layer on ndctl side does not keep track of volumes separately.

[avalluri]

Is it meant for performance?

Yes, When doing this change I felt like, almost every operation in LVM mode has the need to fetch the current list of devices from lvm. So instead of parsing lvm output on every call, cache the device info is better.

We have state of volumes now (at least) at 3 levels: Controller has set Volume registry, Node has own,

Information at the Controller is different from Node.

and LVM layer has one more of it's own. With every such state layer, we increase the risk that something gets out of sync.

I agree.

Also, similar layer on ndctl side does not keep track of volumes separately.

Because it's not executing ndctl binary, instead it's using library calls.

[okartau]

Information at the Controller is different from Node.

true, I was just counting where we create some data entries when a Volume is created.
In LVM mode:

0. we have LVM volume itself
1. we have cache entry in lvm.devices
2. we have entry in Node Volume registry
3. we have entry in Controller Volume registry

The players are Node, Controller, and k8s (and possibly even more, some sidecars).
With rapid RPC, messages sent, repeated, delayed, lost, or mutexes (which we also have at multiple players and layers) being waited, the PMEM-CSI will face so many combinations of possible errors that it is impossible to test everything and in long run those 4 entities may get out of sync.
Debugging those problems is difficult/impossible without detailed logs (which are not enabled in production)
That's why I am looking for ways to reduce the amount of state we maintain.
If some LVM operation takes some (minor) extra time, it is no problem whatsoever, as provisioning of Volume is not generally time-critical operation, right.

[pohly]

I don't think we need to value perf. gain of cache over reliability risks.

I fully agree.

[pohly]

Before we add complex code for the sake of performance we have to have:

• a need to improve performance, i.e. the operation is known to be too slow
• tests that measure performance and detect regressions

[pohly]

Can we clean up the Go style before merging?

[pohly]

Please use err :=, it's better Go style.

[avalluri]

Done.

[pohly]

Same here.

[avalluri]

Done