driver: separate controller and node structs

[dhij]

Issue #207

This PR separates the controller and the node parts from the driver struct.

[dhij]

@timoreimann I have not made any changes to the tests but wanted to seek your feedback on the approach I was taking. I embedded the Driver object into the Controller and the Node structs. I am instantiating the Controller and Node using their constructors from main.go but it definitely looks like calling 3 separate Run() is not a smart way to go.

• The main reason for separating the Run() method was because of the csi.RegisterControllerServer(...) and the csi.RegisterNodeServer(...)

[timoreimann]

Hey @dhij 👋

What I think we could do to DRY out the logic is use a variable only used for one of the services / modes as a trigger to distinguish. For us, that could be the token: it is only ever set in Controller mode, and without it we must be running in Node mode.

That way, we can define two separate structs that each have a Run() method with service-specific initialization, with setup logic needed by both being deferred to a shared (possibly embedded) struct.

Does that make sense?

[timoreimann]

Quick addendum to my previous comment: the distinction between which struct to use and return (controller vs node) can probably be made in NewDriver

Existing methods for the corresponding services (living in controller.go and node.go / mount.go, respectively) would then also be moved to one of the structs.

[dhij]

@timoreimann 👋 Timo, yes that makes sense! Just to clarify on the last sentence in your first comment where you said with setup logic needed by both being deferred to a shared (possibly embedded) struct, does that mean we create a shared struct which we then embed into the Driver object?

[timoreimann]

Yes, that's what I had in mind. This shared struct can probably mostly be what the driver is today, so the primary effort may boil down to extracting service-specific variables / fields and methods.

[dhij]

@timoreimann What I have in mind right now are:

• Define an interface with a Run() method
• Have the NewDriver() return the (interface, error) so that it returns Controller OR Node based on the distinguishing variable
• Define Run() method for each Node and Controller structs with service-specific initialization
• Have the (*Driver) Run() method call the corresponding Run() based on the distinguishing variable

Please correct me if the idea above is different from what you had in mind!

Also I read your comment again and wasn't sure what you meant here: Existing methods for the corresponding services (living in controller.go and node.go / mount.go, respectively) would then also be moved to one of the structs. Could you explain this part again?

[timoreimann]

If NewDriver returns one of the implementations, then I'd argue that it should suffice to consult the distinguishing variable for that purpose exclusively, i.e., to decide which implementation is to be returned. Service-specific code can then be encapsulated by the respective Run() implementations.

Looking at the current Driver.Run() code again, we could also consider a slightly different approach: instead of returning Service-specific structs from NewDriver, continue to return the same Driver struct we use today but extend it with a new field that holds an instance of the new interface (i.e., what is implemented by the Controller or Node structs). Then, in the Driver.Run() implementation, we can defer to invoking the field / interface to invoke Service-specific logic.
The advantage I see of this approach is that our (exposed) API wouldn't change by a lot; additionally, it should reduce the amount of code we need to modify overall since Run() seems to be dominated by generic code which can mostly stay the way it is right now.

I'm slightly leaning towards the second approach, though IMHO it often takes an attempt to implement to judge for sure. I'd appreciate if you could try things out and report back on how you feel about it. (Feel also free to push to your branch to review potentially even very unfinished solutions.)

Existing methods for the corresponding services (living in controller.go and node.go / mount.go, respectively) would then also be moved to one of the structs.

The methods in these files are mostly receivers to the Driver struct. Along with the split into two structs we are aiming for, these methods should also be moved accordingly. That is, methods in controller.go should mostly be homed with the new Controller struct, and likewise the latter should become receivers to the new Node struct. This essentially eases reasoning about why a particular field should exist on one struct or the other, which is the higher goal of the issue in question.

Let me know if these answers are helpful.

[dhij]

@timoreimann The second approach seems like a good idea, and the answers helped a lot thank you! I will try things out and get back to you!

[dhij]

👋 @timoreimann I just pushed a commit where we will be using the same Driver struct but with a field holding an instance of the corresponding interface (Controller/Node struct). What I have now are:

• Driver struct has a csi.NodeServer and csi.ControllerServer interface fields
• The NewDriver() holds an instance of the interface accordingly
• The Run() invokes the service-specific logic

I noticed some fields such as the publishInfoVolumeName, region, and log need to be duplicated across both the Controller and Node structs since some of their method implementations called those fields internally.

I would appreciate any feedback 🙂