Re: Problems with commit feature negotiation and write-through proxies

[Philip Martin]

Evgeny Kotkov  writes:

> I would be fine with this approach as well, but perhaps with a few tweaks:

Right, I'll incorporate those changes, test it and commit.

-- 
Philip

Re: Problems with commit feature negotiation and write-through proxies

[Evgeny Kotkov]

Evgeny Kotkov  writes:

>> Index: subversion/mod_dav_svn/version.c
>> ===
>> --- subversion/mod_dav_svn/version.c(revision 1820704)
>> +++ subversion/mod_dav_svn/version.c(working copy)
>> @@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
>>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
>>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
>>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
>> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
>> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
>> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
>>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
>>/* Mergeinfo is a special case: here we merely say that the server
>> * knows how to handle mergeinfo -- whether the repository does too
>> @@ -297,6 +294,19 @@ get_option(const dav_resource *resource,
>>  { "create-txn-with-props",  { 1, 8, 0, "" } },
>>};
>>
>> +  /* These capabilities are used during commit and when acting as
>> + a WebDAV slave (SVNMasterURI) their availablity depends on
>> + the master version (SVNMasterVersion) rather than our own
>> + (slave) version. */
>> +  struct capability_versions_t {
>> +const char *capability_name;
>> +svn_version_t min_version;
>> +  } capabilities[] = {
>> +{ SVN_DAV_NS_DAV_SVN_SVNDIFF1, { 1, 7, 0, ""} },
>> +{ SVN_DAV_NS_DAV_SVN_SVNDIFF2, { 1, 10, 0, ""} },
>> +{ SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM, { 1, 10, 0, ""} },
>> +  };
>
> I would be fine with this approach as well, but perhaps with a few tweaks:

I also notice that this patch only advertises the new capabilities if the
server is configured to enable HTTPv2.  This is probably unintended, as
I think that there is no reason to disable these PUT-related capabilities
(such as being able to parse svndiff1/svndiff2) if the server is configured
to prefer HTTPv1 protocol with SVNAdvertiseV2Protocol off.


Thanks,
Evgeny Kotkov

Re: Problems with commit feature negotiation and write-through proxies

[Evgeny Kotkov]

Philip Martin  writes:

> We already have system for handling create-txn and create-txn-with-props
> so I was thinking of extending that code:
>
> Index: subversion/mod_dav_svn/version.c
> ===
> --- subversion/mod_dav_svn/version.c(revision 1820704)
> +++ subversion/mod_dav_svn/version.c(working copy)
> @@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
> -  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
>apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
>/* Mergeinfo is a special case: here we merely say that the server
> * knows how to handle mergeinfo -- whether the repository does too
> @@ -297,6 +294,19 @@ get_option(const dav_resource *resource,
>  { "create-txn-with-props",  { 1, 8, 0, "" } },
>};
>
> +  /* These capabilities are used during commit and when acting as
> + a WebDAV slave (SVNMasterURI) their availablity depends on
> + the master version (SVNMasterVersion) rather than our own
> + (slave) version. */
> +  struct capability_versions_t {
> +const char *capability_name;
> +svn_version_t min_version;
> +  } capabilities[] = {
> +{ SVN_DAV_NS_DAV_SVN_SVNDIFF1, { 1, 7, 0, ""} },
> +{ SVN_DAV_NS_DAV_SVN_SVNDIFF2, { 1, 10, 0, ""} },
> +{ SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM, { 1, 10, 0, ""} },
> +  };

