Changes to DBus and engine APIs for idempotence #1614

jbaublitz · 2019-08-21T13:34:34Z

This PR is a continuation of #1597 which was closed as Github was not updating the branch and kicking off CI for new commits.

The current state of this PR is:

non-rust tests still need to be fixed
Engine API is idempotent and returns changed resources or set of changed resources, filtering out no-ops in the return value
DBus API uses object paths for identifying resources that exist in DBus and resolves them to UUIDs or names for the engine calls - it is also idempotent and follows the same convention of returning only changed resources
There is a new set of types and a trait EngineActions to provide the return types of the idempotent engine API

jbaublitz · 2019-08-21T17:27:24Z

@mulkieran All tests are good to go.

jbaublitz · 2019-08-22T15:13:09Z

@mulkieran I know I said this last time, but I think changes are finally good to review and merge once I've made any edits you request. Just pushed a commit cleaning up the rest.

mulkieran

I'm still working through this. Here is a very minor formatting comment, though.

src/dbus_api/api.rs

mulkieran

Rename filesystem is a bit inconsistent right now, see points...

src/dbus_api/filesystem.rs

jbaublitz

All requested changes have been made.

jbaublitz · 2019-08-26T16:05:19Z

Restarting the build that failed due to a Travis error.

jbaublitz · 2019-08-26T16:13:04Z

@bgurney-rh Hm there's a reproducible failure on the Travis side with apt-get as displayed here. I'm not entirely sure how to work around it, but I've retried the build a few times and this error message is consistent. Any ideas?

mulkieran · 2019-08-26T16:33:01Z

We have the occasional error (not failure) on the Travis cross-compilation target. Usually it's Java-related(!), but this error is a new one on me. Maybe it will simply go away shortly, as it seems like an error in Travis resources.

mulkieran

Just a few more, as I'm working my way through it.

src/dbus_api/filesystem.rs

src/dbus_api/pool.rs

drckeefe · 2019-08-26T17:09:05Z

@bgurney-rh Hm there's a reproducible failure on the Travis side with apt-get as displayed here. I'm not entirely sure how to work around it, but I've retried the build a few times and this error message is consistent. Any ideas?

From looking around this issue can come up if apt-get is looking at the update repo for information on the packages, but installing local copies of the packages. If they are not in sync then the size of the old local package

@bgurney-rh Hm there's a reproducible failure on the Travis side with apt-get as displayed here. I'm not entirely sure how to work around it, but I've retried the build a few times and this error message is consistent. Any ideas?

Looking around it seems this issue happens when the apt-get update repo reflects a different package size then the local copy of the package being installed. Solutions seems to be either install the local copy of the package that apt-get is throwing this error for or use the No-Cache=True option for apt-get example "sudo apt -o Acquire::https::No-Cache=True update"

mulkieran · 2019-08-26T19:35:09Z

~~We'll make the cross-compilation Travis task an allowed failure for now.~~ It has started working again on its own.

mulkieran

Still working...here's a few minor stylistic requests.

src/dbus_api/pool.rs

src/engine/strat_engine/backstore/metadata/bda.rs

jbaublitz

I resolved all of the requested changes and have also removed all references and imports for Uuid in src/dbus_api/pool.rs and have replaced them with the type declarations from the engine.

jbaublitz · 2019-09-11T19:40:04Z

Rebased

mulkieran

I did an end-to-end from the CLI down through the changes in the engine for creating filesystems and it all looks good. Next comes destroying filesystems. I have just one renaming request to make an easier read.

src/dbus_api/pool.rs

mulkieran

Just checked off destroy filesystems. A few further requests.

src/dbus_api/pool.rs

src/engine/strat_engine/pool.rs

mulkieran · 2019-10-02T13:27:58Z

@jbaublitz Please rebase, just to get rid of the Travis failure.

jbaublitz · 2019-10-02T14:16:38Z

@mulkieran The PR is already fully up to date. I'm going to restart the failed job, and if it fails again, I think I'll need to do further investigation.

jbaublitz · 2019-10-02T14:25:04Z

It turns out this was my fault. I added an unnecesary list comprehension and pylint seems to have just started picking that up as we saw in some of the other PRs. This is now resolved and pushed.

mulkieran · 2019-10-04T12:29:52Z

