Redeploy check logic still looks for the deploy on-disk instead of in cloudfiles

[tobias]

https://github.com/clojars/clojars-web/tree/master/src/clojars/routes/repo.clj#L128

[kahunamoore]

Looking at this one to see if I can help. I've read up on the jclouds API as well as some of the rackspace API docs as well. Is anyone else looking at this?

I don't want to duplicate efforts so please comment here if you are also working on this issue.

@tobias The referenced comment does not indicate whether the existence check should be for "Is CDN accessible?" vs just "Is the file in the blobstore/container?"

Also, more generally, what is the overall strategy for putting files on the CDN? Is it the storage of record or just a mirror/cache? Some of the clojars code seems to treat the CND as a mirror instead of as the only storage for uploaded jars. Maybe I'm reading the code wrong but this calls seems to upload the file to the clojar's server local storage and then, as a post processing step, uploads it to cloudfiles.

https://github.com/clojars/clojars-web/blob/master/src/clojars/routes/repo.clj#L275

I'm guessing this is just an intermediate phase until the final switch is thrown. It might help to create a protocol API for the storage and then have two implementations, one for the local storage and one for the CDN with each being unit tested for proper operation. You've probably already discussed or documented this somewhere so a link to clarifying info would help me avoid asking more stupid questions.

[kahunamoore]

Ok, looks like I missed this announcement:

"... deploys are now written to Rackspace Cloudfiles in addition to the on-disk repo. This is a step in the long journey to having the repo served by more resilient infrastructure."

I'll check the mailing list before posting comments here... my bad.

[tobias]

@kahunamoore - thanks for helping out!

Yes, the goal is to eventually have Cloudfiles be the storage of record, and have all user reads hit a CDN that is pointed at the Cloudfiles container. We're still working with a CDN provider to get sponsorship (we can't use the Rackspace CDN due to SSL certificate limitations there which impact our ability to support Java 6 clients). Once that happens, we'll get it setup and start beta testing the CDN.

The check here is to see if this non-SNAPSHOT version has already been deployed, so we would want to check that it is in the Cloudfiles container - it's safe to assume that if it is there, it is accessible via the CDN. The check itself is simple, it should be enough to call clojars.cloudfiles/artifact-exists?, looking for the pom file for that version, and would require passing the Cloudfiles Connection to assert-non-redeploy. However, this change may break tests that setup the repo by moving files around on disk instead of deploying them, since the tests use an in-memory Cloudfiles representation that only gets populated via deploys.

I think for now I'd like to see this implemented as an assertion that backs up the filesystem-based check until we are ready to switch over to cloudfiles fully. So the check would be: see if the version exists on disk. If it does, see if it exists in cloudfiles. If that latter check fails, report it as an error (like we do here: https://github.com/clojars/clojars-web/blob/master/src/clojars/routes/repo.clj#L212), but still deny the user's redeploy. This allows us to get a bit more confidence in our cloudfiles setup in production without relying on it yet.

As for a protocol for writing to disk/cloudfiles, I'm not sure that's worth the effort, as we're hoping to move away from the on-disk repo.

And re: docs - it's safer to assume that something isn't documented. That's something I need to be better about.

[kahunamoore]

Working my way through it. Pretty straightforward so far. I hope I'm not making more work for you answering questions... once I'm up to speed with the existing clojars conventions, code organization, error reporting, testing, etc. it won't be so bad.

Q: Is there an existing function that takes the group-id, artifact-id, version, file and/or path (as used in the clojars.routes.repo namespace) into a cloudfiles {path,file} url? I wrote one I think is ok but would rather use an existing one.

The similar code that calls cf/put-file generates a path based on the local filesystem like so:

path (fu/subpath (.getAbsolutePath from-dir) (.getAbsolutePath file))

This won't work if we eventually eliminate storing the file locally (modulo any temporary files created during the push to cloudfiles, if any) unless we can be assured the above code will always map directly into a valid cloudfiles url.

Second, I'm looking at the best way to structure the error reporting you mentioned and see the report-error call I need to make. The assert-non-redeploy fn already throws-invalid if all of the asserted conditions are true but I don't see anything catching the exception nor calling report-error but maybe I'm missing something.

I have committed my current changes (does not compile) to my fork for your review:

coopsource@805fe02

There is a cleanup, error reporting, tests to do still but I just want to confirm that I'm working in the right general direction. I know I put one liners into functions but I find it easier to read code written in that style and can revert to inline code instead. Comments, suggestions welcome.

Re: protocol suggestion... agreed, nevermind.

Alan

[tobias]

On Wed, Jun 22, 2016 at 3:35 AM, Alan Moore [email protected]
wrote:

Working my way through it. Pretty straightforward so far. I hope I'm not
making more work for you answering questions... once I'm up to speed with
the existing clojars conventions, code organization, error reporting,
testing, etc. it won't be so bad.

I'm happy to answer any questions, no problem at all. However, answers may
be delayed depending on how busy I am with $day_job.

