🌱 consolidate image layer handling; move fs utils #1690

joelanford · 2025-02-02T20:09:23Z

Description

Now that we have a monorepo, we can begin consolidating various bits of code that had identical or substantially similar implementations in catalogd and operator-controller.

This PR adds an image utility package, for now with common code that handles applying an image to an unpack directory. Of note is that catalogd and operator-controller had separate sets of filters used when applying the image layers. This PR, extracts the separate filter functions into the image utility library, and then updates the (still mostly duplicate) unpacker implementations to use the new shared code.

In follow-up PRs, we can further de-dup the containers/image unpacker implementations (and any other duplicate code we find).

NOTE: To avoid an import cycle, I needed to also make a new fs utility package for internal/rukpak/source/helpers.go (which are all fs-related helpers). I also took the opportunity to move internal/fsutil/* into the new fs` utility package.

Fixes #1711

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-02-02T20:09:38Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`557c03f`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/67a3cfac7628ed0008df262e
😎 Deploy Preview	https://deploy-preview-1690--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2025-02-02T20:22:33Z

Codecov Report

Attention: Patch coverage is 61.45833% with 37 lines in your changes missing coverage. Please review.

Project coverage is 67.58%. Comparing base (46cec30) to head (557c03f).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/util/image/layers.go	57.35%	24 Missing and 5 partials ⚠️
catalogd/internal/source/containers_image.go	54.54%	3 Missing and 2 partials ⚠️
internal/rukpak/source/containers_image.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1690      +/-   ##
==========================================
+ Coverage   67.47%   67.58%   +0.10%     
==========================================
  Files          59       59              
  Lines        5003     4982      -21     
==========================================
- Hits         3376     3367       -9     
+ Misses       1380     1371       -9     
+ Partials      247      244       -3

Flag	Coverage Δ
e2e	`53.08% <41.17%> (-0.30%)`	⬇️
unit	`54.77% <33.33%> (-0.49%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/util/fs/fs.go

perdasilva · 2025-02-03T11:29:44Z

internal/util/fs/fs.go

-	OwnerWritableDirMode  os.FileMode = 0700
-	OwnerReadOnlyFileMode os.FileMode = 0400
-	OwnerReadOnlyDirMode  os.FileMode = 0500
+	ownerWritableFileMode os.FileMode = 0700


I wonder if we should export these and use them in the image utils. Maybe we could even add a comment here to explain why we don't give group perms.

I didn't see any usage outside of this package, so opted to unexport them. If there's a strong use case that comes up later, we can always export them again. This is all internal, so it doesn't matter either way from a public API standpoint, but it can be nice to keep even internal packages nice and tidy to avoid leaky abstractions.

perdasilva · 2025-02-03T11:31:59Z

I'm wondering if we should call

if err := i.deleteOtherImages(bundle.Name, canonicalRef.Digest()); err != nil {
		return nil, fmt.Errorf("error deleting old images: %w", err)
}

On every successful run of unpack. E.g. always ensure that the other images previously unpacked are gone, and we don't have some garbage hanging around because the process got killed mid deletion (though I guess the cache clear at pod restart should handle that...)

joelanford · 2025-02-03T13:30:03Z

/hold

There's a problem we discovered with EnsureEmptyDirectory, when it tries to delete directories with read-only bits set. Need to fix that first, then rebase this.

camilamacedo86 · 2025-02-04T07:48:57Z

internal/util/image/layers.go

+	fsutil "github.com/operator-framework/operator-controller/internal/util/fs"
+)
+
+func ForceOwnershipRWX() archive.Filter {


Could we doc the funcs ?

+1 we should add go docs for these functions.

joelanford · 2025-02-04T13:08:43Z

On every successful run of unpack. E.g. always ensure that the other images previously unpacked are gone, and we don't have some garbage hanging around because the process got killed mid deletion

You're thinking to move it up right after we determine the canonical ref, but before we check to see if we already have the canonical ref cached? I think that makes sense to me. Right now this is only called in the case that we actually pull a new image, but not when we return an existing one.

(though I guess the cache clear at pod restart should handle that...)

Yes, but it's better to keep packages self-contained and avoid relying on hidden side-effects like that. I feel like that is one of OLMv0's downfalls.

azych · 2025-02-05T08:33:04Z

internal/util/image/layers.go

+		return fmt.Errorf("error making unpack directory read-only: %w", err)
+	}
+	return nil
+}


to me all of those wrapper functions above are very specific to the logic required in source and it might be a good idea to defer placing them in an external dedicated util/ package until there is a need to share them, so imho for now it would be better to have them close to where they're being used

nvm, my bad - I noticed they are actually shared between op-con and catalogd ;)

azych

/lgtm

LalatenduMohanty · 2025-02-05T15:46:17Z

.golangci.yaml

@@ -66,6 +66,8 @@ linters-settings:
      alias: ctrl
    - pkg: github.com/blang/semver/v4
      alias: bsemver
+    - pkg: "^github.com/operator-framework/operator-controller/internal/util/([^/]+)$"
+      alias: "${1}util"


It is a good way to enforce import aliasing

LalatenduMohanty · 2025-02-05T20:06:12Z

internal/rukpak/source/containers_image.go

+	} else if errors.Is(err, fsutil.ErrNotDirectory) {
+		if err := fsutil.DeleteReadOnlyRecursive(unpackPath); err != nil {
+			return nil, err
+		}


This seems to be a new code i.e. we are deleting the file if it is not a directory, is that right?

It was originally in IsDirectoryPresent, but and I both @perdasilva thought that was a terrible idea given the name and location of that helper function. So I renamed that function to GetDirectoryModTime and moved the delete call to the caller side.

joelanford · 2025-02-05T20:50:13Z

I'm going to hold off on moving deleteOtherImages. I'm thinking about this scenario:

I'm using image foobar:v1. It's unpacked and working great
I decide I want foobar:v2, so I update my image ref.
Oops, for some reason I can't pull that image (typo, auth problem, etc.)
Set my ref back to foobar:v1. Phew still unpacked so I don't have to pull it again.

Signed-off-by: Joe Lanford <[email protected]>

camilamacedo86

It seems good for me 👍

camilamacedo86 · 2025-02-05T22:52:32Z

/lgtm

joelanford requested a review from a team as a code owner February 2, 2025 20:09

perdasilva reviewed Feb 3, 2025

View reviewed changes

internal/util/fs/fs.go Outdated Show resolved Hide resolved

perdasilva reviewed Feb 3, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2025

joelanford marked this pull request as draft February 3, 2025 13:30

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2025

joelanford force-pushed the consolidate-layer-handling branch 3 times, most recently from 257d9d4 to 29dfafc Compare February 4, 2025 06:13

joelanford marked this pull request as ready for review February 4, 2025 06:14

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2025

joelanford removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2025

camilamacedo86 reviewed Feb 4, 2025

View reviewed changes

azych reviewed Feb 5, 2025

View reviewed changes

azych approved these changes Feb 5, 2025

View reviewed changes

openshift-ci bot assigned azych Feb 5, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2025

LalatenduMohanty mentioned this pull request Feb 5, 2025

Consolidate image layer handling; move fs utils #1711

Closed

LalatenduMohanty reviewed Feb 5, 2025

View reviewed changes

consolidate image layer handling; move fs utils

557c03f

Signed-off-by: Joe Lanford <[email protected]>

joelanford force-pushed the consolidate-layer-handling branch from 29dfafc to 557c03f Compare February 5, 2025 20:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2025

camilamacedo86 approved these changes Feb 5, 2025

View reviewed changes

openshift-ci bot assigned camilamacedo86 Feb 5, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2025

joelanford added this pull request to the merge queue Feb 5, 2025

Merged via the queue into operator-framework:main with commit bf13d14 Feb 6, 2025
21 of 22 checks passed

joelanford mentioned this pull request Feb 7, 2025

🌱 Monorepo pt2: fully consolidate image pull/cache implementations #1731

Merged

4 tasks

joelanford deleted the consolidate-layer-handling branch February 14, 2025 19:43

🌱 consolidate image layer handling; move fs utils #1690

🌱 consolidate image layer handling; move fs utils #1690

Uh oh!

Conversation

joelanford commented Feb 2, 2025 • edited by LalatenduMohanty Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecov bot commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perdasilva commented Feb 3, 2025

Uh oh!

joelanford commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford commented Feb 4, 2025

Uh oh!

azych Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azych left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford commented Feb 5, 2025

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Feb 5, 2025

Uh oh!

Uh oh!

Uh oh!

joelanford commented Feb 2, 2025 •

edited by LalatenduMohanty

Loading

netlify bot commented Feb 2, 2025 •

edited

Loading

codecov bot commented Feb 2, 2025 •

edited

Loading

azych Feb 5, 2025 •

edited

Loading