stratis-storage / stratisd Goto Github PK

Right now some have no license header, some have Mozilla something, no license file in directory, etc.

And export constants and so forth as appropriate.

I think we should do this sooner rather than later.

I think that the contents of dbus_api.rs could be split into multiple files, for manager, for pool, and so on and that in this way any problems with circular dependencies can be avoided.

Once the snapshot is created, it is a volume like any other. It should be destroyed using the method for removing any volume, DestroyVolumes.

BusException: org.freedesktop.DBus.Error.NotSupported: Unable to autolaunch a dbus-daemon without a $DISPLAY for X11

Or we should fix instances where, e.g., CreatePool() returns an object path even though it wasn't able to create the pool because the pool was already there.

But I like the first approach the best.

Does it give you the items in reverse order, just like get_items()?

See https://bugzilla.redhat.com/show_bug.cgi?id=817854

Devicemapper's mechanism for receiving events currently requires a thread to sleep in an ioctl call for each device in question. I'm working with the LVM team to add a new mechanism that will achieve this goal in a non-blocking, single-threaded fashion. Alasdair and I have a preliminary agreement on how to proceed, so I need to:

1. Write a doc describing the proposed solution and its implementation details
2. Get initial internal signoff from lvm-team
3. Write the code
4. Submit upstream, get it merged
5. Wait until a kernel is released that includes the new feature
6. Implement support in devicemapper-rs
7. Implement support in stratisd event loop

Until this happens, stratisd may need to poll -- read status from dm devices periodically to make sure everything is fine.

That is, either the engine has its own return codes and the dbus API changes them to its visible return codes, or else the engine serves up the return codes.

It doesn't seem to need the extra functionality of PathBuf.

It needs to be in main(), and then if the dbus socket becomes readable, then we need to call our dbus code to handle the incoming request.

This is because we will also be expecting non-DBus events, such as timers, netlink events, and who knows what else.

I'm going to set a not-very close milestone for this because I do know we're only worried about DBus near-term, but this will need to happen at some point.

This is something we should figure out how to do generically.

Probably the way it is done in the CLI is a good way, i.e.,

• Parse the signature.
• Generate functions from the signature that build the desired value from the corresponding value in the other types. in args need to be broken down from MessageItems to their inner types, outargs need to be built up.

I solved this problem with decorators in the CLI. This is nice, because the majority of the cost, that is, the generation of the functions, gets paid when the module is loaded. Only the cost of executing the functions gets paid at runtime. How to pull of the same thing in Rust is not so obvious. In the CLI, it is not necessary to unpack the in arguments, since the types of the in arguments are subtypes of the desired arguments due to the implementation of the Python library.

There was the idea of a 1 to many relationship broached at one point. Could we do something smart for this relationship. And, what if the user wants to specify a whole lot of snapshots? Should we actually force them to specify the names of all the snapshots? Do they not care about the names and would they like to specify snapshots in differrent ways?

Currently, a cache is created automatically when a pool is created.

This is tricky, because it is not clear what devices should belong to the cache.

Currenlty a cache has only properties, not methods. One of a cache's properties is its Dev. In the instance I'm looking at, I created a pool consisting of a single dev. The cache's unique dev is thus the pool's unique dev. This seems like it must be wrong.

If the cache is automatically created when the pool is created, initially, the cache will have no devices, or else it must get devices from the devices in the pool. It is not clear to me how stratis will always choose the correct devices for the cache, so the second option seems bad. This leaves the first option, with a cache with no devices. Given this possibility, stratis will have to be able to deal with the possibility of an empty cache or a cache that is way too small, such that there is no benefit.

The alternative is to make the user do the work, creating the cache and specifying the devices for the cache explicitly. This feels more desirable to me than auto-creating the cache. Since stratis presents a real API a smart client of stratis could encapsulate the creation of a pool and its cache into s single action in a reasonable way, i.e., stratis doesn't have to do so itself. This is the way I would prefer.

