Merge ~kzapalowicz/snappy-hwe-snaps/+git/udisks2:fix/fail-to-mount-after-unmount into ~snappy-hwe-team/snappy-hwe-snaps/+git/udisks2:master

[Konrad Zapałowicz]

Make sure that unmount Completed and Errors channels are
subscribed so that the ciborium go routines do not block
on them.

This fixes https://bugs.launchpad.net/plano/+bug/1689310

[System Enablement Bot (system-enablement-ci-bot)]

FAILED: Continuous integration, rev:81eedabe21e2e0ca4fd997df6412c022695dde52
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1527/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1747/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1435/console
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1334

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1527/rebuild

review: Needs Fixing (continuous-integration)

[System Enablement Bot (system-enablement-ci-bot)]

FAILED: Continuous integration, rev:fa4c532e6c2c945aab4b79c5263c47338f077f0b
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1528/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1748/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1436/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1335/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1528/rebuild

review: Needs Fixing (continuous-integration)

[System Enablement Bot (system-enablement-ci-bot)]

PASSED: Continuous integration, rev:cc57ba0d42f010ae044edbfee3ff6eb1cfe1f33c
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1529/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1759
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1437/console
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-test-snap/2142
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1346/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1529/rebuild

review: Approve (continuous-integration)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

[Tony Espy (awe)]

@Konrad

This merge proposes code changes to the master branch. I thought our code changes always were made on the upstream branches except for packaging type scripts which live in the packaging branch (eg. service command wrappers, ...). As ciborium isn't part of upstream udisks2, it's probably OK to live in master, but there are actual changes proposed to udisks2 code in this merge proposal as well. Was this intentional?

review: Needs Information

[Konrad Zapałowicz (kzapalowicz)]

I do not understand, what do you mean by upstream branch?

> @Konrad
>
> This merge proposes code changes to the master branch. I thought our code
> changes always were made on the upstream branches except for packaging type
> scripts which live in the packaging branch (eg. service command wrappers,
> ...). As ciborium isn't part of upstream udisks2, it's probably OK to live in
> master, but there are actual changes proposed to udisks2 code in this merge
> proposal as well. Was this intentional?

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

@Tony, I think you are getting confused by the weird way the repo/ciborium seems to be structured.

snapcraft.yaml says that the sources for "ciborium" branch is ".". The thing is that those sources include "udisks2" folder, *which is part of ciborium sources*, not of udisks2. If you take a look inside that folder it is all go code.

[Tony Espy (awe)]

@Alfonso

Thanks for taking a look, and you're correct, all of the code is ciborium, even though some of the paths start with "udisks2".

IMHOP the addition of ciborium to repo's packaging branch was a mistake. This probably should have been added to it's own upstream branch. We should discuss next week, and if all agree, create a card for fixing this.

[Konrad Zapałowicz (kzapalowicz)]

@Alfonso, thanks for explanations

@Tony, if all now good then ack please :-)

[Tony Espy (awe)]

A couple of comments:

1) Commit cc57ba0 & 5273bbe both essentially do the same thing, they make a channel write operation to an unbuffered channel non-blocking, and discard whatever was to be sent. As such, I think the commit messages should look similar, but they're different. I'd suggest:

udisks2: use non-blocking channel writes

This commit makes sure that if the client does not subscribe to
the unmountCompleted channel, a write to it won't block, and the
event discarded.

dispatcher: use non-blocking channel writes

This commit makes sure that writes to the Additions or Jobs channel
won't block if there's no reader. In both cases the event is discarded.

2) You've essentially fixed the bug with unmountCompleted two ways. You first added non-blocking IO writes in udisks2.go, but then you changed the client in commit fa4c532 to uses SubscribeUnmountEvents. Note, you only added the non-blocking IO for writes to unmountCompleted, not unmountError. I would think that just fa4c532 alone would fix the problem, wouldn't it?

3) Is dropping events in the dispatcher the right thing to do? Don't these events correlate to DBus messages from udisks2? Under what conditions would these two channels (Jobs or Additions) not be read? What are the side effects of these events being dropped?

review: Needs Fixing

[Simon Fels (morphis)]

> IMHOP the addition of ciborium to repo's packaging branch was a
> mistake. This probably should have been added to it's own
> upstream branch. We should discuss next week, and if all agree,
> create a card for fixing this.

It's not a mistake as we merged the portions of ciborium we really need on purpose into master. We have modified ciborium already in a way that it doesn't make sense to upstream it also given that there is no upstream anymore. What we have here is a stripped down version of the original ciborium code (mainly just the udisks2 library of it) which has all UI related parts removed and an extra chunk of unit tests added. There is no sense anymore calling this an "upstream" project anymore. We agreed on this a while back when we discussed the addition of ciborium in general.

> As ciborium isn't part of upstream udisks2, it's probably OK to
> live in master, but there are actual changes proposed to udisks2
> code in this merge proposal as well. Was this intentional?

In general master doesn't have to be a packaging-only branch. It is fine to ship code here as well which is specific to the snap and not used elsewhere or coming from an upstream project. As described above portions of ciborium (not ciborium itself) was moved into master as this is really a new, udisks2 snap specific thing, we want to maintain and develop on its own. Because of this it is fine to live in master.

> @Tony, I think you are getting confused by the weird way the
> repo/ciborium seems to be structured.

Go structures its code into packages where each folder becomes its own package. I agree that we maybe should move all Go code into a ciborium subfolder (but lets not call it ciborium as it is not ciborium anymore).

