Skip to content

WIP- vendor containers/{image,storage} #1445

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

WIP- vendor containers/{image,storage} #1445

vrothberg wants to merge 1 commit into from

Conversation

@vrothberg
Copy link
Member

@vrothberg vrothberg commented Mar 26, 2019

Avoid redundantly pulling the same image (or layer).

Signed-off-by: Valentin Rothberg rothberg@redhat.com

@vrothberg
Copy link
Member Author

vrothberg commented Mar 26, 2019

Do not merge this PR, it's used to increase confidence for the changes to storage and image by letting it pass Buildah's CI.

@vrothberg
Copy link
Member Author

vrothberg commented Mar 26, 2019

This PR needs to be updated. We also need to merge #1400 first to make it pass.

@rh-atomic-bot
Copy link
Collaborator

rh-atomic-bot commented Mar 26, 2019

☔ The latest upstream changes (presumably 3d74031) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Mar 27, 2019

@vrothberg #1400 Is merged can you rework this one?

@vrothberg
Copy link
Member Author

vrothberg commented Mar 27, 2019

@vrothberg #1400 Is merged can you rework this one?

Sure. Already rebased and pushed.

@rhatdan
Copy link
Member

rhatdan commented Mar 27, 2019

Can you remove [WIP]?
LGTM

@vrothberg
Copy link
Member Author

vrothberg commented Mar 27, 2019

Can you remove [WIP]?

Not until the PRs over image, storage and skopeo are merged. This PR is testing the new copy-detection mechanism.

I can open a dedicated PR to vendor in the latest image/storage.

@vrothberg
Copy link
Member Author

vrothberg commented Mar 27, 2019

All tests are hitting a timeout. I'll vendor the storage changes first to exclude some potential error sources from image.

@vrothberg vrothberg force-pushed the copy-detection branch 7 times, most recently from e306618 to 6ee4f15 Compare March 28, 2019 12:44
@vrothberg vrothberg mentioned this pull request Mar 28, 2019
Merged
@vrothberg
Copy link
Member Author

vrothberg commented Mar 29, 2019

@mtrmac @nalind @TomSweeneyRedHat the integration tests don't seem to fire off as the last logs are calling tests/test_runner.sh and do not show any log afterward. containers/podman#2763 over at libpod is passing now. I try to wrap my head around it I fail to reproduce.

@vrothberg
Copy link
Member Author

vrothberg commented Mar 29, 2019

It ultimately suggests a deadlock but I don't see how.

@vrothberg
Copy link
Member Author

vrothberg commented Mar 29, 2019

Good news before the weekend. No deadlock and tests are passing now. I took a shot a travis_wait which is famous for suppressing output when a timeout is being hit.

I don't yet fully understand WHY we hit the timeout but removing travis_wait let's us pass again. One really annoying thing about travis_wait is that it does not print any log at the time out which complicates debugging.

I've already checked with @TomSweeneyRedHat who wants a head nod from @nalind to make sure that removing travis_wait does not introduce any undesired side-effect.

mtrmac
mtrmac reviewed Apr 1, 2019
View reviewed changes
pkg/blobcache/blobcache.go
}

func (s *blobCacheDestination) SupportsBlobLocks() bool {
return false
Copy link
Contributor

@mtrmac mtrmac Apr 1, 2019

Choose a reason for hiding this comment

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

(Note that the cache object can wrap a storageImageDestination in Commit. I don’t quite think that it matters — we are only actually creating some new layers, so there should not be any concurrent downloads of that.)

@vrothberg
vendor containers/{image,storage}
8b8746f 
Avoid redundantly pulling the same image (or layer).

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg vrothberg mentioned this pull request Apr 2, 2019
Closed
@rh-atomic-bot
Copy link
Collaborator

rh-atomic-bot commented Apr 14, 2019

☔ The latest upstream changes (presumably fcc12bd) made this pull request unmergeable. Please resolve the merge conflicts.

@vrothberg
Copy link
Member Author

vrothberg commented Apr 18, 2019 •
edited
Loading

Closing in favour of containers/image#611.

@vrothberg vrothberg closed this Apr 18, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mtrmac mtrmac mtrmac left review comments

Assignees

No one assigned

Labels

locked - please file new issue/PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vrothberg @rh-atomic-bot @rhatdan @mtrmac