-
Notifications
You must be signed in to change notification settings - Fork 30
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
Split controller and node binaries #95
base: main
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
add to whitelist |
retest this please |
@jarrpa can you give some more context why we need two separate binaries? |
After taking a quick look, I think this is going to need a design doc and significant discussion. Perhaps I'm misunderstanding things like the This seems like more than "split controller and node binaries" |
@Madhu-1 This allows logic isolation of the different roles, and is a precursor to bringing in different types of Controllers, such as for heketi and gluster-block. @JohnStrunk Yes. The reason for the cache is because when not using default REST parameters there is no way to communicate with the target Gluster cluster from a DeleteVolume request. This is the result of me accidentally squashing two patches together, because I did the REST changes first and the split second. I'll try to untangle them after the fact. |
Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>
@JohnStrunk Patches untangled, PTAL |
I'll also note that I do have a local version of |
I'm still not understanding the motivation. Now each type of CSI driver has multiple containers that must be built. |
That's not true. Each CSI driver just needs to build its own Controller container. We can keep one Node container compatible with multiple Controllers. This is what I do in my WIP code for having a heketi Controller driver, a gd2 Controller driver, and a GlusterFS Node driver that works with both. This would then allow you to, for example, update the Controller containers without touching the Node container. |
It should be noted, the way I designed my multi-Controller implementation was that both Controller pods listened as the |
Only having a Controller container may work for Heketi vs. GD2 for gluster-fuse, but I'm skeptical that a common node container works in all cases. Again, what does this get us? My point is that having a combined binary and single container model doesn't provide any less code sharing while keeping it simple... For Heketi, use image 1 everywhere; for GD2, use image 2 everywhere. Later, when you want to use g-b, that's image 3, and something else like loopback is image 4, etc. Keeping track of which Controller container I can use with which node container on top of that sounds like it's asking for errors in deployment, so if it doesn't get us something really tangible, why do it? |
There was already talk of having a separate driver for gluster-block, at the very least, under the name of something like I personally see no tangible benefit in keeping things in a single binary, so it seems only natural to split the binaries at logical boundaries for crash isolation at the very least. |
Yes, each driver should have its own binary and container. That binary supports the entire set of supported CSI APIs for that driver. This patch has been about potentially having multiple binaries and containers per driver by splitting the CSI API implementation across binaries and containers. Today, we have a single binary that is deployed in the provisioner, attacher, and node roles (all as separate pods). Only a portion of the API is used in each of those roles, but the unified binary keeps it all together as a single unit that is built, tested, and deployed as a consistent version. Requiring an admin to specify different images for each is simply unnecessary and it increases the likelihood of error. It's also a burden on QE and the maintainers to ensure all images are built and in-sync. |
These are non-existent burdens if our automation is at least half-way decent. You don't have to make the admin remember different versions, just tag all the images with the same versions if you want. And again, this all becomes moot with an Operator to handle the versioning for you. If all else fails, just build all container images every time one of them changes and version all of them, with a stated policy that we only support deployments with matching tags between driver images. The primary reason for all this is that I don't want to have two identical node drivers running per node in the case where a cluster is running both heketi- and gd2-based controller drivers. The only way to accomplish this would be to have a single driver that handles both heketi and gd2 APIs dynamically, which I believe we also wanted to avoid. Given all this, I still see no good reason not to do this, especially given the benefit of keeping as much logic separated as possible. |
@JohnStrunk any update on this PR? |
I have nothing to add to my previous comments. |
@JohnStrunk If you don't have anything else to add, that would indicate that you do not refute the final points I made. Is that the case, and if so may this PR go through? |
In the interest of moving some things along, I created #110 to merge the work of at least splitting the driver into different libraries. |
This PR splits the controller and node CSI roles into two separate binaries. Driver tests are not currently implemented as the CSI sanity test suite does not yet support split driver roles (PR submitted for that: kubernetes-csi/csi-test#135).
Signed-off-by: Jose A. Rivera jarrpa@redhat.com