We should decide whether we should be using this or not for error message. Should we be restricting its use to a certain kind of error message, maybe non-Stratis ones?

Even if I'm in Stratis CLI where I'm the administrator, but also here.

Initial implementation should allow pools, filesystems, and blockdevs to be added and removed. This includes:

• Validating method arguments are present and typechecked
• Calling the appropriate Engine or Pool method
• Converting the response, either success or error, to the method's defined return values

SimEngine should:

• Fully support methods in Engine and Pool traits
• Support basic adding/removing of simulated pools, filesystems, and blockdevs, to the extent possible via the DBus API
• Optionally support returning random error results instead of always succeeding

I introduced a bug in dbus_api.create_pool() while trying to handle the possibility that the items in the array of device specs might have the incorrect type. Here's the code:

    let mut devstrings = devs.iter().map(|x| x.inner::<&String>());
    if devstrings.any(|x| x.is_err()) {
        return Err(MethodErr::invalid_arg(&item1));
    }
    let blockdevs = devstrings.map(|x| Path::new(x.unwrap())).collect::<Vec<&Path>>();

The problem is that devstrings, which is a Map, is mutable. If I check if any of the results matches Err, I consume the iterator. So, if all the inhabitants of the array have the correct type, there is a bug, because blockdevs is always the empty vec. In other words, the code has to hit the return statement, or the list of devices has to have been empty originally, for this code to be correct.

I've thought of a lot of ways to solve this problem, involving use of various iterators. But it is a very generic problem, it will recur, I'ld prefer to adhere to the accepted solution if there is one.

A description of the problem:

1. For each element in the vector, perform an operation that may fail, and which produces a new element. This is a basic map operation, except for the failure part.
2. If there is a failure for any element, exit from the method with an Err value.
3. Otherwise get a new iterator which contains the elements on which the operation is performed.
4. Do something with that new iterator.

We will need rust available to do any CI for Stratisd, so we need to investigate whether any standard services makes Rust available. There is some hope for TravisCI.

Previously the engine returned the object path. It looks to me like that now the dbus API has the job of synthesizing the object path. But, in that case, the engine's createpool method must still return something, and it is currently defined to return nothing. Should it be returning a pool object? Should it be returning a pool name? DBus API must maintain a mapping between object paths and some identifier for pools, so this is an important question.

The idea is that there are some destructive actions, which the user would be grateful to be warned about? This is probably easily done by wrapping "action" methods, like:

def prompt_wrapper(the_func):
    if the_func in DESTRUCTIVE_ACTIONS and SAFE_MODE:
        def new_func(namespace):
            # do the prompt, then
            the_func()
    else:
        new_func = the_func

    return the_func

Or the wrapper could be made more specific to individual action methods in a number of ways.

There's no DBus in the initramfs, so if stratisd needs to be in there and do something, it needs to cope with its absence.

This means that the default value returned on error needs to be changed to a valid object path, probably "/".

The dbus-rs library does not ensure this.

There is a model of doing this in lvm-dubstep that is in lvm tree.

We can verify this in Stratis CLI independently, I think.

https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names-interface