@jbaublitz Please take a look at #1650 edits. One is minor and obvious. The other is where I discovered that there was on additional place where we should enforce idempotency. It turns out that that fits easily into the rename category. Please take a close look at it. I took a look at the CLI and find that we don't actually use that part of the D-Bus API anywhere so I filed an issue: stratis-storage/stratis-cli#922; it's outside the scope of this PR. I've also put the D-Bus doc changes up for review: stratis-storage/stratis-docs#129.

I expect I'll need one more pass over the PR after this one, but we're definitely on the downhill now.

jbaublitz · 2019-10-07T15:39:45Z

I've reviewed the changes and incorporated them into this PR. This all looks good to me.

mulkieran · 2019-10-08T13:07:26Z

Please take a look at: #1650.

jbaublitz · 2019-10-08T14:24:58Z

@mulkieran All of the requested changes make sense to me other than the removal of is_changed() which I'm not against but would like to discuss a bit more first.

.changed_ref() is lingering from one point in time where I needed to perform .changed() and .unchanged() on the same test result, but now that .unchanged() has been removed, this seems like a reasonable thing to get rid of as well until we need it again down the road.

With .is_changed(), I'm a little less clear on why we'd do this. Given that this is both used (even if just in tests at the moment) and follows a common Rust idiom of providing a method that essentially simulates pattern matching like .is_ok() and .is_err() in the Result type when the wrapped data is unimportant, I'm more hesitant with the removal of this one. Furthermore, if we remove .changed_ref(), all reference-based destructuring will have to be done through pattern matching. If this pattern weren't idiomatic, I'd have no qualms removing .is_changed() but this seems to be a common way to simplify code in the case in which I'm using it here. Do you have thoughts on that? The argument I can see for removing it is that currently we always want the contained data outside of the context of testing functions and I am not sure if we'll ever want to omit those from DBus responses. If this seems like it's not worth adding to the trait for testing purposes, I can see the value in that argument.

mulkieran · 2019-10-08T15:17:50Z

@jbaublitz You're correct that is_* is a common usage in, e.g., the Result or Option type. I don't mind if you leave is_changed in at this time, but make sure to document it if you do.

jbaublitz · 2019-10-08T21:45:51Z

I've updated this PR with all other requested changes and kept .is_changed(). Anything else I should look at before I rebase and submit for final review?

mulkieran

Ok! Please squash and make a final version.

Make internal changes to the engine to better support idempotent operations using the DBus API so that the return value of DBus methods provides information on what was changed and omits resources that were not changed. Co-Authored-By: mulhern <[email protected]>

jbaublitz · 2019-10-09T13:51:59Z

Squashed and ready for merge.

mulkieran · 2019-10-09T13:54:11Z

Squashed and ready for merge.

Whoops! I spoke loosely. It should have been something more like "...make a final version, squashed".

jbaublitz · 2019-10-09T14:00:18Z

Ok, I'll close this out.

jbaublitz added the do not merge label Aug 21, 2019

jbaublitz requested a review from mulkieran August 21, 2019 13:34

jbaublitz self-assigned this Aug 21, 2019

jbaublitz force-pushed the issue-project-51 branch from 2069422 to 3431426 Compare August 21, 2019 15:52

mulkieran requested changes Aug 26, 2019

View reviewed changes

src/dbus_api/api.rs Outdated Show resolved Hide resolved

mulkieran requested changes Aug 26, 2019

View reviewed changes

src/dbus_api/filesystem.rs Outdated Show resolved Hide resolved

src/dbus_api/filesystem.rs Outdated Show resolved Hide resolved

src/dbus_api/filesystem.rs Show resolved Hide resolved

jbaublitz commented Aug 26, 2019

View reviewed changes

jbaublitz force-pushed the issue-project-51 branch from 33d64e3 to efdd131 Compare August 26, 2019 15:04

mulkieran requested changes Aug 26, 2019

View reviewed changes

src/dbus_api/filesystem.rs Outdated Show resolved Hide resolved

src/dbus_api/pool.rs Outdated Show resolved Hide resolved

mulkieran requested changes Aug 27, 2019

View reviewed changes

src/dbus_api/pool.rs Outdated Show resolved Hide resolved

src/dbus_api/pool.rs Outdated Show resolved Hide resolved

src/engine/strat_engine/backstore/metadata/bda.rs Outdated Show resolved Hide resolved

jbaublitz force-pushed the issue-project-51 branch from 189e0bf to 9db5c99 Compare August 28, 2019 14:01

jbaublitz commented Aug 28, 2019

