https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422016-02-29T15:42:47ZArvadosArvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=358502016-02-29T15:42:47ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Target version</strong> set to <i>Arvados Future Sprints</i></li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=360962016-03-02T20:02:05ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/36096/diff?detail_id=35367">diff</a>)</li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=360972016-03-02T20:04:31ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/36097/diff?detail_id=35368">diff</a>)</li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=361182016-03-02T20:25:50ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Category</strong> set to <i>Keep</i></li><li><strong>Assigned To</strong> set to <i>Radhika Chippada</i></li><li><strong>Target version</strong> changed from <i>Arvados Future Sprints</i> to <i>2016-03-16 sprint</i></li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=362162016-03-04T15:15:28ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=362552016-03-07T20:02:21ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/36255/diff?detail_id=35501">diff</a>)</li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=364742016-03-11T21:01:37ZTom Cleggtom@curii.com
<ul></ul><p>(from irc) Duration flags should be DurationVar not IntVar</p>
<p>The doneEmptyingTrash line here should move down two lines, so it stops the trash collector after a signal is received, rather than during program startup...<br /><pre><code class="diff syntaxhl"> go func(sig <-chan os.Signal) {
<span class="gi">+ doneEmptyingTrash <- true
</span> s := <-sig
log.Println("caught signal:", s)
listener.Close()
</code></pre></p>
<p>It would be better (and easier to test reliably) to pass the <code>doneEmptyingTrash</code> channel to the <code>emptyTrash()</code> func explicitly rather than using a global.</p>
<p>I think this should break if <code>err == nil</code>, not if <code>err != nil</code>. (With the current code, we give up as soon as we <em>fail</em> to untrash a block. Shouldn't we stop as soon as we <em>succeed</em> in untrashing, and return an error only if <em>none</em> of the trash we found was untrashable?)</p>
<pre><code class="go syntaxhl"> <span class="k">for</span> <span class="n">_</span><span class="p">,</span> <span class="n">f</span> <span class="o">:=</span> <span class="k">range</span> <span class="n">files</span> <span class="p">{</span>
<span class="k">if</span> <span class="n">strings</span><span class="o">.</span><span class="n">HasPrefix</span><span class="p">(</span><span class="n">f</span><span class="o">.</span><span class="n">Name</span><span class="p">(),</span> <span class="n">prefix</span><span class="p">)</span> <span class="p">{</span>
<span class="n">err</span> <span class="o">=</span> <span class="n">os</span><span class="o">.</span><span class="n">Rename</span><span class="p">(</span><span class="n">v</span><span class="o">.</span><span class="n">blockPath</span><span class="p">(</span><span class="n">f</span><span class="o">.</span><span class="n">Name</span><span class="p">()),</span> <span class="n">v</span><span class="o">.</span><span class="n">blockPath</span><span class="p">(</span><span class="n">loc</span><span class="p">))</span>
<span class="k">if</span> <span class="n">err</span> <span class="o">!=</span> <span class="no">nil</span> <span class="p">{</span>
<span class="k">break</span>
<span class="p">}</span>
<span class="p">}</span>
<span class="p">}</span>
</code></pre>
<p>Also in Untrash, if no trashed block was found, we should return a non-nil error that satisfies <code>os.IsNotExist()</code>.</p>
<p>This regexp should be more careful to only match files that trash could conceivably be used by the trash function. Suggest:<br /><pre><code class="diff syntaxhl"><span class="gd">-var trashRegexp = regexp.MustCompile(`.*([0-9a-fA-F]{32}).trash.(\d+)`)
</span><span class="gi">+var trashRegexp = regexp.MustCompile(`/([0-9a-f]{32}).trash.(\d+)$`)
</span></code></pre></p>
<p>Style: Please un-pyramid the code in <code>(v *UnixVolume) EmptyTrash</code>; e.g., <code>return nil</code> after <code>log.Printf</code>, instead of putting the rest of the func in an <code>else</code>.<br /><pre>
if err != nil {
log.Printf("EmptyTrash error for %v: %v", path, err)
} else if !info.Mode().IsDir() {
matches := trashRegexp.FindStringSubmatch(path)
if len(matches) == 3 {
...
</pre></p>
<p>Also in <code>(v *UnixVolume) EmptyTrash</code> -- either it should return a non-nil error if an error is encountered during filepath.Walk() (either passed in to the walk function, or encountered in the walk function), or it should not have a function signature that suggests it is capable of returning errors. The latter is probably more reasonable: the only thing the caller would do differently is log the error, and we already log errors right from EmptyTrash.</p>
<p><code>(v *UnixVolume) EmptyTrash</code> should print stats even if it encountered an error (i.e., the stats shouldn't be in an "else")</p>
The volume tests shouldn't test the emptyTrash goroutine. Instead they should just call EmptyTrash on the volume being tested. This way it won't be necessary to rely on sleep(), and we can reduce the three nearly-identical test functions to one sequence that checks various behaviors:
<ul>
<li>put block</li>
<li>set trashLifetime=time.Hour</li>
<li>trash block</li>
<li><code>if err == ErrNotImplemented { return }</code> (rest of the tests don't apply)</li>
<li>get block → <code>os.IsNotExist(err)</code></li>
<li>empty trash</li>
<li>untrash block</li>
<li>get block → <code>err == nil</code></li>
<li>untrash block → <code>os.IsNotExist(err)</code></li>
<li>get block → <code>err == nil</code></li>
<li>set trashLifetime=time.Nanosecond</li>
<li>trash block</li>
<li>untrash block → <code>err == nil</code> (confirm the data doesn't go away until we call EmptyTrash)</li>
<li>trash block</li>
<li>empty trash</li>
<li>untrash block → <code>os.IsNotExist(err)</code></li>
<li>get block → <code>os.IsNotExist(err)</code></li>
<li>put block</li>
<li>set trashLifetime=time.Nanosecond</li>
<li>trash block</li>
<li>put same block again → <code>err == nil</code></li>
<li>empty trash</li>
<li>get block → <code>err == nil</code> (confirm we don't delete an un-trashed block by mistake even if the same block is also in trash)</li>
<li>trash block</li>
<li>set trashLifetime=time.Hour</li>
<li>trash block → <code>err == nil</code></li>
<li>empty trash</li>
<li>untrash block → <code>err == nil</code> (confirm we don't delete the unexpired copy, even if the same block is in both expired and unexpired trash)</li>
</ul>
<p>I think if we change the empty-trash condition from "deadline < now" to "deadline <= now" then a trash lifetime of 1ns should assure us that whatever we put in the trash is eligible for deletion right away (even if we empty trash in the same nanosecond, the unix timestamp only has 1-second precision so a trash lifetime under 1s would effectively mean "eligible for emptying right now").</p>
<p>The existing "delete" tests already test for correct behavior when <code>trashLifetime==0</code> so we don't need to add more tests for that, right?</p>
<p>Nit: Instead of parsing as int with Atoi and then casting to int64, we could just use strconv.ParseInt here:<br /><pre><code class="diff syntaxhl"><span class="gd">-deadline, err := strconv.Atoi(matches[2])
-...
-if int64(deadline) < ...
</span><span class="gi">+deadline, err := strconv.ParseInt(matches[2], 10, 64)
+...
+if deadline < ...
</span></code></pre></p> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=364932016-03-14T14:31:52ZRadhika Chippadaradhika@curoverse.com
<ul></ul><blockquote>
<p>(from irc) Duration flags should be DurationVar not IntVar</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>The doneEmptyingTrash line here should move down two lines, so it stops the trash collector after a signal is received, rather than during program startup...</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>It would be better (and easier to test reliably) to pass the doneEmptyingTrash channel to the emptyTrash() func explicitly rather than using a global.</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>I think this should break if err == nil, not if err != nil.</p>
</blockquote>
<p>Good catch. Fixed</p>
<blockquote>
<p>and return an error only if none of the trash we found was untrashable?</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>Also in Untrash, if no trashed block was found, we should return a non-nil error that satisfies os.IsNotExist().</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>This regexp should be more careful to only match files that trash could conceivably be used by the trash function. Suggest:</p>
</blockquote>
<p>Updated</p>
<blockquote>
<p>Style: Please un-pyramid the code</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>Also in (v *UnixVolume) EmptyTrash -- either it should return a non-nil error if an error is encountered ...</p>
</blockquote>
<blockquote>
<p>Done (changed to not return error)</p>
</blockquote>
<blockquote>
<p>(v *UnixVolume) EmptyTrash should print stats even if it encountered an error (i.e., the stats shouldn't be in an "else")</p>
</blockquote>
<p>Done</p>
<blockquote>
<p>Test sequence listed ...</p>
</blockquote>
<p>Added test with the listed sequence. (There is one extra trash listed in this sequence: the fifth from the bottom before changing trashLifetime to 1 hour. I omitted this)</p>
<blockquote>
<p>The volume tests shouldn't test the emptyTrash goroutine. Instead they should just call EmptyTrash on the volume being tested.</p>
</blockquote>
<p>I am uncomfortable with the idea that we do not test the emptyTrash goroutine at all. Instead of removing this (after adding your suggested test sequence), I improved the test with the goroutine to use 1ns for trashLifetime so that it does not waste time during the test. But if you'd rather remove it, I will remove it.</p>
<blockquote>
<p>The existing "delete" tests already test for correct behavior when trashLifetime==0 so we don't need to add more tests for that, right?</p>
</blockquote>
<p>Deleted it</p>
<blockquote>
<p>Nit: Instead of parsing as int with Atoi and then casting to int64, we could just use strconv.ParseInt here:</p>
</blockquote>
<p>Done</p> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=365412016-03-15T14:01:07ZTom Cleggtom@curii.com
<ul></ul><blockquote>
<p>I am uncomfortable with the idea that we do not test the emptyTrash goroutine at all. Instead of removing this (after adding your suggested test sequence), I improved the test with the goroutine to use 1ns for trashLifetime so that it does not waste time during the test. But if you'd rather remove it, I will remove it.</p>
</blockquote>
<p>The idea of testing emptyTrash is nice, but having it fire every 1ns while we're running our volume tests doesn't seem like a very effective way to test it. We still have to call EmptyTrash ourselves in order to avoid races, so the goroutine mainly has the effect of making the test flaky:</p>
<pre>
--- FAIL: TestUnixVolumeWithGenericTests (0.32s)
volume_generic_test.go:1007: file does not exist
</pre>
<p>We could test the emptyTrash goroutine separately by setting up some UnixVolumes, calling Put and Trash, and waiting in a busy loop for the trashed data to disappear. This doesn't belong in the generic volume test suite, though -- we don't need to test it for each volume type/config.</p> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=365462016-03-15T15:28:01ZRadhika Chippadaradhika@curoverse.com
<ul></ul><p>Deleted testTrashUntrashWithEmptyTrashGoroutine</p> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=366172016-03-16T15:19:31ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Story points</strong> set to <i>2.0</i></li></ul> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=366232016-03-16T16:20:05ZTom Cleggtom@curii.com
<ul></ul><p>Radhika Chippada wrote:</p>
<blockquote>
<p>Added test with the listed sequence. (There is one extra trash listed in this sequence: the fifth from the bottom before changing trashLifetime to 1 hour. I omitted this)</p>
</blockquote>
<p>I've added that in, with comments. The idea is to put the same data in the trash twice, with different deadlines, and then empty the trash after the first deadline has passed but before the second one arrives. In this case it should still be possible to untrash: i.e., we don't throw out the "new" trash just because we also had identical "old" trash.</p>
<p>Also escaped the "."s in the regexp and moved it next to the function that uses it.</p>
<p><a class="changeset" title="8554: Dedup Get() checks, add comments, fix up regexp." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/9c571f5fcbdf8120c30ab29b1ba3a41f52d37425">9c571f5</a> look ok?</p> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=366242016-03-16T16:42:47ZTom Cleggtom@curii.com
<ul></ul><p>...and the rest of the un-pyramid thing → <a class="changeset" title="8554: Un-pyramid code" href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/7309faba0c878f9ae8a195e089eb6e71dc1d197a">7309fab</a></p> Arvados - Feature #8554: [Keep] Implement trash/untrash behavior in volume_unixhttps://dev.arvados.org/issues/8554?journal_id=366642016-03-16T19:25:07ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p>Applied in changeset arvados|commit:5ba59abf2fece90309815b15b2d118860fb3b1b3.</p>