Q: Is there an existing function that takes the group-id, artifact-id,
version, file and/or path (as used in the clojars.routes.repo namespace)
into a cloudfiles {path,file} url? I wrote one I think is ok but would
rather use an existing one.

I don't think there is one currently, but it would be nice to have such a
function. It should probably live in clojars.file-utils or in
clojars.cloudfiles, depending on how narrow its use is.

The similar code that calls cf/put-file generates a path based on the
local filesystem like so:

path (fu/subpath (.getAbsolutePath from-dir) (.getAbsolutePath file))

This won't work if we eventually eliminate storing the file locally
(modulo any temporary files created during the push to cloudfiles, if any)
unless we can be assured the above code will always map directly into a
valid cloudfiles url.

We'll always have a per-deployment staging repo on disk, since we use that
for atomic deploys, but, for checking for existence given a group-id,
artifact-id, and version, you are correct, this function won't work.

Second, I'm looking at the best way to structure the error reporting you
mentioned and see the report-error call I need to make. The
assert-non-redeploy fn already throws-invalid if all of the asserted
conditions are true but I don't see anything catching the exception nor
calling report-error but maybe I'm missing something.

throw-invalid is used to communicate validation errors to the user - we
have ring middleware that catches that and turns it into the appropriate
response here:
https://github.com/clojars/clojars-web/tree/master/src/clojars/routes/repo.clj#L330.
It sets :report? false on the exception, which prevents it from being
reported to Yeller.

The primary intent of this check is to replace the on-disk check for when
we are using only cloudfiles for the repo, so, until we are at that point,
this check should do nothing user visible, and only report to us via Yeller
(directly via report-error) when the assertion fails (i.e. the artifact
exists on disk, but doesn't exist in cloudfiles). And this is to confirm
that the code is working, and to give us another checkpoint to confirm that
our cloudfiles upload process worked (at least for the artifact in
question).

I have committed my current changes (does not compile) to my fork for your

review:

coopsource/clojars-web@805fe02
coopsource@805fe02

There is a cleanup, error reporting, tests to do still but I just want to
confirm that I'm working in the right general direction. I know I put one
liners into functions but I find it easier to read code written in that
style and can revert to inline code instead. Comments, suggestions welcome.

This looks good so far, modulo my comments above. I like the small,
descriptive functions.

[kahunamoore]

Ok, further along. I've been able to run some of the tests and none of them seemed to have failed. I also deployed a test project to my local clojars-web server and it seemed like it did the right thing. It did not fail for the user regardless of what the state of the cloudfiles deployment was in and reports to the log (stdout during dev) when I force the state manually. However, I do not know if my generated cloudfiles URL is correct and cannot verify the call to artifact-exists? (e.g. jc/blob-exists?) since I don't have any of the rackspace usernames/passwords/containers, etc.)

I will push my changes out to my fork on github so you can have a look at the current state of the code/review. When that is done (by tomorrow mid-day) I'll post a link here.

[tobias]

Thanks!

Yes, local dev that needs cloudfiles isn't currently possible if you aren't one of the admins, unfortunately. I just created an issue to solve that (#544), and will try to address that soon.

The user-cannot-redeploy test should be exercising your code (https://github.com/clojars/clojars-web/tree/master/test/clojars/test/integration/uploads.clj#L176), since the first deploy will write to the test on-disk repo and to the in-memory cloudfiles, though there isn't a good way currently to assert that the check against cloudfiles is proper.

[kahunamoore]

Ok, sorry about that. Wife fell ill and a bunch of other stuff came up. I'm back on this.

[tobias]

No worries, all things Clojars move pretty slowly anyway :)

[kahunamoore]

And I'm back... sorry about that. The day job had a big tradeshow we had to prepare for. I'm going to take a look at this over the weekend. What has changed that would impact the original requirements or design consideration? I'll try to read through the changes and recently closed issues to see if I need to change how this will work... Thoughts? Comments?

BTW - thanks for the hard work getting clojars moved to rackspace. Let me know if there are other higher priority issues you'd rather I work on.

[tobias]

@kahunamoore howdy!

I've done quite a bit of work on the CDN/CloudFiles features on another branch (https://github.com/clojars/clojars-web/tree/storage-protocol) which include changes that obviate the need for this issue. I was going to comment and close once I merged to master. Sorry for pulling the rug out from under you :(

If you are still interested in helping with other issues, #540, #547, and #558 are probably fairly straightforward, and #559 is time-sensitve, since Yeller shuts down on Nov. 20. Feel free to take any of those (or pretty much anything anything else tagged ready if another one of those strikes your fancy).

[kahunamoore]

No worries. I wasn't happy with my changes anyway and it was hard not being able to test the code due to permissions issues. I'm happy to take a look at those other issues. Like you said, #559 sounds the most urgent so I'll be taking a look at that one now.

Thanks for all your help!

[tobias]

Closing since the storage protocol changes are now on master.