I would be fine with this approach as well, but perhaps with a few tweaks:

 - I think that it would be better to handle all four capabilities that
   relate to proxied writes in the same place:

 SVN_DAV_NS_DAV_SVN_EPHEMERAL_TXNPROPS
 SVN_DAV_NS_DAV_SVN_SVNDIFF1
 SVN_DAV_NS_DAV_SVN_SVNDIFF2
 SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM

   Otherwise, the handling becomes asymmetric, as the first of them is still
   checked using the separate dav_svn__check_ephemeral_txnprops_support()
   function, but the three new capabilities are checked using the new array-
   based approach.

 - The SVN_DAV_NS_DAV_SVN_SVNDIFF1 capability should probably be
   bound to version 1.10 instead of 1.7.

   The reason for that is that mod_dav_svn didn't properly advertise
   svndiff1 support with this specific header until 1.10, and I think
   that we should have the same behavior for the setups with a write-
   through proxy.

   (Even though mod_dav_svn has been able to parse it for a long time,
currently ra_serf avoids sending svndiff1 to servers that don't
advertise it with this specific header, as there might be third-party
implementations of the Subversion's HTTP protocol that can only
parse svndiff0).


Thanks,
Evgeny Kotkov

Re: Problems with commit feature negotiation and write-through proxies

[Philip Martin]

Evgeny Kotkov  writes:

> Philip Martin  writes:
>
>> In the past we supported different versions on the master and slave and
>> we introduced the SVNMasterVersion directive to configure it.
>> Unfortunately that information is not immediately available in
>> get_vsn_options() where the server advertises some commit features.  If
>> I simply remove the SVN_DAV_NS_DAV_SVN_SVNDIFF2 header from
>> get_vsn_options() then I can commit.
>>
>> I'm not sure how we fix this.  Do we delay the svndiff2 negotiation
>> until later in the commit?  Do we abandon mixed master/slave versions?
>
> I think that we might be able to selectively stop advertising the new-
> in-1.10 capabilities based on the SVNMasterVersion value, similarly to
> how we handle SVN_DAV_NS_DAV_SVN_EPHEMERAL_TXNPROPS.
>
> Perhaps, we could do something along the lines of the attached patch?

We already have system for handling create-txn and create-txn-with-props
so I was thinking of extending that code:

Index: subversion/mod_dav_svn/version.c
===
--- subversion/mod_dav_svn/version.c(revision 1820704)
+++ subversion/mod_dav_svn/version.c(working copy)
@@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
   /* Mergeinfo is a special case: here we merely say that the server
* knows how to handle mergeinfo -- whether the repository does too
@@ -297,6 +294,19 @@ get_option(const dav_resource *resource,
 { "create-txn-with-props",  { 1, 8, 0, "" } },
   };
 
+  /* These capabilities are used during commit and when acting as
+ a WebDAV slave (SVNMasterURI) their availablity depends on
+ the master version (SVNMasterVersion) rather than our own
+ (slave) version. */
+  struct capability_versions_t {
+const char *capability_name;
+svn_version_t min_version;
+  } capabilities[] = {
+{ SVN_DAV_NS_DAV_SVN_SVNDIFF1, { 1, 7, 0, ""} },
+{ SVN_DAV_NS_DAV_SVN_SVNDIFF2, { 1, 10, 0, ""} },
+{ SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM, { 1, 10, 0, ""} },
+  };
+
   /* Add the header which indicates that this server can handle
  replay REPORTs submitted against an HTTP v2 revision resource. */
   apr_table_addn(r->headers_out, "DAV",
@@ -347,6 +357,21 @@ get_option(const dav_resource *resource,
   apr_table_addn(r->headers_out, SVN_DAV_SUPPORTED_POSTS_HEADER,
  apr_pstrdup(r->pool, posts_versions[i].post_name));
 }
+
+  /* Report the capabilites that rely on the master version. */
+  for (i = 0; i < sizeof(capabilities)/sizeof(capabilities[0]); ++i)
+{
+  if (master_version
+  && (!svn_version__at_least(master_version,
+ capabilities[i].min_version.major,
+ capabilities[i].min_version.minor,
+ capabilities[i].min_version.patch)))
+continue;
+
+  apr_table_addn(r->headers_out, "DAV",
+ capabilities[i].capability_name);
+
+}
 }
 
   return NULL;


-- 
Philip

Re: Removal of spark buildbot

