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: new implementation using unlinked files #1061

Merged
merged 28 commits into from
Oct 12, 2023
Merged

fetcher: new implementation using unlinked files #1061

merged 28 commits into from
Oct 12, 2023

Conversation

hdonnay
Copy link
Member

@hdonnay hdonnay commented Sep 12, 2023 •
edited
Loading

I've tried to do this like 3 times and think I finally hit on a way to only gradually break the API.

Todo:

  • Fix tests
  • Coverage pass
  • Wait for 1.19 to be removed from CI

@hdonnay hdonnay force-pushed the hack/fetcher-ng branch 5 times, most recently from 55cf8b0 to 0c750f5 Compare September 14, 2023 17:41
@codecov
Copy link

codecov bot commented Sep 14, 2023 •
edited
Loading

Codecov Report

Attention: 85 lines in your changes are missing coverage. Please review.

Files Coverage Δ
libindex/metrics.go 100.00% <100.00%> (ø)
rhel/parser.go 75.43% <ø> (ø)
rhel/releases.go 100.00% <ø> (ø)
rhel/repositoryscanner.go 59.53% <ø> (ø)
rhel/rhcc/rhcc.go 90.00% <ø> (ø)
rhel/rhcc/updater.go 69.37% <ø> (ø)
indexer/controller/fetchlayers.go 0.00% <0.00%> (ø)
layer.go 68.27% <62.37%> (-5.58%) ⬇️
libindex/fetcher.go 72.31% <78.30%> (+15.99%) ⬆️

... and 3 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@hdonnay hdonnay force-pushed the hack/fetcher-ng branch 2 times, most recently from 3af7059 to d3bb619 Compare September 14, 2023 18:45
@hdonnay hdonnay added the needs-changelog Label for PRs that need a changelog note. label Sep 14, 2023
@hdonnay
Copy link
Member Author

hdonnay commented Sep 15, 2023

Supersedes and closes #652.

@hdonnay hdonnay force-pushed the hack/fetcher-ng branch 5 times, most recently from eb731bc to b925cbf Compare October 3, 2023 19:05
@hdonnay
Copy link
Member Author

hdonnay commented Oct 3, 2023

I think this is in a place to get reviewed if you can find time. What it does (roughly in order):

  • Move decompression logic to a dedicated package
  • Modify the Layer type and add a LayerDescription; the former has state, the latter doesn't.
  • Add a package to hold API migration helpers and add some helpers.
  • Add a new fetcher interface that uses LayerDescription
  • Implement a test fetcher with the new interface
  • Implement a real fetcher with the new interface and using O_TMPFILE
  • Unify the various ways of getting Layers out of the test hierarchy
  • Update tests

There's still work to be done to move everything over to using the FS method of the Layer, but this is already very large. I can do that in a "stacked" PR, or this one, or later.

@hdonnay hdonnay changed the title fetcher NG fetcher: new implementation using unlinked files Oct 3, 2023
@hdonnay hdonnay marked this pull request as ready for review October 3, 2023 19:34
@hdonnay hdonnay requested a review from a team as a code owner October 3, 2023 19:34
@hdonnay hdonnay requested review from RTann and crozzy and removed request for a team October 3, 2023 19:34
@hdonnay
Copy link
Member Author

hdonnay commented Oct 3, 2023

I can also cherry-pick some the of not strictly related test changes into other PRs if any reviewer would rather.

@hdonnay
Copy link
Member Author

hdonnay commented Oct 9, 2023

Proposed changelog entry:

libindex: move to O_TMPFILE fetcher

This release uses a new fetcher (the component responsible for pulling layers
locally) that makes use of the O_TMPFILE flag to open(2). This ensures that
layer files will be cleaned up even in the event of an unclean shutdown,
including being sent a KILL signal.

@hdonnay hdonnay force-pushed the hack/fetcher-ng branch 2 times, most recently from 1f45682 to cc3cdb3 Compare October 9, 2023 16:46
@hdonnay hdonnay mentioned this pull request Oct 9, 2023
Open
3 tasks
crozzy
crozzy reviewed Oct 11, 2023
View reviewed changes
libindex/fetcher.go Outdated Show resolved Hide resolved
@hdonnay
libindex: implement new unlinked file fetcher
a844586 
This fetcher implementation uses the O_TMPFILE flag to open(2) to open
files that are not linked into the file system. Doing this means that
the layer contents are cleaned up even if the process exits uncleanly.

Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
fetch: add ghcr and optional decompression support
eba5e71 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
fetch: documentation, switch from *testing.T to testing.TB
fe97f8e 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
test: documentation fixes
8fbe967 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
test: remove unused http.RoundTripper implementation
957a02b 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
test: port various other fetcher abstractions to `claircore.LayerDesc…
6f27cfc 
…ription`

There's too many of these things.

Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
claircore: update tests for test package changes
46c8758 
These tests should probably be audited and removed where they don't make
sense anymore.

Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
test: add matcher for LayerDescriptions and Digests
6375527 
This makes gomock a bit nicer to use.

Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
controller: update tests
62508a0 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
whiteout: fix logs in Resolver test
199573a 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
whiteout: update tests
14dca71 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
rpm: update tests
26539f2 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
rhcc: update tests
f32073c 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
rhel: update tests
9e537ff 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
python: update tests
7baf230 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
osrelease: update tests
6d1aa81 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
gobin: update tests
0c22819 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
dpkg: update tests
60bdacd 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
alpine: update tests
62862b0 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
claircore: update tests
19e1375 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
ruby: update tests
694fd4b 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay
periodic: update layer handling
58b6b4a 
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
crozzy
crozzy approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

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

Concerns addressed face to face, LGTM

@hdonnay hdonnay merged commit 58b6b4a into quay:main Oct 12, 2023
8 checks passed
@hdonnay hdonnay deleted the hack/fetcher-ng branch October 12, 2023 22:15
@hdonnay hdonnay removed the needs-changelog Label for PRs that need a changelog note. label Oct 12, 2023
@hdonnay hdonnay mentioned this pull request Oct 13, 2023
Merged
crozzy added a commit to crozzy/clair that referenced this pull request Apr 9, 2024
@crozzy
contrib: remove rms that were needed for previous fetcher
ee9a320 
Since quay/claircore#1061 we should no longer
need the pre-removal of fetch layers.

Signed-off-by: crozzy <joseph.crosland@gmail.com>
crozzy added a commit to crozzy/clair that referenced this pull request Apr 9, 2024
@crozzy
contrib: remove rms that were needed for previous fetcher
f19b59b 
Since quay/claircore#1061 we should no longer
need the pre-removal of fetch layers.

Signed-off-by: crozzy <joseph.crosland@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crozzy crozzy crozzy approved these changes

@RTann RTann Awaiting requested review from RTann RTann is a code owner automatically assigned from quay/clair

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hdonnay @crozzy