Fake yaml devicegraph

[shundhammer]

https://trello.com/c/P6cGt6Zr/292-5-libstorage-load-mockup-hardware-tree-from-yaml-files

• Read fake storage hardware description from YaML file
• Abstract and concrete factory for fake device graphs
• New separate examples/ directory for simplistic use cases

[wfeldt]

looks ok, at least to me

[ancorgs]

"able to create and some methods" something is missing or surplus in that sentence

[wfeldt]

"are requited to implement .create_xz ... and some methods ..."

Looks perfectly fine a sentence to me.

[shundhammer]

Actually, no:

"Subclasses are required to implement a create_xy() method for each factory product 'xy' the factory is able to create and some methods to support some basic sanity checks of the generated device tree"

Subclasses are required to implement

• a create_xy() method for each factory product 'xy' the factory is able to create

and

• some methods to support some basic sanity checks

[ancorgs]

I see. You are right, it was just a matter of reading it twice 😉

[ancorgs]

I dislike this approach for the same reasons I disliked the original code of the proposal with that PROPOSAL_BASE and PROPOSAL constants.

In my opinion, a FooFactory should generate by its own means objects of type Foo.

With this approach, we generate a Devicegraph object and we store it "somewhere" (in the drawer with the label FAKE) and the we pass that object to a ad-hoc "factory" that is used to tweak the object.

To be honest, I find the all FakeProbing class a little but cumbersome.

I would have expected the code on this file to look more functional. In Ruby, methods with input/output parameters or that simply leave the result elsewhere else instead of returning it feel wrong... and are usually hard to test. This looks way more ruby to me.

#!/usr/bin/env ruby

$LOAD_PATH.unshift(File.expand_path('../../lib', __FILE__))

require "storage/fake_devicegraph_factory"
require "storage/debug_devicegraph"
using Yast::Storage::DebugDevicegraph

FILENAME = "fake-devicegraphs"
factory = Yast::Storage::FakeDevicegraphFactory.new
devicegraph = factory.load_yaml_file("#{FILENAME}.yml")

storage = StorageManager::create_fake_instance
storage.probed = devicegraph

storage.probed.dump_disks
storage.probed.write_graphviz("#{FILENAME}.gv")
system("dot -Tpng <#{FILENAME}.gv >#{FILENAME}.png")
system("display #{FILENAME}.png")

# Clean up
File.delete("#{FILENAME}.gv")
File.delete("#{FILENAME}.png")

Apart from being "more Ruby", I think it's more obvious.

[shundhammer]

I disagree.

Why would we want to pollute the StorageManager with something that absolutely does not belong there? It provides infrastructure for using the new libstorage. It is a fringe case that anybody might need or want to use a different Storage::Environment for different purposes, one of which is faking those device trees. But it's not limited to that, it's very special, and it's pretty far away from its normal usage. That's why there is this special class FakeProbing. It uses the StorageManager for its special setup, but it does not extend the very visible public API of the StorageManager. This is on purpose.

This is also why the FakeDeviceFactory gets the device graph as an input parameter rather than returning a newly created one. FakeProbing sets up the special case, FakeDeviceFactory uses it - it creates fake devices on the device graph it gets. It is a factory to create fake devices, not fake devicegraphs.

[shundhammer]

Also, it is technically not possible to create an anonymous device graph first and then start the FakeProbing mechanics: FakeProbing needs to be first to prevent libstorage-bgl from doing any probing when its first instance is created. That's basically the main thing FakeProbing does. Copying an arbitrary device graph to the "probed" device graph is just secondary functionality; the primary one is to set up the StorageManager / libstorage-bgl so that it doesn't do any real probing to begin with (which would require root permissions for one thing, and, more importantly, would pollute the "probed" device graph with the real devices from the host machine - which is not desired here).

[ancorgs]

Running this in my system doesn't seem to trigger the hardware probing, although I could be wrong (or I could have misunderstood you):

require "storage"
graph = Storage::Devicegraph.new
Storage::Disk.create(graph, "/dev/sdaX")
puts graph.to_s

[ancorgs]

If the main functionality of FakeProbingis creating the initial instance of libstorage to avoid the real probing and its secondary functionality is to set StorageManager.probed to a fake one. I think a code like this is more obvious about those intentions:

using FakedStorageManager # If you don't want to "pollute" StoreManager itself

StorageManager::create_fake_instance
StorageManager.instance.probed = a_method_returning_the_devicegraph

Than this other one

fake_probing = FakeProbing.new # This initializes StorageManager.instance as a non-obvious side effect
a_method_populating_a_given_devicegraph(fake_probing.devicegraph)
fake_probing.to_probed # A message sent to fake_probing that causes a change in the state of StorageManager.instance

Second proposal is sending messages to A and expecting the status of B to change. First proposal sends message directly to B, even if it implies extending its API (something that can be done cleanly only in the test code with a refinement).

Another side note (not 100% relevant to the current discussion), having dump_disks in FakeProbing is something that doesn't fit with the main or secondary responsibility (avoiding the probe and setting probed).

[ancorgs]

Anyway, let's merge this and move forward for the time being. The code works and we need to have all the code merged if we plan to move it to a different repo.

After the sprint is finished, we will decide if this kind of discussions about API belong to code review or not. https://trello.com/c/lTZScC5Z/145-how-to-solve-code-review-disagreements