[Stefan Sperling]

On Wed, Jan 10, 2018 at 09:23:49PM +1100, Gavin McDonald wrote:
> > On 10 Jan 2018, at 9:18 pm, Stefan Sperling  wrote:
> > I am currently setting up a new OpenBSD/sparc64 buildbot.
> > I'll send more info once I've got it completely set up.
> 
> Good news!
> 
> In the meantime this sparc box and config have been removed.
> 
> Will be glad to assist in getting the new one attached to our master.
> 
> Thanks
> 
> Gav…

Hey Gavin,

I am almost ready to connect my new bot.
Just now doing another test build after tweaking my scripts at
https://svn.apache.org/repos/asf/subversion/trunk/tools/buildbot/slaves/bb-openbsd

I've lost the buildslave password. Can you tell me what I could do to
view the password or to get it changed?

I suppose the final step would be to re-enable bb-openbsd in our slave config?
Since I can access that file in SVN so I could do that myself, I guess.

Thanks,
Stefan

Re: Problems with commit feature negotiation and write-through proxies

[Evgeny Kotkov]

Philip Martin  writes:

> In the past we supported different versions on the master and slave and
> we introduced the SVNMasterVersion directive to configure it.
> Unfortunately that information is not immediately available in
> get_vsn_options() where the server advertises some commit features.  If
> I simply remove the SVN_DAV_NS_DAV_SVN_SVNDIFF2 header from
> get_vsn_options() then I can commit.
>
> I'm not sure how we fix this.  Do we delay the svndiff2 negotiation
> until later in the commit?  Do we abandon mixed master/slave versions?

I think that we might be able to selectively stop advertising the new-
in-1.10 capabilities based on the SVNMasterVersion value, similarly to
how we handle SVN_DAV_NS_DAV_SVN_EPHEMERAL_TXNPROPS.

Perhaps, we could do something along the lines of the attached patch?


Thanks,
Evgeny Kotkov
Index: subversion/mod_dav_svn/dav_svn.h
===
--- subversion/mod_dav_svn/dav_svn.h(revision 1820731)
+++ subversion/mod_dav_svn/dav_svn.h(working copy)
@@ -359,12 +359,24 @@ svn_boolean_t dav_svn__get_list_parentpath_flag(re
master server version (if provided via SVNMasterVersion).  */
 svn_boolean_t dav_svn__check_httpv2_support(request_rec *r);
 
-/* For the repository referred to by this request, should ephemeral
-   txnprop support be advertised?  */
-svn_boolean_t dav_svn__check_ephemeral_txnprops_support(request_rec *r);
+typedef enum dav_svn__commit_capability_t
+{
+  dav_svn__capability_ephemeral_txnprops,
+  dav_svn__capability_svndiff1,
+  dav_svn__capability_svndiff2,
+  dav_svn__capability_put_result_checksum
+} dav_svn__commit_capability_t;
 
+/* For the repository referred to by this request, is the specified
+   commit-related capability supported?  Note that this also takes into
+   account the support level expected of based on the specified
+   master server version (if provided via SVNMasterVersion).  */
+svn_boolean_t
+dav_svn__check_commit_capability(request_rec *r,
+ dav_svn__commit_capability_t capability);
 
 
+
 /* SPECIAL URI
 
SVN needs to create many types of "pseudo resources" -- resources
Index: subversion/mod_dav_svn/mod_dav_svn.c
===
--- subversion/mod_dav_svn/mod_dav_svn.c(revision 1820731)
+++ subversion/mod_dav_svn/mod_dav_svn.c(working copy)
@@ -924,17 +924,30 @@ dav_svn__check_httpv2_support(request_rec *r)
 
 
 svn_boolean_t
-dav_svn__check_ephemeral_txnprops_support(request_rec *r)
+dav_svn__check_commit_capability(request_rec *r,
+ dav_svn__commit_capability_t capability)
 {
   svn_version_t *version = dav_svn__get_master_version(r);
 
-  /* We know this server supports ephemeral txnprops.  But if we're
- proxying requests to a master server, we need to see if it
- supports them, too.  */
-  if (version && (! svn_version__at_least(version, 1, 8, 0)))
-return FALSE;
+  /* Unless we are proxying requests to a master server with a declared
+ version number, there is no need to dumb ourselves down. */
+  if (!version)
+return TRUE;
 
-  return TRUE;
+  /* We _are_ proxying requests to another master server with a declared
+ version number.  See if we need to selectively disable some of the
+ capabilities that it doesn't support. */
+  switch (capability)
+{
+  case dav_svn__capability_ephemeral_txnprops:
+return svn_version__at_least(version, 1, 8, 0);
+  case dav_svn__capability_svndiff1:
+  case dav_svn__capability_svndiff2:
+  case dav_svn__capability_put_result_checksum:
+return svn_version__at_least(version, 1, 10, 0);
+  default:
+SVN_ERR_MALFUNCTION_NO_RETURN();
+}
 }
 
 
