Skip to content

simplify download server #307

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
Closed
rphillips wants to merge 1 commit into from
Closed

simplify download server #307

rphillips wants to merge 1 commit into from

Conversation

@rphillips
Copy link

@rphillips rphillips commented Oct 3, 2019 •
edited
Loading

Instead of maintaining a custom threaded http server, this PR migrates the downloader to use Python's included threaded http server.

This PR also captures sigterm, which was already merged into master.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 3, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 3, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rphillips
To complete the pull request process, please assign spadgett
You can assign the PR to them by writing /assign @spadgett in a comment when ready.

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

Details 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

@rphillips rphillips force-pushed the fixes/fixup_download_server branch from 0edcfa4 to e003418 Compare October 3, 2019 20:32
@spadgett
Copy link
Member

spadgett commented Oct 3, 2019

/cc @wking

@rphillips rphillips force-pushed the fixes/fixup_download_server branch from e003418 to 365ec36 Compare October 3, 2019 20:58
wking
wking reviewed Oct 3, 2019
View reviewed changes
wking
wking reviewed Oct 3, 2019
View reviewed changes
@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Oct 4, 2019

/hold

for review comments

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2019
@rphillips rphillips force-pushed the fixes/fixup_download_server branch from 365ec36 to 1418c24 Compare October 4, 2019 17:08
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2019
@rphillips rphillips force-pushed the fixes/fixup_download_server branch from 1418c24 to 4e21daa Compare October 4, 2019 17:12
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2019
@rphillips rphillips changed the title simplify download server and use serve_forever() simplify download server Oct 4, 2019
@rphillips
Copy link
Author

rphillips commented Oct 4, 2019

/retest

@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Oct 4, 2019

@rphillips please also clarify the what & why of the PR in the description. Thanks!

wking
wking reviewed Oct 4, 2019
View reviewed changes
wking
wking reviewed Oct 4, 2019
View reviewed changes
manifests/07-downloads-deployment.yaml
from SocketServer import ThreadingMixIn
import SimpleHTTPServer, os, re, signal, sys, tarfile, tempfile, zipfile
class ThreadedHTTPServer(ThreadingMixIn, HTTPServer):
Copy link
Member

@wking wking Oct 4, 2019

Choose a reason for hiding this comment

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

I added the bulk of this script in #177. As discussed there, it was largely a copy-paste from ci-operator. @smarterclayton added that code on 2018-06-25 in openshift/ci-operator#35. I'm not familiar enough with its source to know what it's all about, but he links to https://stackoverflow.com/questions/46210672/ below and that says:

No, I don't want a hack such as ThreadingMixIn that merely collects up the response and returns it as a unit.

So I expect he was aware of this approach and has consciously decided against it. But maybe there have been stdlib improvements in 2.7 since that SO post? I haven't hunted deeply enough to know.

Copy link
Author

@rphillips rphillips Oct 8, 2019

Choose a reason for hiding this comment

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

I don't understand the original motivation... The main thread accepts the client and uses a thread to handle the I/O of the client. There is not a streaming interface with the current code.

Copy link
Contributor

@benjaminapetersen benjaminapetersen Oct 8, 2019

Choose a reason for hiding this comment

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

I am hesitant to do a ton with this w/o test coverage.
We will have an e2e test coming soon with #306, probably, which will indirectly help ensure this component doesn't fall over.

@rphillips rphillips force-pushed the fixes/fixup_download_server branch from 4e21daa to 63bb075 Compare October 8, 2019 01:02
@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Oct 22, 2019

/close

I'm going to close this as I'm not sure we have a good reason for the change. Feel free to reopen if we want to pick up the conversation again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjaminapetersen benjaminapetersen Awaiting requested review from benjaminapetersen

@jhadvig jhadvig Awaiting requested review from jhadvig

1 more reviewer

@wking wking wking left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rphillips @openshift-ci-robot @spadgett @benjaminapetersen @wking