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

Fix corrupted packages with missing conanmanifest.txt files #4662

Merged
merged 22 commits into from Mar 25, 2019

Conversation

Projects
None yet
5 participants
@danimtb
Copy link
Member

commented Mar 5, 2019

Changelog: Fix: Fix corrupted packages with missing conanmanifest.txt files
Docs: omit

Close #4698
@revisions: 1

We found an issue on Bintray about missing files in a package, in that case only the conaninfo.txt file was uploaded out of the conan_package.tgz and conanmanifest.txt.

In that case the problematic comes when you want to upload a new package as ir fais becaouse of the following:

  • Conan checks in the package upload that the package exist requesting the SNAPSHOT and only checking if there is something (in this case it is true, the conaninfo.txt is there)
  • Then it tries to get the digest of the package to perform the diff (in this case it fails as there is no conanmanifest.txt)
  • The conan_package.tgz is never uploaded as the client fails in the previous step.

To mitigate this I propose a behavior that is already being done in the recipe upload: Check that the manifest is in the snapshot (actually in the recipe upload it checks the availability of the digest and if not there -404- it assusmes there is no package uploaded).

I the digest is for some reason not available it assumes there is no existing package uploaded and it uploads everything.


Another useful change to mitigate this (not proposed at the time of opening this PR) is changing the order of the files upload. Now the files are uploaded in this order: conanmanifest.txt --> conaninfo.txt --> conan_package.tgz and it is done intetionally this way for some reason

def _download_files(self, file_urls, quiet=False):

From my point of view, the most problematic file during the upload is the conan_package.tgz and it should be uploaded in first place, then the conaninfo.txt and finally the conanmanifest.txt to wrap up the complete upload.

@ghost ghost assigned danimtb Mar 5, 2019

@ghost ghost added the stage: review label Mar 5, 2019

@danimtb danimtb modified the milestone: 1.14 Mar 5, 2019

@danimtb danimtb changed the title Feature/package upload WIP: Fix corrupted packages with missing conanmanifest.txt files Mar 5, 2019

@danimtb danimtb added the whiteboard label Mar 5, 2019

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

Included some test to check also the search and install commands

@SSE4

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

I don't think changing the order helps here. upload has to be transnational operation - conan either uploads everything or nothing. if it's interrupted during the upload (e.g. Ctrl-C pressed, conan crashed, network error occurred or whatever else happened), it should never finalize the transaction and files that were already uploaded or partially uploaded by that time should be discarded.

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

That is the best solution but the upload is not done in one transaction but file by file and it seems that cannot be changed in the server side. I know this fix does not fix the issue but it tries to minimize it as much as we can

@SSE4

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

still better than nothing. if it reduces the rate of problem occurrence, I fully support applying it.
however, that seems to be a design problem, which has to be solved may be in the next version of conan REST API, if it's ever planned.

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

So finally I think I have a bunch of interesting tests here bringing some additional information to the original situation like the one described in #4672

To me, this is something that it is worth to have consistent behavior and probably a naive solution would be to check in install/download and upload of packages that the 3 files that define a package are correctly in place, although that would mean that if in the future we decide to remove one of those files the old clients will be broken (although this is something that would require a 2.0 or equivalent).

Having all this into account I would like to know your opinion or at least bring this discussion to one of our whiteboard sessions @conan-io/barbarians

PS: Don't worry about the prints in tests, those are just a proof of concept to get the idea. I will rework them when we agree on a solution 😜

Show resolved Hide resolved conans/client/remote_manager.py Outdated
Show resolved Hide resolved conans/client/rest/rest_client_v1.py
Show resolved Hide resolved conans/client/rest/rest_client_v2.py
Show resolved Hide resolved conans/test/functional/corrupted_packages_test.py Outdated
@@ -234,7 +234,7 @@ def reuse_test(self):
self.client.run("install Hello0/1.0@lasote/stable --build")
self.client.run("upload Hello0/1.0@lasote/stable --all")

client2 = TestClient(servers=self.servers, users={"default": [("lasote", "mypass")]})
client2 = TestClient(servers=self.servers, users={"myremote": [("lasote", "mypass")]})

This comment has been minimized.

Copy link
@danimtb

danimtb Mar 15, 2019

Author Member

This test was failing before at this point https://github.com/conan-io/conan/pull/4662/files#diff-9e273c58f528ec321db5954d73096c3aR133 due to bad credentials for the remote. I am not sure if this PR is changing any behavior or the test was wrong 😕

@danimtb danimtb assigned lasote and unassigned danimtb Mar 15, 2019

@danimtb danimtb changed the title WIP: Fix corrupted packages with missing conanmanifest.txt files Fix corrupted packages with missing conanmanifest.txt files Mar 15, 2019

@danimtb danimtb requested review from memsharded, lasote and jgsogo Mar 15, 2019

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Added the changes to improve the integrity of packages in the complete loop of upload-search-install-upload.

The search couldn't be improved as the values returned by the server are the conaninfo content directly, so not much we can do.

Please take note that this makes an extra API call to check the snapshot in the package installation (download) from the server

@ghost ghost assigned danimtb Mar 18, 2019

danimtb added some commits Mar 18, 2019

Show resolved Hide resolved conans/client/remote_manager.py Outdated
@jgsogo

jgsogo approved these changes Mar 18, 2019

Copy link
Member

left a comment

LGTM, this will help the user to cope with some errors

Show resolved Hide resolved conans/client/remote_manager.py Outdated
Show resolved Hide resolved conans/client/cmd/uploader.py
Show resolved Hide resolved conans/client/remote_manager.py Outdated

@danimtb danimtb requested a review from lasote Mar 21, 2019

Show resolved Hide resolved conans/client/remote_manager.py Outdated

@danimtb danimtb removed their assignment Mar 22, 2019

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

PR is ready. Please take a look so it can be merged

@lasote

lasote approved these changes Mar 25, 2019

@lasote lasote merged commit f93b8e2 into conan-io:develop Mar 25, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.