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

Retry downloads, respect URL list, validate tar hash #74100

Merged
merged 1 commit into from
Feb 20, 2019

Conversation

mtaufen
Copy link
Contributor

@mtaufen mtaufen commented Feb 15, 2019

I tested this with the instructions in the README: https://github.com/pjh/kubernetes/blob/gce-windows-cluster/cluster/gce/win1803/README-GCE-Windows-kube-up.md

What type of PR is this?
/kind bug

What this PR does / why we need it:

  1. Respects that NODE_BINARY_TAR_URL may be a list of URLs, instead of just one URL.
  2. Improves network disruption tolerance by retrying downloads (similar to download-or-bust in Linux scripts).
  3. Validates NODE_BINARY_TAR_HASH against downloaded files, if it is provided.
NONE

@yujuhong @pjh

@mtaufen mtaufen added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. area/node-lifecycle Issues or PRs related to Node lifecycle labels Feb 15, 2019
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Feb 15, 2019
}
}

# Attempts to downlaod the file from URLs, trying each URL in succession (with retries)
Copy link

Choose a reason for hiding this comment

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

s/downlaod/download

@mtaufen
Copy link
Contributor Author

mtaufen commented Feb 19, 2019

/test ci-kubernetes-e2e-windows-gce

@mtaufen mtaufen force-pushed the file-download-improvements branch 2 times, most recently from bf6f0af to 9e1bd0e Compare February 20, 2019 00:45
@mtaufen mtaufen changed the title WIP Retry downloads, respect URL list, validate tar hash Retry downloads, respect URL list, validate tar hash Feb 20, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2019
@mtaufen
Copy link
Contributor Author

mtaufen commented Feb 20, 2019

@pjh @yujuhong ready for review

Copy link
Contributor

@pjh pjh left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@@ -86,5 +86,69 @@ function Get-InstanceMetadataValue {
}
}

function Validate-Hash {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment that the hash is SHA1, or change the function name to indicate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


# Attempts to download the file from URLs, trying each URL in succession (with retries)
# until it succeeds. It will loop through the URLs list forever until it has a success.
# If successful, it will write the file to OutDir, with a
Copy link
Contributor

Choose a reason for hiding this comment

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

OutFile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if ($Hash -ne $null) {
Try {
Validate-Hash -Hash $Hash -Path $OutFile
} Catch {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a couple extra leading spaces here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if ($kube_env.ContainsKey('NODE_BINARY_TAR_HASH')) {
$hash = ${kube_env}['NODE_BINARY_TAR_HASH']
}
MustDownload-File -Hash $hash -OutFile ${tmp_dir}\${filename} -URLs $urls
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change other Invoke-WebRequest calls in this file to use MustDownload-File?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

# Attempts to download the file from URLs, trying each URL in succession (with retries)
# until it succeeds. It will loop through the URLs list forever until it has a success.
# If successful, it will write the file to OutDir, with a
# filename matching the base (final) segment of the URL. You can optionall provide a Hash
Copy link
Contributor

Choose a reason for hiding this comment

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

optionally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


# TODO(mtaufen): We could probably set this in a more general place.
# Disable progress bar to increase download speed.
$ProgressPreference = 'SilentlyContinue'
Copy link
Contributor

Choose a reason for hiding this comment

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

You could move this to Set-PrerequisiteOptions in k8s-node-setup.psm1, or (perhaps better) just move this to be "global" at the top of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to top of file.

@yujuhong
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtaufen, yujuhong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2019
@k8s-ci-robot k8s-ci-robot merged commit f04ce29 into kubernetes:master Feb 20, 2019
@mtaufen
Copy link
Contributor Author

mtaufen commented Feb 21, 2019

Looks like this had a small bug, fix in #74324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/node-lifecycle Issues or PRs related to Node lifecycle cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants