Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[XrdHTTP] Implement RFC3230 for providing resource digest #769

Merged
merged 8 commits into from
Jul 26, 2018

Conversation

bbockelm
Copy link
Contributor

DAVIX et al uses RFC3230 to implement checksumming over HTTP.

This PR implements the RFC to the point where a gfal-copy works with the ADLER32 checksum algorithm.

Note that this also contains a bugfix in XrdOfs that prevents checksums from working when the underlying filesystem doesn't support fattrs.

@ffurano
Copy link
Contributor

ffurano commented Jul 19, 2018

Hi Brian, this is a nice one. I checked carefully and it seems fine to me. Now I'll compile for all the platforms and do some tests.
I would add this code to line 2399:
m_req_digest.clear();
m_resource_with_digest = "";
just because it's better to have clean variables across requests, it can ease debugging. Would you mind to add it?

Instead I can't fully understand the modification to XrdXrootdXeq. Even if it's tiny I'd prefer Andy to have a look at it. Basically you are not treating ENOTSUP as an error, which seems strange to me. Andy?

@bbockelm
Copy link
Contributor Author

@ffurano - the latest commit addresses your concern.

Indeed, right now, any XrdOfs object that doesn't support extended attributes returns an ENOTSUP. Because that line got an error code, the checksum attempt failed instead of falling back to recalculating the checksum on the fly.

@ffurano
Copy link
Contributor

ffurano commented Jul 19, 2018

Hi, to me it compiles fine in all the relevant platforms (for me :-D ) and I started testing in a totally vanilla server

It seems to work fine, except for md5. I don't think that an md5 should be base64 encoded, at least this is not what I see around.

Example:
$ md5sum /var/spool/xrootd/xrdhttpbase/dynafeds_demo/services6
cbda22bcb41ab0151b438589aa4637e2 /var/spool/xrootd/xrdhttpbase/dynafeds_demo/services6
$ gfal-sum http://littlexrdhttp.cern.ch:1094/dynafeds_demo/services6 md5
http://littlexrdhttp.cern.ch:1094/dynafeds_demo/services6 y9oivLQasBUbQ4WJqkY34g==

The same file checksummed on a DPM, with various protocols:

$ gfal-sum https://dpmhead-trunk.cern.ch/dpm/cern.ch/home/dteam/services6 md5
https://dpmhead-trunk.cern.ch/dpm/cern.ch/home/dteam/services6 cbda22bcb41ab0151b438589aa4637e2
$ gfal-sum root://dpmhead-trunk.cern.ch//dpm/cern.ch/home/dteam/services6 md5
root://dpmhead-trunk.cern.ch//dpm/cern.ch/home/dteam/services6 cbda22bcb41ab0151b438589aa4637e2
$ gfal-sum gsiftp://dpmhead-trunk.cern.ch//dpm/cern.ch/home/dteam/services6 md5
gsiftp://dpmhead-trunk.cern.ch//dpm/cern.ch/home/dteam/services6 cbda22bcb41ab0151b438589aa4637e2

@bbockelm
Copy link
Contributor Author

Hi @ffurano -

Indeed, DPM appears to suffer from the same issue as gfal-copy:

https://its.cern.ch/jira/browse/DMC-1063

To quote the IANA entry for MD5:

The MD5 algorithm, as specified in [RFC1321]. The output of this algorithm is encoded using the base64 encoding [RFC4648].

Brian

@abh3
Copy link
Member

abh3 commented Jul 21, 2018

Speaking of ENOTSUP for check sums. I thought the patch was OK but now I'm not sure. ENOTSUP is returned if the checksum algorithm is not supported as well. So, the patch would have the unintended effect of running into the checksum calculation code where the results will be undefined given that client may have wanted checksum x and the code may compute something but it won't be x (it may not even do anything reasonable at that point).

If the system doesn't support extended attributes then XRootD should not be configured to compute local check sums via the OFS plugin. You would need to supply a script to do so.

see http://xrootd.org/doc/dev47/xrd_config.htm#_Toc483004748

If you did that then this problem wouldn't exist. So, I don't understand what problem is being solved here that couldn't have been solved with the right configuration. So, I can't endorse the change to XrdXrootdXeq.cc at this point.

@bbockelm
Copy link
Contributor Author

Hi Andy,

I don't understand. The code to run a script is not reachable unless one has this ENOTSUP code.

Brian

@abh3
Copy link
Member

abh3 commented Jul 21, 2018

Not at all. In line XrdXrootdXeq.cc:396 a specific check is made whether check sums are allowed to be computed using a local method. If so, a call is made to do that which, if not supported, will fail the request (i.e. one should not configure local computation if it cannot be done). Otherwise, we fall through to check if a script is available to do this (line 400) and if there is one it is invoked. So, if you specified that the checksum is to be computed using a script (or program) then the whole thing works regardless of whether or not extended attributes are supported. If you said you wanted local computation then the default implementation requires that extended attributes be supported, I suspect that the notion was that local computation should happen no matter what. The answer is that if you supplied a checksum manager plug-in then that might be possible (DPM does this). Otherwise, it's a no-no. I agree that the documentation should be clearer on this point. That said, it's hard to cover all the cases since there are way too many implicit variables that control whether or not a checksum can be computed at all.

@abh3
Copy link
Member

abh3 commented Jul 21, 2018

I should add that in the local computation, the default checksum manager can return success (i.e. everything is just fine) but the checksum has not been recorded (line 439). If that is the case, we automatically default to an external script if one has been supplied. This takes care of the case where sites want to compute check sums using a offline process and may or may not wish to have users force a real-time checksum. Let's just say the site requirements were pretty onerous here and that's why the code allows a wide range of options. Now that I further considered this (over a nice French meal) I really think the proposed patch to XrdXrootdXeq.cc should be removed.

@bbockelm
Copy link
Contributor Author

All I'd really like to do is have this configuration line work:

xrootd.chksum max 2 md5 adler32 crc32

on a filesystem that does not support extended attributes, such as an NFS-mounted directory. Are we really saying this is an unsupported configuration? I.e., in the case of an NFS server, a user has to write their own scripts to make a md5 checksum work!?

This reverts commit bcb0f30.

It appears locally-computed checksums are not meant to be supported
on filesystems without extended attributes.  In that case, the
admin write their own scripts for checksum calculation.
@abh3
Copy link
Member

abh3 commented Jul 22, 2018

No, a user need not write anything. The site has to decide whether or not check sums are to be supported for file systems that aren't able to record the check sum value because it potentially implies a lot more of I/O and may be more resource intensive than a site wishes. If a site is OK with check sum re- computation, the above config line would simply be:

xrootd.chksum max 2 md5 adler32 crc32 prog2-compute-checksum

The client has no notion that this is being done but the site specifically allows it by supplying the appropriate program (which, in single checksum cases, may be a two line shell script).

Now, if you are saying the site should be able to compute check sums without storing the result for future queries and avoid writing a small script, then certainly post an enhancement request (i.e. some option on the directive that allows that to happen). The point here is that potentially resource intensive operations should be explicitly enabled.

@bbockelm
Copy link
Contributor Author

I reverted the change to the XrdXrootdXeq a few days back - let's open the discussion about configuring checksums in a separate ticket.

@bbockelm
Copy link
Contributor Author

@abh3 @ffurano - can this go forward? I have the underlying xrootd-hdfs checksum support working, but it's a bit awkward to test that piece without this one.

@ffurano
Copy link
Contributor

ffurano commented Jul 26, 2018

On it now. I have seen the ticket on DMC/Davix/gfal about the md5 encoding, and I'm opening one for DPM too. That's easy to fix, thank you very much for the hint.

@ffurano ffurano merged commit 599d95e into xrootd:master Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants