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

fetcher/soup: Drop outdated max queue size assertion #1453

Closed
wants to merge 1 commit into from
Closed

fetcher/soup: Drop outdated max queue size assertion #1453

wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

Since f4d1334 the primary pull code maintains a
maximum queue. In that commit message I said Note that I kept an assertion..
But I think this is wrong since while it covers a lot of the normal cases, if
one is e.g. trying to fetch a ton of refs, the primary pull code doesn't yet
queue those. While it'd be nice to queue those, it isn't worth carrying
an extra assertion just in the libsoup side.

Closes: #1451

@cgwalters
Copy link
Member Author

@rh-atomic-bot delegate=dbnicholson

@rh-atomic-bot
Copy link

✌️ @dbnicholson can now approve this pull request

dbnicholson
dbnicholson reviewed Feb 14, 2018
View reviewed changes
src/libostree/ostree-fetcher-soup.c
@@ -386,8 +385,6 @@ start_pending_request (ThreadClosure *thread_closure,
OstreeFetcherPendingURI *pending;
GCancellable *cancellable;

g_assert_cmpint (g_hash_table_size (thread_closure->outstanding), <, thread_closure->max_outstanding);
Copy link
Member

Choose a reason for hiding this comment

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

While I'm generally happy to remove an assert that the code is not doing anything to prevent, note that the curl fetcher has a similarly hardcoded assertion.

I'm also curious why the fetcher is getting to 24 requests when the pull queue code is supposed to stop at 8. Oh, I think I might see it. Most fetches check fetcher_queue_is_full before calling start_fetch or start_fetch_deltapart.

However, all the initial requests are started with initiate_request. In that function, if no delta is found then you go into queue_scan_one_metadata_object, which eventually checks fetcher_queue_is_full. But if a delta is found, then you go to initiate_delta_request, which calls _ostree_fetcher_request_to_membuf without checking that the fetcher queue is full.

So, I think you can hit this problem if you pull more than 24 deltas. The solution would appear to be that a queue is needed for superblocks.

Copy link
Member Author

@cgwalters cgwalters Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I updated it ⬇️ to drop the assertion from libcurl too. You're right about the code chain involved here...I think it's fixable, we'd need to maintain integer offsets as iterators over the bits that call initiate_request(), but doing too many HTTP requests is better than crashing for now.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I think you could refactor a bit with the existing scheme using a pending_fetch_superblock set and start_superblock_fetch utility function that does the actual _ostree_fetcher_request_to_membuf call. Then you'd just check fetcher_queue_is_full in initiate_delta_request and either add to the set or call start_superblock_fetch.

@cgwalters
fetcher: Drop max queue size assertion in libsoup/libcurl backends
0953ce0 
Since f4d1334 the primary pull code maintains a
maximum queue. In that commit message I said `Note that I kept an assertion.`.
But I think this is wrong since while it covers a lot of the normal cases, if
one is e.g. trying to fetch a ton of refs, the primary pull code doesn't yet
queue those.  While it'd be nice to queue those, it isn't worth carrying
extra assertions in the backends that can still trigger.

Closes: ostreedev#1451
@dbnicholson
Copy link
Member

@rh-atomic-bot r+ 0953ce0

@rh-atomic-bot
Copy link

⚡ Test exempted: merge already tested.

@dbnicholson dbnicholson mentioned this pull request Feb 15, 2018
Closed
pwithnall added a commit to pwithnall/ostree that referenced this pull request May 28, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
b34c2f8 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
pwithnall added a commit to pwithnall/ostree that referenced this pull request May 28, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
a9ec7a7 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
pwithnall added a commit to pwithnall/ostree that referenced this pull request May 28, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
3b8b95a 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
pwithnall added a commit to pwithnall/ostree that referenced this pull request May 29, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
1520222 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
pwithnall added a commit to pwithnall/ostree that referenced this pull request May 30, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
f5a2013 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
pwithnall added a commit to pwithnall/ostree that referenced this pull request May 30, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
82215d3 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
pwithnall added a commit to endlessm/ostree that referenced this pull request May 30, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
3f0bfb8 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev/ostree#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>
rh-atomic-bot pushed a commit that referenced this pull request May 30, 2018
@pwithnall @rh-atomic-bot
lib/repo-pull: Support queuing delta superblock requests
f342e66 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>

Closes: #1600
Approved by: jlebon
pwithnall added a commit to endlessm/ostree that referenced this pull request Jun 4, 2018
@pwithnall
lib/repo-pull: Support queuing delta superblock requests
28c1882 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev/ostree#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>

(eos3.3 backport: Rework changes in initiate_request() due to changes in
how static deltas are prioritised; drop use of
OSTREE_FETCHER_REQUEST_OPTIONAL_CONTENT.)

https://phabricator.endlessm.com/T22555
ramcq pushed a commit to endlessm/ostree that referenced this pull request Sep 6, 2018
@pwithnall @ramcq
lib/repo-pull: Support queuing delta superblock requests
d31d82e 
Just like all the other requests made for delta parts and objects by the
pull code, use a queue for delta superblocks. Currently this doesn’t do
any prioritisation or retries after transient failures, but it could do
in future.

This means that delta superblocks are now subject to the parallel
request limit in the fetcher, which was a problem highlighted here:
ostreedev/ostree#1453 (comment).

Signed-off-by: Philip Withnall <[email protected]>

Closes: #1600
Approved by: jlebon

(cherry picked from commit f342e66)
Signed-off-by: Robert McQueen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbnicholson dbnicholson dbnicholson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cgwalters @rh-atomic-bot @dbnicholson