Index: subversion/mod_dav_svn/version.c
===
--- subversion/mod_dav_svn/version.c(revision 1820731)
+++ subversion/mod_dav_svn/version.c(working copy)
@@ -152,9 +152,6 @@ get_vsn_options(apr_pool_t *p, apr_text_header *ph
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INHERITED_PROPS);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_INLINE_PROPS);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_REVERSE_FILE_REVS);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF1);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_SVNDIFF2);
-  apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_PUT_RESULT_CHECKSUM);
   apr_text_append(p, phdr, SVN_DAV_NS_DAV_SVN_LIST);
   /* Mergeinfo is a special case: here we merely say that the server
* knows how to handle mergeinfo -- whether the repository does too
@@ -210,12 +207,29 @@ get_option(const dav_resource *resource,
   "");
 
   /* If we're allowed (by configuration) to do so, advertise support
- for ephemeral transaction properties. */
-  if (dav_svn__check_ephemeral_txnprops_support(r))
+ for various commit-related 

Re: Removal of spark buildbot

[Gavin McDonald]

> On 10 Jan 2018, at 9:18 pm, Stefan Sperling  wrote:
> 
> On Wed, Jan 10, 2018 at 02:56:51AM +, Philip Martin wrote:
>> Gavin McDonald  writes:
>> 
>>> https://ci.apache.org/builders/serf-sparc64-solaris
>>>  shows pending
>>> jobs since last August and the previous
>>> build that actually ran was July last year.
>>> 
>>> From what I can tell, the buildbot vm has been offline for over 5 months 
>>> now.
>>> 
>>> Unless it will be revived, I intend to remove the node from our
>>> master, and remove the associated jobs from the subversion.conf file
>>> in the next few days.
>> 
>> Yes, do so.  WANdisco did attempt to revive the buildbot after I left
>> but I believe the hardware has failed.
> 
> I am currently setting up a new OpenBSD/sparc64 buildbot.
> I'll send more info once I've got it completely set up.

Good news!

In the meantime this sparc box and config have been removed.

Will be glad to assist in getting the new one attached to our master.

Thanks

Gav…

Re: Removal of spark buildbot

[Stefan Sperling]

On Wed, Jan 10, 2018 at 02:56:51AM +, Philip Martin wrote:
> Gavin McDonald  writes:
> 
> > https://ci.apache.org/builders/serf-sparc64-solaris
> >  shows pending
> > jobs since last August and the previous
> > build that actually ran was July last year.
> >
> > From what I can tell, the buildbot vm has been offline for over 5 months 
> > now.
> >
> > Unless it will be revived, I intend to remove the node from our
> > master, and remove the associated jobs from the subversion.conf file
> > in the next few days.
> 
> Yes, do so.  WANdisco did attempt to revive the buildbot after I left
> but I believe the hardware has failed.

I am currently setting up a new OpenBSD/sparc64 buildbot.
I'll send more info once I've got it completely set up.