Restructuring of the API

[sdwilsh]

Putting this up to get thoughts.

As I'm thinking about how this might be used outside of sdwilsh/hass-freenas, I'm thinking changing the API up a bit.

Today

from pyfreenas import Machine as FreeNASMachine

machine = await Machine.create(
    "hostname.of.machine",
    username="someuser",
    password="password",
)
await machine.refresh()
disks = machine.disks
vms = machine.vms

One con I see with this approach is that if a user doesn't care about disks and only vms, they have to fetch this data all the time.

Proposed

from pyfreenas import Machine as FreeNASMachine

machine = await Machine.create(
    "hostname.of.machine",
    username="someuser",
    password="password",
)

disks = await machine.refresh_disks()
vms = await machine.refresh_vms()

# Still available, as state is still cached
disks = machine.disks
vms = machine.vms

This addresses the con above, and any call site that wants to setup a job that pools every so often to update the state can still trivially do so.

[sdwilsh]

@tobiasbp, looking to hear your thoughts on this. I'm not tied to the refresh_ prefix for these, so also open to suggestions there.

[tobiasbp]

Fundamentally, I'm confused about accessing the API using websockets vs. RESTful: What is possible using websockets, that is not possible using RESTful and vice versa? The use cases relevant to me (Reading current server state, and possibly adding or removing things like share and datasets), do not seem to benefit from using websockets. However, I can imagine that some would (Any examples?). Something that would benefit from the persistent connections? I think the FreeNAS GUI uses the API via websockets, so maybe if you were building a FreeNAS dashboard, websockets would be preffered. On the other hand, I'm not aware of any downsides to using websockets?

We were talking about supporting both methods. If we did, would that influence the syntax of the wrapper?

Now, my comments on your proposal:

One con I see with this approach is that if a user doesn't care about disks and only vms, they have to fetch this data all the time.

I much prefer to not have the user forced to read everything from the server. In most uses cases, that would pull down a lot of unneeded data. Consider a scenario were the whole API was supported by the wrapper!

Proposed

from pyfreenas import Machine as FreeNASMachine

machine = await Machine.create(
    "hostname.of.machine",
    username="someuser",
    password="password",
)

disks = await machine.refresh_disks()
vms = await machine.refresh_vms()

# Still available, as state is still cached
disks = machine.disks
vms = machine.vms

This addresses the con above, and any call site that wants to setup a job that pools every so often to update the state can still trivially do so.

It would also be nice to be able to do machine.refresh(), but then you would need to know what resources of the machine you are interested in. Maybe you could pass something to the constructor?

r = ['disks','vms']

machine = await Machine.create(
    "hostname.of.machine",
    username="someuser",
    password="password",
    resources=r,
)

# Refresh everything
await machine.refresh()

# Refresh disks only
await machine.disks.refesh()

Maybe the resource 'truenas' would always be loaded. What do you think? (UPDATE: I see you already load system.info).

UPDATE: Come to think of it, why not just add disks to the internal list of resources once the user wants to access disk data? Then the constructor does not need to change.

Also, I was thinking about the cache: Why do we need it? Why not always load fresh data from the FreeNAS-system?

[sdwilsh]

What is possible using websockets, that is not possible using RESTful and vice versa?

The big thing that websockets has over the REST API is that the server can publish events back to the client. While the code today is doing simple calls, my plan was to use the events to then keep information up-to-date on things like disks and vms without having to ask the server to update (apart from the initial call). My primary goal of this project was to integrate FreeNAS/TrueNAS into Home Assistant, and that wants as real-time of information as it can get.

I think that use-case answers the bulk of your questions, and I've put some additional thought into this. Let me adjust my proposal based on the information and questions you've provided now, and hopefully we can both agree on how to move forward :)

Ideally I would have coded this up in a pull request, but I don't have time for that tonight, so I'll make this python code as good as I can on the fly showing how this might be structured.

Proposal

Just using disk for now as an example, but this would be extended to other concepts as well.

New machine.py:

from abc import abstractmethod, ABC

class MachineABC(ABC):
    @abstractmethod
    @staticmethod
    async def create():
        """Create and validate authentication with the concrete implementation of this class."""

    @abstractmethod
    async def get_disks() -> List[Disk]:
        """Get the disks on the remote machine.""""

disk.py:

from abc import abstractmethod, ABC

class Disk(ABC):
    """Base class for disk implementations for the Caching and Non-Caching clients"""
    @abstractmethod
    @property
    def description(self) -> str:

    @abstractmethod
    @property
    def model(self) -> str:

    @abstractmethod
    @property
    def name(self) -> str:

    @abstractmethod
    @property
    def serial(self) -> str:

    @abstractmethod
    @property
    def description(self) -> str:

    @abstractmethod
    @property
    def size(self) -> int:

    @abstractmethod
    @property
    def temperature(self) -> int:

    @abstractmethod
    @property
    def type(self) -> DiskType:

    def __eq__(self, other):
        if not isinstance(other, self.__class__):
            return NotImplemented
        return self.serial.__eq__(other.serial)

    def __hash__(self):
        return self.serial.__hash__()

New websockets/machine.py:

class CachingMachine(MachineABC):
    """A Machine implementation that connects over websockets and keeps the information it fetches up-to-date real-time."""
    @staticmethod
    async def create():
        ...

New websockets/disk.py would largely copy the current disk code, but with some minor changes. Namely, it would inherit from the ABC Disk class, and include the additional available method as well.

New rest/disk.py would look a bit different, as it would need all of its properties at time on construction, but you could just pass the API response to it to accomplish that, realistically.

I think with this change, both of our use cases are covered with sensible APIs, but there is still a common interface between them.

[tobiasbp]

Sounds like a great plan. Let's do it.

As for the always updated version using websockets: Would the user then need to subcribe (In some way) to the Machine to get the full benefit of using webockets, or would she pass some mechanism/handler to the Machine ?

[sdwilsh]

My intention is that if it's the caching version, it does it all automatically. I would probably avoid subscribing until they first call get_disks the first time, and then once we've gotten them once, we know to subscribe.

[tobiasbp]

I mean, when someone uses the wrapper (Websockets version) to build something, would they constantly look at the data in the Machine to look for changes (I know the Machine subscribes, and is automatically updated on changes on FreeNAS), or are you planning on implementing something "fancy"?. Non-"fancy" would be the user polling py-freenas without generating traffic on the network since py-freenas is always updated through subscriptions.

Let's say I was building a GUI: Maybe I would like for py-freenas to run some function of mine when FreeNAS reports changes, instead of me looking for changes in the Machine and then running my function. In that case I would like to pass a handler to py-freenas when something changes. That is, do you envision that the user would able to "subscribe" (In non-websocket-lingo) to changes in the py-freenas Machine?

[sdwilsh]

I need to have some degree of subscription support when things are added at least. Haven't thought much more beyond that (and the available property already on Disk and VirturalMachine, to be honest.

[sdwilsh]

@tobiasbp, I tossed up #11 which does some of the work. Wanted to get your thoughts, now that there is real code, before I go any further with it.