[Simon Fels (morphis)]

review: Needs Fixing

[Konrad Zapałowicz (kzapalowicz)]

> > @Tony, I think you are getting confused by the weird way the
> > repo/ciborium seems to be structured.
>
> Go structures its code into packages where each folder becomes its own
> package. I agree that we maybe should move all Go code into a ciborium
> subfolder (but lets not call it ciborium as it is not ciborium anymore).

I have found the naming problematic while trying to understand how the
different components play together here. I mean that we have udisksd and
something that lays in udisks2 folder which is - in fact - a stripped
version of ciborium.

[Konrad Zapałowicz (kzapalowicz)]

>
> A couple of comments:
>
> 1) Commit cc57ba0 & 5273bbe both essentially do the same thing, they make a
> channel write operation to an unbuffered channel non-blocking, and discard
> whatever was to be sent. As such, I think the commit messages should look
> similar, but they're different. I'd suggest:
>
> udisks2: use non-blocking channel writes
>
> This commit makes sure that if the client does not subscribe to
> the unmountCompleted channel, a write to it won't block, and the
> event discarded.
>
> dispatcher: use non-blocking channel writes
>
> This commit makes sure that writes to the Additions or Jobs channel
> won't block if there's no reader. In both cases the event is discarded.
>
> 2) You've essentially fixed the bug with unmountCompleted two ways. You first
> added non-blocking IO writes in udisks2.go, but then you changed the client in
> commit fa4c532 to uses SubscribeUnmountEvents. Note, you only added the non-
> blocking IO for writes to unmountCompleted, not unmountError. I would think
> that just fa4c532 alone would fix the problem, wouldn't it?

Yeah it would however since udisks2 is a separate package I feel like it is
equally important to make sure it will not block.

> 3) Is dropping events in the dispatcher the right thing to do? Don't these
> events correlate to DBus messages from udisks2? Under what conditions would
> these two channels (Jobs or Additions) not be read? What are the side effects
> of these events being dropped?

Without discarding the events the dispatcher blocks completely and will not
operate. I agree that discarding is not ideal however it guarantees that
the dispatcher will remain operational.

[Tony Espy (awe)]

@Simon

Yes, there's no reason we can't keep the code in this branch, although it would've been much cleaner to have placed it in its own branch. We have bigger issues to deal with, so lets just leave as is for now.

[Tony Espy (awe)]

@Konrad

>> 2) You've essentially fixed the bug with unmountCompleted two ways. You first
>> added non-blocking IO writes in udisks2.go, but then you changed the client in
>> commit fa4c532 to uses SubscribeUnmountEvents. Note, you only added the non-
>> blocking IO for writes to unmountCompleted, not unmountError. I would think
>> that just fa4c532 alone would fix the problem, wouldn't it?
>
>Yeah it would however since udisks2 is a separate package I feel like it is
>equally important to make sure it will not block.

Sure, but you only fixed one specific case. The rest of ciborium is full of blocking channel code.

>> 3) Is dropping events in the dispatcher the right thing to do? Don't these
>> events correlate to DBus messages from udisks2? Under what conditions would
>> these two channels (Jobs or Additions) not be read? What are the side effects
>> of these events being dropped?
>
>Without discarding the events the dispatcher blocks completely and will not
>operate. I agree that discarding is not ideal however it guarantees that
>the dispatcher will remain operational.

How can this happen if we control both sides? What's the side-effect of dropping these events?

[System Enablement Bot (system-enablement-ci-bot)]

FAILED: Continuous integration, rev:5258116bac8293e14b2eec297abedf19bbf0714c
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1549/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1849/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1457/console
    FAILURE: https://jenkins.canonical.com/system-enablement/job/generic-test-snap/2231/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1428/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1549/rebuild

review: Needs Fixing (continuous-integration)

[System Enablement Bot (system-enablement-ci-bot)]

PASSED: Continuous integration, rev:f4519ed2997d8bfcdfcb416e406a6504a041483e
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1551/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1861
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1459/console
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-test-snap/2243
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1440/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1551/rebuild

review: Approve (continuous-integration)

[Simon Fels (morphis)]

LGTM

review: Approve

[Tony Espy (awe)]

+1

review: Approve

diff --git a/cmd/ciborium/main.go b/cmd/ciborium/main.go
index 8e6d605..a5d909e 100644
--- a/cmd/ciborium/main.go
+++ b/cmd/ciborium/main.go
@@ -67,6 +67,7 @@ func main() {
 	blockAdded, blockError := udisks2.SubscribeAddEvents()
 	mountRemoved := udisks2.SubscribeRemoveEvents()
 	mountCompleted, mountErrors := udisks2.SubscribeMountEvents()
+	unmountCompleted, unmountErrors := udisks2.SubscribeUnmountEvents()
 
 	// create a routine per couple of channels, the select algorithm will make use
 	// ignore some events if more than one channels is being written to the algorithm
@@ -87,6 +88,10 @@ func main() {
 				log.Println("Failed to mount device:", e)
 			case path := <-mountCompleted:
 				log.Println("Successfully mount device:", path)
+			case e := <-unmountErrors:
+				log.Println("Failed to unmount device:", e)
+			case path := <-unmountCompleted:
+				log.Println("Successfully unmount device:", path)
 			case m := <-mountRemoved:
 				log.Println("Path removed", m)
 			}