[sled agent] Fakes are better than mocks; get rid of mocks

[smklein]

Sled agent uses a lot of mocks to represent interactions with underlying resources. It uses a library named mockall to help with this interaction. We should use fakes instead.

• svc interface (sled-agent/src/illumos/svc.rs)
• dladm interface (sled-agent/src/illumos/dladm.rs)
• fstyp interface (sled-agent/src/illumos/fstyp.rs)
• zpool interface (sled-agent/src/illumos/zpool.rs)
• zfs interface (sled-agent/src/illumos/zfs.rs)
• some free functions (sled-agent/src/illumos/mod.rs)
• instances (sled-agent/src/instance.rs)

Background

When the sled agent is provisioning a zone, it "mocks" access to zones. This enables unit tests to verify "hey, you would have created a zone here", but in reality, make no such call.

This is okay-ish for some types of unit tests, where we have a module foo which depends on a interface resource:

• foo can be conditionally compiled with cfg(test) to use the mockResource
• foo's tests can then use mockall's API to set expectations about calls to the mockResource

This is one such example of a test written in this style.

Problems

This testing strategy really breaks down when it gets nested. Suppose we have a module baz, which depends on foo and bar.

• foo's dependency on mockResource still needs to be captured, so baz also needs to set up "expectation" calls in tests
• The same applies to bar, and any other modules which contains mocks
• This basically leaks implementation details, and makes tests really difficult to write

This is apparent for modules like the storage_manager, where we're interfacing with:

• ZFS provisioning
• Zpool provisioning
• Zone provisioning

and all of them would need to be mocked to actually write tests.

Proposal

We should implement fakes wherever these mocks are being used. This will give us a more flexible interface for testing, and make dependencies on "global-ish resources" much more apparent.

Admittedly, doing so will require providing the interfaces to these modules as up-front objects. I propose doing so with Arc-bound trait objects, rather than generics, to keep things relatively simple. The cost of a single Box + Arc's refcounting should be trivial compared with the cost of "provisioning a filesystem" or "managing a zone".

[davepacheco]

Sorry for the ignorant question but what do we mean by "fakes"? I googled it and found https://stackoverflow.com/questions/346372/whats-the-difference-between-faking-mocking-and-stubbing ...which has several different definitions.

[smklein]

My intention was "a struct that lies about implementing the functionality, often by just storing a HashMap of provided arguments".

According to that stackoverflow post, I'd say it's closest to:

Fake objects actually have working implementations, but usually take some shortcut which makes them not suitable for production

For example:

• We could define the access to Zpools as a trait, ZpoolInterface. Maybe this contains methods like "get zpool" and "provision zpool".
• We can implement the real access to zpools as ZpoolAccess, which implements ZpoolInterface. It actually makes calls, either through a native library, or through the CLI, to provision real zpools.
• We can also implement fake access to zpools as FakeZpoolAccess. This just contains a HashMap<Name of Zpool, Metadata about Zpool> which we maintain in memory. "provision zpool" adds an entry to the map, "get zpool" queries the hashmap.
• Any clients that depend on interfacing with zpools can act on a dyn ZpoolInterface, to be compatible with either variant.

[smklein]

Closing largely in favor of the option proposed in #5226