MVP Agent Implementation

[iamjoemccormick]

Blocks:

• MVP Agent Implementation beegfs-go#177

This PR makes two changes to the protos:

(1) Add Remote and Sync as BeeGFS node types.

(2) Add a new BeeAgent service and related messages that allow configuring BeeGFS resources (e.g., NICs, targets) and executing services on a particular machine.

• Note: The goal of this PR is to get early feedback as we work towards a minimum viable product. Thus I would treat these as a "good first draft" and we will make changes as needed with no backwards compatibility guarantees until an official release.

Tagging @rustybee42 to ensure this changes aligns with the standards we've defined for protobufs. Particularly around where I choose to use or not use optional. In general I omitted the optional keyword if an empty string was not valid input for a particular field and would need to be checked anyway, or if an empty string was the best default for a particular field.

Tagging @pranavpadmasali as he'll be the primary reviewer on the other PR and may have related feedback.

[rustybee42]

issue: enum variants should be prefixed with the enum name, e.g. INSTALL_TYPE_UNKNOWN. If we ever define another enum with a variant of the same name here, it will clash in certain languages (C++).

This should also most likely be called INSTALL_TYPE_UNSPECIFIED to follow the convention. If UNKNOWN has actually a meaning, it should be added as an extra variant.

[rustybee42]

issue: same as the other enum, but less important as it's scoped by the message.

But still, if we ever add another enum definition to this message, we might have the same problem.

[rustybee42]

note: I checked the management, adding more types here seems to be fine (needs some patching when updating the dependency, but that is to be expected). I also think that remote and sync are valid node types to add here, even if the management currently doesn't know about or handle them in any way.

[rustybee42]

nitpick: We should name this like the file it is defined in. I also think that we shouldn't add Bee to names if not absolutely necessary (e.g. when names belong to some uncontrolled, global namespace like environment variables)

[rustybee42]

issue: "Service" should be called "Node" throughout the definitions to go along with the common terminology used throughout the code (at least the newer additions). Unless I missed something fundamental here.

Note that this is only about the code and APIs, not the user facing names. It might still be called "service" there (the terminology is not very clear there anyway). Although I think we would benefit from using only one term, at least in new stuff.

[rustybee42]

nitpick: The wording "will be created" implies that each and every consumer of this acts like. But this is just an API skeleton which might have multiple users and we don't have control over what they do.

So this should be worded "should be created" or similar. Or more generally, the description should mainly be about what data the field is supposed to carry, what it means, under what conditions it can or must be set and what it is supposed to be used for (but not phrasing this as a matter of fact).

[rustybee42]

issue: Fields must have a description on their meaning and under which condition they can/must/should be set.

I put this comment here because this is marked as optional, most likely to indicate that this field is optional from a users perspective (as a non-primitive it is explicitly set anyway). I would strongly advise against this. optional is a modifier to set the field to "explicitly set" and should have no business with application logic, even if the name kind of suggests it (bad choice in my view, and that's probably why it gets removed in later protobuf editions).

My main reason here is that in the opposite case, not having optional doesn't necessarily mean that value is not optional (e.g. in case of using empty strings for meaning "unset"). It always depends on the applications that use it. Also there might cases where a field is sometimes optional and sometimes not, depending on the rest of the message or application states.

Instead, the field must be commented and the comment must state these conditions. Like it is done in management.proto and also on some fields below in Target.

Note that this is not about the "everything explicitly set" or not discussion. To make the usage conditions of a field clear, it MUST be clearly documented in text form - then it is actually not that important if it is explicitly set or not and it's ok to do it only if needed.