Bring dbus up when we can #1067

tasleson · 2018-07-24T20:49:55Z

We want stratisd to come up before filesystems get mounted, so
that a user can use Stratis backed filesystems in fstab. However,
dbus is not available at this time and we need to be able to start
and run without dbus. When this happens we will periodically
check to see if we can bring up the dbus API.

Fixes: #846

mulkieran · 2018-07-25T18:53:10Z

src/dbus_api/api.rs

-    ),
-    dbus::Error,
-> {
+pub fn connect<'a>(engine: Rc<RefCell<Engine>>) -> Result<DbusConnectionData<'a>, dbus::Error> {


Since this change removes the type complexity, it ought to be able to remove the allow(type_complexity) directive as well.

Agreed, will update

mulkieran · 2018-07-25T19:29:01Z

src/bin/stratisd.rs

        }

+        #[cfg(feature = "dbus_enabled")]
+        let poll_timeout = timeout(dbus_handle.is_none(), eventable.is_none());


I'm guessing that all the arguments to timeout above and below have the wrong values, except for the literal false. Assuming false is a correct argument, then it makes sense for the first argument to evaluate to false if D-Bus is not enabled. So, in the call to timeout above, it makes more sense for the first argument to be dbus_handle.is_some(), which will evaluate to true if stratisd has a D-Bus connection, but false otherwise. I think that a similar argument holds for the second argument.

The arguments are correct and the function is doing the correct thing based on them. However, I can see the confusion. The timeout function simply selects the returned value in a prioritized order of shorter poll when dbus is compiled in but not available vs. an engine that isn't eventable. When dbus_handle.is_none() is true dbus is not available, thus we want to the time out to be shorter, 1 second. When dbus_handle.is_none() is false, then we evaluate if engine is eventable. If eventable.is_none() is true then the engine isn't eventable and we want the timeout to be 10 seconds. However, I need to ask @agrover why this was selected as the comments indicate that when we aren't eventable that the call to the engine is a no-op. Lastly when both dbus and eventable are both false we return -1 which is to wait forever on socket activity.

Obviously I need to re-work this to make it less confusing, so I'll see what I can do. Passing in the reverse and negating the comparisons in timeout may be better.

mulkieran · 2018-07-25T19:30:40Z

src/bin/stratisd.rs

@@ -157,6 +157,18 @@ fn trylock_pid_file() -> StratisResult<File> {
    }
 }

+/// Return desired timeout based on if dbus timeout should be selected, non-eventable timeout
+/// selected or default selected


I believe that this function should be better documented, to explain the meaning of the arguments and also the reason behind the choices of the return values. This will help understand if it is called correctly and also inform any subequent revisions of the values that may become necessary.

With Andy's comment at the meeting, I think we can make this much simpler in that we don't require a separate timeout for eventable.

tasleson · 2018-07-26T19:22:17Z

Reduced poll timeout calculation complexity and removed type complexity allow.

Also rebased.

mulkieran

The source looks fine to me.

However, it would be tidier if you started a new PR with all commits squashed into one, or with two commits, one for the refactoring change which changed the return type of connect, and one for all the rest of the changes.

tasleson · 2018-07-26T20:21:05Z

However, it would be tidier if you started a new PR with all commits squashed into one, or with two commits, one for the refactoring change which changed the return type of connect, and one for all the rest of the changes.
Merge state

Agreed, I can certainly squash. I've been leaving things as separate commits to allow easier review of subsequent changes. However, with the auto merge thing, I guess that's not really appropriate.

tasleson · 2018-07-26T20:51:41Z

Squashed

mulkieran

I'm fine w/ this PR as it is.

But to be clear, I had requested that this PR be closed, and a new PR, with changes squashed, be put up for review, not that the commits in this PR just be squashed.

tasleson · 2018-07-26T22:03:34Z

But to be clear, I had requested that this PR be closed, and a new PR, with changes squashed, be put up for review, not that the commits in this PR just be squashed.
Merge state

I posed this question on IRC and @agrover really said there isn't a defacto process to follow. What in your opinion is the advantage of creating a new PR which is squashed vs. squashing the existing PR?

mulkieran · 2018-07-26T22:38:47Z

For PRs that are being changed due to review, and are fairly complex, it makes sense just to add change commits to the end of the PR. I can explain the reasoning behind this more fully if necessary.

For PRs that are actually committed to the repo, it makes sense to separate the PR into distinct and coherent individual commits, as far as is possible and within limits of difficulty of the task. I can expand on this more fully if necessary.

It makes sense to make these two PRs separate because:

The first type of PR provides a handy record of the reasoning behind decisions made which can be lost or confused by a force push.
It provides the person who submitted the PR with an opportunity to reassess their PR in light of the changes that they have been asked to make before making the final copy.
It is a courtesy to a hypothetical second reviewer who may still have had their reservations about the PR, even if the first reviewer said that in their opinion it was good to go.

We want stratisd to come up before filesystems get mounted, so that a user can use Stratis backed filesystems in fstab. However, dbus is not available at this time and we need to be able to start and run without dbus. When this happens we will periodically check to see if we can bring up the dbus API. Signed-off-by: Tony Asleson <tasleson@redhat.com>

tasleson · 2018-07-30T21:05:00Z

Re-based to correct merge conflict induced by merge of 9b720f9

mulkieran · 2018-08-14T18:44:41Z

It is not clear to me why stratis-cli tests managed to succeed with this change, as they failed on my machine with this change. NOTE: I can not reliably reproduce the problem on my own machine, so at this time I am giving up.

tasleson added this to To do in Stratis 1.0 (Sprint 9) via automation Jul 24, 2018

tasleson self-assigned this Jul 24, 2018

tasleson moved this from To do to In Review in Stratis 1.0 (Sprint 9) Jul 24, 2018

mulkieran requested changes Jul 25, 2018

View reviewed changes

tasleson force-pushed the dbus_delayed_bringup branch from d6c7a1d to f80c668 Compare July 26, 2018 19:20

mulkieran requested changes Jul 26, 2018

View reviewed changes

tasleson force-pushed the dbus_delayed_bringup branch from f80c668 to 72762cc Compare July 26, 2018 20:51

mulkieran previously approved these changes Jul 26, 2018

View reviewed changes

tasleson dismissed mulkieran’s stale review via f2a1236 July 30, 2018 21:03

tasleson force-pushed the dbus_delayed_bringup branch from 72762cc to f2a1236 Compare July 30, 2018 21:03

mulkieran approved these changes Jul 31, 2018

View reviewed changes

agrover approved these changes Aug 1, 2018

View reviewed changes

mergify bot merged commit 0a81f4f into stratis-storage:master Aug 1, 2018

Stratis 1.0 (Sprint 9) automation moved this from In Review to Done (Current Sprint) Aug 1, 2018

agrover moved this from Done (Current Sprint) to Done (Previous Sprints) in Stratis 1.0 (Sprint 9) Aug 2, 2018

tasleson deleted the dbus_delayed_bringup branch September 7, 2018 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring dbus up when we can #1067

Bring dbus up when we can #1067

tasleson commented Jul 24, 2018

mulkieran Jul 25, 2018

tasleson Jul 26, 2018

mulkieran Jul 25, 2018

tasleson Jul 26, 2018 •

edited

mulkieran Jul 25, 2018

tasleson Jul 26, 2018 •

edited

tasleson commented Jul 26, 2018 •

edited

mulkieran left a comment

tasleson commented Jul 26, 2018

tasleson commented Jul 26, 2018

mulkieran left a comment

tasleson commented Jul 26, 2018

mulkieran commented Jul 26, 2018

tasleson commented Jul 30, 2018

mulkieran commented Aug 14, 2018 •

edited

Bring dbus up when we can #1067

Bring dbus up when we can #1067

Conversation

tasleson commented Jul 24, 2018

mulkieran Jul 25, 2018

Choose a reason for hiding this comment

tasleson Jul 26, 2018

Choose a reason for hiding this comment

mulkieran Jul 25, 2018

Choose a reason for hiding this comment

tasleson Jul 26, 2018 • edited

Choose a reason for hiding this comment

mulkieran Jul 25, 2018

Choose a reason for hiding this comment

tasleson Jul 26, 2018 • edited

Choose a reason for hiding this comment

tasleson commented Jul 26, 2018 • edited

mulkieran left a comment

Choose a reason for hiding this comment

tasleson commented Jul 26, 2018

tasleson commented Jul 26, 2018

mulkieran left a comment

Choose a reason for hiding this comment

tasleson commented Jul 26, 2018

mulkieran commented Jul 26, 2018

tasleson commented Jul 30, 2018

mulkieran commented Aug 14, 2018 • edited

tasleson Jul 26, 2018 •

edited

tasleson Jul 26, 2018 •

edited

tasleson commented Jul 26, 2018 •

edited

mulkieran commented Aug 14, 2018 •

edited