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.

Sign up for GitHub

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

Jump to bottom

[DownloadBuildArtifactsV0] Add download post-check #14065

Merged
merged 19 commits into from
Feb 2, 2021
Merged

[DownloadBuildArtifactsV0] Add download post-check #14065

merged 19 commits into from
Feb 2, 2021

Conversation

alexander-smolyakov
Copy link
Contributor

@alexander-smolyakov alexander-smolyakov commented Dec 11, 2020 •
edited
Loading

Task name:

  • DownloadBuildArtifactsV0

Description:
This PR implements an additional check of the downloaded artifact' items.

Changes:

  • Added the possibility to check the integrity of the downloaded artifact' items

Additions:

  • Introduced new task optional parameter - Check downloaded files (Can be found in advanced settings)
  • Introduced new task optional parameter - Retry count (Can be found in advanced settings)
  • Added handlerCheckDownloadedFiles function for validation results of artifact download
  • Added isItemCorrupted function for checking information from download ticket of artifact item
  • Added check of the file size in local storage

Refactoring:

  • The logic for download build artifacts was extracted to the download handlers classes
  • Introduced new interfaces to contain all needed information to generate Source and Destination providers
  • All download operations are now executed via the executeWithRetry handler

Others:

  • Bumped up task minor version to 183
  • Added loc strings for messages

UI Demonstration
image

Documentation changes required: Yes

Added unit tests: No

Attached related issue:

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

Aleksandr Smolyakov added 3 commits December 9, 2020 10:33
Add consistency checker
a799ba0 
- Move functions to separate module
- Implement consistency checker
Bump up task version
caa74c0
Polish the source code
ca78ba0 
* Fix code formatting in the download_helper module
* Added comments for the checkArtifactConsistency and isItemCorrupted functions
* Move some debug messages to loc strings
* Add additional checks to the isItemCorrupted function to prevent false alarms
@alexander-smolyakov alexander-smolyakov requested a review from a team December 11, 2020 13:07
@alexander-smolyakov alexander-smolyakov self-assigned this Dec 11, 2020
@MattGal
Copy link
Member

MattGal commented Dec 16, 2020

Can anyone give an ETA as to when this might be merged?

@alexander-smolyakov
Copy link
Contributor Author

Can anyone give an ETA as to when this might be merged?

Hi @MattGal, I'm currently double-checking all changes to prevent any possible regressions for this task, so I believe we will merge these changes within 2-3 workdays.

Aleksandr Smolyakov added 2 commits December 23, 2020 12:23
Revert changes
dab98c3
Resolve review points
f5df3d5 
- Revert refactoring changes
- Introduce a new task option - "Check downloaded files"
- Added check of the file size in local storage
- Improve debug logging
@alexander-smolyakov
Copy link
Contributor Author

Test run of the pipeline with these changes (with Check downloaded files option enabled): https://dev.azure.com/v-alsmo/ArtifactsTest/_build/results?buildId=768&view=results

resolve addressed review points
af9cc4c 
- Fix default value of the option in task.lock.json
- Change strategy of checking download file
- Add commentary to isItemCorrupted function
@MattGal
Copy link
Member

MattGal commented Jan 4, 2021

@alexander-smolyakov any update on when this might get merged?

@MattGal MattGal mentioned this pull request Jan 5, 2021
Closed
@safern
Copy link
Member

safern commented Jan 5, 2021

I see that the only code path that uses execWithRetries is a windows specific code path and when isZipDownloadDisabledBool==true. So this fix will only cause our issues to manifest on a different way by showing an error that the downloaded artifact is corrupt.

Can you add retry for the other code paths so that this task is more resilient?

@anatolybolshakov
Copy link
Contributor

Checked with lightweight artifact (38 b) - I think it make sense to test it with more heavy artifacts also (at least 200 mb)

@hoyosjs
Copy link
Member

hoyosjs commented Jan 27, 2021

@alexander-smolyakov any updates on when this can get merged and deployed? It's hitting pretty often in our CI.

Resolve review points
00164c3 
- Fix linter warnings
- Split download handlers classes to separate files
- Add commentaries to the code
- Update log messages
@alexander-smolyakov
Copy link
Contributor Author

Checked with lightweight artifact (38 b) - I think it make sense to test it with more heavy artifacts also (at least 200 mb)

@anatolybolshakov I tested my changes on heavyweight artifacts items (The summary size of Artifact is 24399 mb).

The build artifact that I used for the test: https://dev.azure.com/v-alsmo/ArtifactsTest/_build/results?buildId=929&view=artifacts&pathAsName=false&type=publishedArtifacts

Run of the test pipeline that I used for e2e testing: https://dev.azure.com/v-alsmo/ArtifactsTest/_build/results?buildId=939&view=results

@alexander-smolyakov
Copy link
Contributor Author

@MattGal @hoyosjs currently we on the testing stage for this PR, we plan to ship this fix with the release for the 183rd sprint. Usually, it requires two or three weeks to deploy the release to all Azure DevOps users. This means that the fix will be available for all users around the 5th of March.

@anatolybolshakov
Copy link
Contributor

anatolybolshakov commented Jan 29, 2021 •
edited
Loading

Could it make sense to expose retry count as input also (in advanced inputs group)?

@alexander-smolyakov
Copy link
Contributor Author

Could it make sense to expose retry count as input also (in advanced inputs group)?

Good point, I will add an additional input for it.
Thanks!

Aleksandr Smolyakov added 2 commits January 29, 2021 16:54
@anatolybolshakov
Copy link
Contributor

Btw tested it with artifact from the same pipeline - LGTM for me 👍

EzzhevNikita
EzzhevNikita approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@EzzhevNikita EzzhevNikita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Changes tested with artifacts of about 500mb, found no issues (pipeline run)

anatolybolshakov
anatolybolshakov approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@anatolybolshakov anatolybolshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@alexander-smolyakov alexander-smolyakov merged commit 44e3baa into master Feb 2, 2021
@alexander-smolyakov alexander-smolyakov mentioned this pull request Feb 2, 2021
Closed
@MattGal MattGal mentioned this pull request Feb 22, 2021
Merged
@SamuelGuine
Copy link

The UI "parallelizationLimit" input seem not used ? Same in previous version 0.178. Only the variable "release.artifact.download.parallellimit" works.

@safern safern mentioned this pull request Mar 8, 2021
Merged
@alexander-smolyakov
Copy link
Contributor Author

The UI "parallelizationLimit" input seem not used ? Same in previous version 0.178. Only the variable "release.artifact.download.parallellimit" works.

Hey @SamuelGuine, we fixed the issue with parallelizationLimit input (Link to related PR). This fix will be available in two or three weeks for all Azure DevOps users.

@hoyosjs hoyosjs mentioned this pull request May 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egor-bryzgalov egor-bryzgalov egor-bryzgalov approved these changes

@EzzhevNikita EzzhevNikita EzzhevNikita approved these changes

@anatolybolshakov anatolybolshakov anatolybolshakov approved these changes

@narula0781 narula0781 Awaiting requested review from narula0781

Assignees

@alexander-smolyakov alexander-smolyakov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@alexander-smolyakov @MattGal @safern @anatolybolshakov @hoyosjs @SamuelGuine @EzzhevNikita @egor-bryzgalov