Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Julian Foad <ju...@apache.org>]

We seem to have consensus that enabling

   verify_as_revision_before_current_plus_plus()

in production should be safe. It seems like it should be a useful extra 
protection against bug-induced repository corruption. For users who have 
experienced repository corruption and suspect it is bug-induced, or who 
are more concerned about this than about maximizing commit throughput, 
enabling such code would seem to be prudent.

We have long suggested running 'svnadmin verify' on each committed 
revision. Is it because of a policy that we currently ship a 
configuration which by default doesn't do this, favouring speed at the 
expense of consistency checking? Or, when such checking is technically 
available, should our policy be to provide administrative choice?

As I mentioned, one WANdisco customer recently experienced bug-induced 
corruption twice in quick succession, and we were unable to isolate the 
cause. I advised WANdisco to make a special build, enabling this, for 
this customer. However, it is unsatisfactory to ship different builds 
depending on whether a customer currently prefers more speed or more 
reliability.

While I don't support adding too many config knobs, a choice like this 
is something that the software cannot reasonably make by itself, so this 
is a case where a new knob is justified, in my opinion.

Alternatively, given that Subversion's number one priority is supposed 
to be reliability, why would we not enable this verification 
permanently? The argument would, I suppose, be that it is unnecessary. 
Yet we don't have convincing data to support that. Does anyone support 
enabling this permanently? I ask mainly to make us think about it. The 
easy answer is we don't want to reduce commit speed this much (how 
much?) without evidence of need.

Another reason to make it configurable is that then if a user finds 
corruption, like this customer did, they could quickly enable it and 
repeat a similar commit and discover whether it does in fact protect 
against that use case. In the case of the customer mentioned above, we 
lost the opportunity to do that because they had changed their operating 
procedures and moved on for weeks before we were able to get a build 
with this option enabled.

Thoughts, please?

- Julian

Re: [PATCH] Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Stefan Fuhrmann <st...@alice-dsl.de>]

On 16.03.2017 12:50, Julian Foad wrote:
> Daniel Shahaf wrote:
>>> should our policy be to provide administrative choice?
>>
>> A knob sounds good to me [...]
>
> And Bert said "+1" on IRC too.
>
> The attached patch is a basic implementation. The fsfs.conf option is 
> spelled
>
>   [debug]
>   verify-before-commit = true
>
> I put it under [debug] because that section name already exists and 
> holds a "pack after commit" option (not documented in the file 
> template comments) which seems conceptually similar.
>
> What sort of testing do you think this needs? A first level could be 
> to test that the verification code runs when the option is explicitly 
> enabled and doesn't when disabled, and preferably test the default 
> state too. A second level could be to test that the verification 
> actually picks up some (injected) corruption and prevents the commit 
> and exits cleanly. This second level is not directly related to making 
> the feature optional, but it is related to providing the feature at 
> all in a release-mode build.
>
>
>> If we enable this outside maintainer mode, we should look into the
>> points raised upthread about ensuring that 'verify' is read-only.
>> We don't have to fix everything, but we should at least add roadsign
>> comments in the code to reduce the chance that future changes to verify
>> will interact badly with the unusual callsite in 
>> verify_as_revision_before_current_plus_plus().
>
> Ack.
>
>> By the way, are you planning to enable 
>> verify_as_revision_before_current_plus_plus()
>> in alpha3?  I would suggest enabling it unconditionally before alpha3,
>> then reverting it [or adding the knob under discussion] at some point
>> before branching 1.10.x.
>
> Sounds good. To keep track of the need to change the default back 
> again, would a bold coloured box in the release notes would be a 
> suitable place for such a note?
>
>> [We should probably give that function a shorter name... ;)]
>
> Could do. verify_before_commit?

I applied the patch, including the rename, in r1795351.

It adds 50% to our test suite (2:10 instead of 1:26).
If we were to enable it by default in release mode,
we need to add a feature to disable it for mass ops
like 'svnadmin import'.

-- Stefan^2.

Re: [PATCH] Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Daniel Shahaf <d....@daniel.shahaf.name>]

Julian Foad wrote on Thu, Mar 16, 2017 at 11:50:34 +0000:
> What sort of testing do you think this needs? A first level could be to test
> that the verification code runs when the option is explicitly enabled and
> doesn't when disabled, and preferably test the default state too. A second
> level could be to test that the verification actually picks up some
> (injected) corruption and prevents the commit and exits cleanly. This second
> level is not directly related to making the feature optional, but it is
> related to providing the feature at all in a release-mode build.

Another thing we could test: 'make check' with the knob enabled.  I'd
lean to having that use 'svnserve -T' or a threaded MPM, in order to
increase the chances of catching possible bugs related to process-global
state being inappropriately modified by the "Time travel" svn_fs_t
handle.