View reviewed changes

jbaublitz force-pushed the issue-project-51 branch from 9db5c99 to 1db9f15 Compare September 11, 2019 19:38

mulkieran mentioned this pull request Sep 23, 2019

Fix FIXME's introduced by PR#1072 #1074

Open

mulkieran requested changes Sep 23, 2019

View reviewed changes

src/dbus_api/pool.rs Outdated Show resolved Hide resolved

mulkieran requested changes Sep 23, 2019

View reviewed changes

src/dbus_api/pool.rs Outdated Show resolved Hide resolved

src/dbus_api/pool.rs Outdated Show resolved Hide resolved

src/engine/strat_engine/pool.rs Outdated Show resolved Hide resolved

jbaublitz force-pushed the issue-project-51 branch from 1db9f15 to 0795b56 Compare September 24, 2019 13:38

jbaublitz force-pushed the issue-project-51 branch from 41d2b94 to 9d3fb00 Compare September 30, 2019 19:39

jbaublitz force-pushed the issue-project-51 branch 3 times, most recently from 4ab4b4d to 5e4ddf6 Compare October 7, 2019 15:38

mulkieran requested changes Oct 9, 2019

View reviewed changes

jbaublitz force-pushed the issue-project-51 branch from 074036f to ed57a38 Compare October 9, 2019 13:50

mulkieran removed the do not merge label Oct 9, 2019

jbaublitz closed this Oct 9, 2019

jbaublitz mentioned this pull request Oct 9, 2019

Changes to DBus and engine APIs for idempotence #1651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to DBus and engine APIs for idempotence #1614

Changes to DBus and engine APIs for idempotence #1614

jbaublitz commented Aug 21, 2019

jbaublitz commented Aug 21, 2019

jbaublitz commented Aug 22, 2019

mulkieran left a comment

mulkieran left a comment

jbaublitz left a comment

jbaublitz commented Aug 26, 2019

jbaublitz commented Aug 26, 2019

mulkieran commented Aug 26, 2019

mulkieran left a comment

drckeefe commented Aug 26, 2019

mulkieran commented Aug 26, 2019 •

edited

Loading

mulkieran left a comment

jbaublitz left a comment

jbaublitz commented Sep 11, 2019

mulkieran left a comment

mulkieran left a comment

mulkieran commented Oct 2, 2019

jbaublitz commented Oct 2, 2019

jbaublitz commented Oct 2, 2019

mulkieran commented Oct 4, 2019

jbaublitz commented Oct 7, 2019

mulkieran commented Oct 8, 2019

jbaublitz commented Oct 8, 2019

mulkieran commented Oct 8, 2019

jbaublitz commented Oct 8, 2019

mulkieran left a comment

jbaublitz commented Oct 9, 2019

mulkieran commented Oct 9, 2019

jbaublitz commented Oct 9, 2019

Changes to DBus and engine APIs for idempotence #1614

Changes to DBus and engine APIs for idempotence #1614

Conversation

jbaublitz commented Aug 21, 2019

jbaublitz commented Aug 21, 2019

jbaublitz commented Aug 22, 2019

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran left a comment

Choose a reason for hiding this comment

jbaublitz left a comment

Choose a reason for hiding this comment

jbaublitz commented Aug 26, 2019

jbaublitz commented Aug 26, 2019

mulkieran commented Aug 26, 2019

mulkieran left a comment

Choose a reason for hiding this comment

drckeefe commented Aug 26, 2019

mulkieran commented Aug 26, 2019 • edited Loading

mulkieran left a comment

Choose a reason for hiding this comment

jbaublitz left a comment

Choose a reason for hiding this comment

jbaublitz commented Sep 11, 2019

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran left a comment

Choose a reason for hiding this comment

mulkieran commented Oct 2, 2019

jbaublitz commented Oct 2, 2019

jbaublitz commented Oct 2, 2019

mulkieran commented Oct 4, 2019

jbaublitz commented Oct 7, 2019

mulkieran commented Oct 8, 2019

jbaublitz commented Oct 8, 2019

mulkieran commented Oct 8, 2019

jbaublitz commented Oct 8, 2019

mulkieran left a comment

Choose a reason for hiding this comment

jbaublitz commented Oct 9, 2019

mulkieran commented Oct 9, 2019

jbaublitz commented Oct 9, 2019

mulkieran commented Aug 26, 2019 •

edited

Loading