They're supposed to be namespaced by reverse domain name, so we can't use org.storage (which is perfect but we'd need to buy it 😄 ). I'd recommend org.freedesktop over com.redhat -- it's less "corporate", and is also used by a great many other interfaces.

Like MessageItem::Uint16(StratisErrorEnum::STRATIS_OK as u16).

See #90.

Currently, there is just no reply:

[mulhern@dhcp-25-209 stratis-cli]$ PYTHONPATH=./src ./bin/stratis logical create deadpool oubliette
Namespace(func=<function create_volumes at 0x7f10beeae410>, pool='deadpool', subparser_name='create', volume=['oubliette'])


[mulhern@dhcp-25-209 stratis-cli]$ PYTHONPATH=./src ./bin/stratis logical create deadpool oubliette
Namespace(func=<function create_volumes at 0x7fcb09f2d410>, pool='deadpool', subparser_name='create', volume=['oubliette'])


Traceback (most recent call last):
  File "./bin/stratis", line 39, in <module>
    main()
  File "./bin/stratis", line 36, in main
    return next(execution)
  File "/home/mulhern/my-projects/stratis-cli/src/stratis_cli/_main.py", line 41, in run
    raise err
dbus.exceptions.DBusException: org.freedesktop.DBus.Error.NoReply: Message recipient disconnected from message bus without replying

This is due to a seg fault, rather than normal execution. It fails to return a message because it crashes.

It is possible for d-bus and Stratis to get out of sync. We should think thought a strategy for dealing with d-bus failures.

CreateVolume() would require --force to create a new volume of the same name when the old volume has some data on it.
AddDevs() should have a --force option just as CreatePool() has a --force option.

We need a README.md so that potential developers visiting the repo on GitHub will get information on what Stratis is supposed to do and how to contribute code or test it.

Should this necessitate the notion of a primary and secondary filesystem? secondary is RO, but can be promoted to primary, i.e., a restore from a backup.

see https://docs.rs/enum_derive/*/enum_derive/index.html

While I imagine this crate is doing things comparable to what we are doing in dbus_consts.rs, we could get it out of sight and eliminate repetition in our own code by using this.

It seems to handle iteration and stringification, which would let us avoid having to implement iterator() and Display, although get_error_string would still be needed.

Should it be allowed to specify devs that already belong to the pool to which the cache belongs.

Or, would one have to remove the dev from the pool in order to add it to the cache?

Right now a DBus client can get errors in two ways from calling our API.

First, if the call was malformed, for a nonexistent object, or didn't have enough parameters, the client will get a standard DBus error. Second, if the call was good but Stratisd internally had a problem, the result is a successful DBus call, but with an error type in the result other than STRATIS_OK.

I propose we make Stratisd errors actually be returned as DBus errors. We can define own own error names that correspond our existing values in dbus_consts.rs StratisErrorEnum.

This will let clients still differentiate between error types if desired, or if they just want to know success/fail, they can determine this without doing a secondary check for more errors once they know the method call returned success.

thoughts @trgill @mulkieran @tasleson ?

This may be a casting error on d-feet's part, or dbus-python's, not a stratisd error.

It may not really matter very much to the underlying implementation. But making a snapshot will definitely affect the pool, in at least as many ways as making a volume.

blockdev::BlockDevs is actually going to want to write to block devices. We're going to be adapting it for use it in StratisEngine. Does SimEngine really want to need & access actual blockdevs, or should it "sim" them?

Otherwise, we waste the time of the translators, among other things.

There are but four now.

https://github.com/Manishearth/rust-clippy

Clippy is a Rust lint that is pretty good at catching style issues or non-idiomatic code. Unfortunately it requires Rust nightly to run as a compiler plugin, but it can also be used as a Cargo subcommand. However, I found some of its lints were a little too picky, but these can be disabled on a per-lint basis.

https://github.com/stratis-storage/stratisd/blob/master/src/dbus_consts.rs#L159

A mirror across four disks is not the same as RAID10, which is a stripe of two pairs of mirrors, so we should decide and clarify exactly what we mean.

At least, we need to separate the external view by clients of the dbus API from the internal view of the engine. Right now StratisResult is for both.

Probably, it would make sense to make a new return type for the engine, something like EngineResult, which will have its own internal error type as well.

An example is creating a list of volumes. Currently, the simulator creates the volumes it can, and if it fails to create any volume, signals a STRATIS_LIST_FAILURE meaning that it can not be more specific than that, because it may have many failures, with many causes. It is possible to look through the results for the individual volumes to find out details.

Should it roll back, returning the reasons for its failure as before, but not actually creating any volumes?