> Daniel Shahaf wrote:
> >By the way, are you planning to enable verify_as_revision_before_current_plus_plus()
> >in alpha3?  I would suggest enabling it unconditionally before alpha3,
> >then reverting it [or adding the knob under discussion] at some point
> >before branching 1.10.x.
> 
> Sounds good. To keep track of the need to change the default back again,
> would a bold coloured box in the release notes would be a suitable place for
> such a note?

I was thinking of an issue with milestone 1.10.0, but anything we'll run
into before cutting rc1 would be fine.

> >[We should probably give that function a shorter name... ;)]
> 
> Could do. verify_before_commit?

Sure.

> @@ -1017,12 +1028,19 @@ write_config(svn_fs_t *fs,
> +""                                                                           NL
> +"[" CONFIG_SECTION_DEBUG "]"                                                 NL
> +"###"                                                                        NL
> +"### Whether to verify each new revision immediately before finalizing"      NL
> +"### the commit. The default is false in release-mode builds, and true"      NL
> +"### in debug-mode builds."                                                  NL
> +"# " CONFIG_OPTION_VERIFY_BEFORE_COMMIT " = false"                           NL

The phrase "debug-mode builds" isn't quite accurate: on Unix, SVN_DEBUG
is enabled by --enable-maintainer-mode but not by --enable-debug.
(On windows, there's no distinction between maintainer and debug modes,
and SVN_DEBUG is defined in that case.)

I'm not sure whether we should change the comment to match the code, or
vice-versa.

Cheers,

Daniel

[PATCH] Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Julian Foad <ju...@apache.org>]

Daniel Shahaf wrote:
>> should our policy be to provide administrative choice?
>
> A knob sounds good to me [...]

And Bert said "+1" on IRC too.

The attached patch is a basic implementation. The fsfs.conf option is 
spelled

   [debug]
   verify-before-commit = true

I put it under [debug] because that section name already exists and 
holds a "pack after commit" option (not documented in the file template 
comments) which seems conceptually similar.

What sort of testing do you think this needs? A first level could be to 
test that the verification code runs when the option is explicitly 
enabled and doesn't when disabled, and preferably test the default state 
too. A second level could be to test that the verification actually 
picks up some (injected) corruption and prevents the commit and exits 
cleanly. This second level is not directly related to making the feature 
optional, but it is related to providing the feature at all in a 
release-mode build.


> If we enable this outside maintainer mode, we should look into the
> points raised upthread about ensuring that 'verify' is read-only.
> We don't have to fix everything, but we should at least add roadsign
> comments in the code to reduce the chance that future changes to verify
> will interact badly with the unusual callsite in verify_as_revision_before_current_plus_plus().

Ack.

> By the way, are you planning to enable verify_as_revision_before_current_plus_plus()
> in alpha3?  I would suggest enabling it unconditionally before alpha3,
> then reverting it [or adding the knob under discussion] at some point
> before branching 1.10.x.

Sounds good. To keep track of the need to change the default back again, 
would a bold coloured box in the release notes would be a suitable place 
for such a note?

> [We should probably give that function a shorter name... ;)]

Could do. verify_before_commit?

- Julian

Re: verify_as_revision_before_current_plus_plus() on a production repo?

[Daniel Shahaf <d....@daniel.shahaf.name>]

Julian Foad wrote on Mon, Mar 13, 2017 at 11:27:32 +0000:
> We seem to have consensus that enabling
> 
>   verify_as_revision_before_current_plus_plus()
> 
> in production should be safe. It seems like it should be a useful extra
> protection against bug-induced repository corruption.

I think we're as confident about this code as we can be about code that
hasn't been used in production.

> For users who have experienced repository corruption and suspect it is
> bug-induced, or who are more concerned about this than about
> maximizing commit throughput, enabling such code would seem to be
> prudent.

+1

> should our policy be to provide administrative choice?

> Thoughts, please?

A knob sounds good to me: both because this is a new codepath (the FS
layer should be conservative) and because we don't know everyone's use
patterns.

The usual problem with knobs is that they mandate maintaining multiple
codepaths; this consideration does not apply to a knob that simply
enables/disables a sanity check.

If we enable this outside maintainer mode, we should look into the
points raised upthread about ensuring that 'verify' is read-only.
We don't have to fix everything, but we should at least add roadsign
comments in the code to reduce the chance that future changes to verify
will interact badly with the unusual callsite in verify_as_revision_before_current_plus_plus().

> - Julian

By the way, are you planning to enable verify_as_revision_before_current_plus_plus()
in alpha3?  I would suggest enabling it unconditionally before alpha3,
then reverting it [or adding the knob under discussion] at some point
before branching 1.10.x.

Cheers,

Daniel
[We should probably give that function a shorter name... ;)]