[rm nixpkgs] make HEAD request to BinaryCache to ensure binary is cached #1318

savil · 2023-07-27T00:45:22Z

Summary

This PR makes a HTTP HEAD request to https://cache.nixos.org/<hash>.narinfo to verify that the binary is indeed cached.

If not, we fallback to the legacy path that is dependent on downloading nixpkgs.

Implementation note
In IsInBinaryCache, I wanted to insert the HEAD request. However, this function can be called in a loop for all packages, which is bad for perf.

Instead, in this design, a new function FillNarInfoCache(packageList) makes the HEAD requests concurrently, storing results in an in-memory cache.

We require that callers invoke FillNarInfoCache(packageList) prior to invoking IsInBinaryCache. Failing this, IsInBinaryCache will return an error. Not thrilled with this design, since it may lead to inadvertent errors, but this code is pretty core and our test coverage should expose any codepaths that call IsInBinaryCache without FillNarInfoCache.

How was it tested?

I could do devbox shell in a project that has hello package. I inserted some print statements to verify that the HEAD
response was 200 and that the caching for the boolean value of isNarInfoInCache was working.

gcurtis · 2023-07-31T18:55:43Z

internal/devpkg/package.go

+
+	// Check if the narinfo is present in the binary cache
+	if exists, ok := isNarInfoInCache[p.Raw]; ok {
+		fmt.Printf("narInfo cache hit: %v\n", exists)


Should the Printfs be debug logs instead? I'm guessing we want this to be a transparent thing for the user.

oh I'll be deleting the Printfs prior to landing! Just have them there to ensure this code works as I want. (this entire section is also gated by the feature-flag)

I'll remove this after I test the final version of this PR on devbox-cloud

internal/devpkg/package.go

mikeland73

I'm afraid this approach will introduce a perf regression which scales O(n) with number of packages. Thoughts on alternatives in comment?

savil · 2023-08-07T21:03:59Z

needs revision so marking as Draft.

savil · 2023-08-15T00:29:22Z

Current dependencies on/for this PR:

main
- PR [rm nixpkgs] make HEAD request to BinaryCache to ensure binary is cached #1318 👈

This comment was auto-generated by Graphite.

gcurtis

We require that callers invoke FillNarInfoCache(packageList) prior to invoking IsInBinaryCache. Failing this, IsInBinaryCache will return an error. Not thrilled with this design, since it may lead to inadvertent errors, but this code is pretty core and our test coverage should expose any codepaths that call IsInBinaryCache without FillNarInfoCache.

If you're looking for an alternate approach, each package could have a sync.Once that makes the HEAD request. The PackageFromString(s) functions would call go pkg.checkCache.Do(...) to kick off the requests in the background. The Package.IsInBinaryCache method would use the same once to block until the network request has finished.

gcurtis · 2023-08-16T16:53:29Z

internal/devpkg/package.go

+	reqURL := BinaryCache + "/" + pathParts.hash + ".narinfo"
+	res, err := httpClient.Head(reqURL)
+	if err != nil {
+		if os.IsTimeout(err) {


os.IsTimeout is deprecated because it's vague in what's considered a timeout. Use http.NewRequestWithContext instead of setting a timeout in the client. Then you can check with errors.Is(ctx.Err(), context.DeadlineExceeded).

gcurtis · 2023-08-16T17:02:14Z

internal/devpkg/package.go

+			dashIndex = i
+		}
+	}
+	return &storePathParts{hash, name, "" /*version*/}


storePathParts{Version: ""} instead of a comment.

but then hash, and name are not set in the struct, no?

I meant something like storePathParts{hash: hash, name: name}.

gcurtis · 2023-08-16T17:05:07Z

internal/devpkg/package.go

+	return &storePathParts{hash, name, "" /*version*/}
+}
+
+func (p *storePathParts) Equal(other *storePathParts) bool {


If you use storePathParts as a value then you can just compare with ==.

gcurtis · 2023-08-16T17:06:20Z

internal/devpkg/package.go

+	group, _ := errgroup.WithContext(ctx)
+	for _, p := range packages {
+		// If the package's NarInfo status is already known, skip it
+		if _, ok := isNarInfoInCache[p.Raw]; ok {


Suggested change

if _, ok := isNarInfoInCache[p.Raw]; ok {

if isNarInfoInCache[p.Raw] {

actually, here, the value can be false, so I need to check if it is set.

gcurtis · 2023-08-16T17:07:30Z

internal/devpkg/package.go

 	}

-	// enable for nix >= 2.17
-	return vercheck.SemverCompare(version, "2.17.0") >= 0, nil
+	httpClient := &http.Client{


Use a single http.Client for all packages to reuse TCP connections.

gcurtis · 2023-08-16T17:09:05Z

internal/devpkg/package.go

+		group.Go(func() error {
+			return pkg.fillNarInfoCache()
+		})


Can this become group.Go(pkg.fillNarInfoCache)?

gcurtis

I think (correct me if I'm wrong) that fillNarInfoCache is being run concurrently for each package, but it calls a bunch of stuff that isn't safe (writing to isNarInfoInCache, nix.Version, etc.).

gcurtis · 2023-08-16T17:32:55Z

internal/devpkg/package.go

+		}
+		pkg := p
+		group.Go(func() error {
+			return pkg.fillNarInfoCache()


I don't think pkg.fillNarInfoCache is safe to run concurrently. The map, Nix system/version, etc. all need locking.

oh good catch! I can ensure the nix.System value is cached prior to executing this, but may need to add some locking for the map.

savil

@gcurtis updated the code without doing the sync.Once.

If you're looking for an alternate approach, each package could have a sync.Once that makes the HEAD request. The PackageFromString(s) functions would call go pkg.checkCache.Do(...) to kick off the requests in the background. The Package.IsInBinaryCache method would use the same once to block until the network request has finished.

Ooh I like this approach. It would simplify the code, and ensure it works by default: The current pattern of "ensure you call X before calling Y method" is really ugly :(

The reason I didn't directly kick off a HEAD request in one of the package constructors is:

Unfortunately, we recreate the Package structs for the same packages in different places. The structs are not reused. This is why I have a devpkg variable isNarInfoInCache, and not a struct field.
We may create a Package struct which is not used for IsBinaryInCache, and so we'd do wasted work.

One idea is to modify your idea from the PackageFromString constructor to the IsInBinaryCache where we read the result (the modification is: lazily initialize it):

Inside pkg.IsInBinaryCache(), we can do go sync.Once(pkg.fillNarInfoCache). And call that sync.Once(pkg.fillNarInfoCache) again to block for the return value.

If its the first time, it'll kick off a goroutine in the background.

~~Let me try in a stacked PR.... so as not to block this PR but also try to improve it.~~ EDIT: nope, this doesn't work: it'll be block other packages.

savil · 2023-08-16T22:04:03Z

internal/devpkg/package.go

+			dashIndex = i
+		}
+	}
+	return &storePathParts{hash, name, "" /*version*/}


but then hash, and name are not set in the struct, no?

savil · 2023-08-16T22:04:31Z

internal/devpkg/package.go

+	group, _ := errgroup.WithContext(ctx)
+	for _, p := range packages {
+		// If the package's NarInfo status is already known, skip it
+		if _, ok := isNarInfoInCache[p.Raw]; ok {


actually, here, the value can be false, so I need to check if it is set.

savil · 2023-08-16T22:04:41Z

internal/devpkg/package.go

+		}
+		pkg := p
+		group.Go(func() error {
+			return pkg.fillNarInfoCache()


savil · 2023-08-16T23:38:01Z

Hmm, I need to make some fixes...moving to draft.

savil · 2023-08-17T17:38:12Z

@gcurtis PTAL

gcurtis

Approving to unblock, but I think are a couple more race conditions in fillNarInfoCache.

gcurtis · 2023-08-17T17:42:02Z

internal/devpkg/package.go

+	pathParts := newStorePathParts(sysInfo.StorePath)
+	reqURL := BinaryCache + "/" + pathParts.hash + ".narinfo"
+	//nolint:govet
+	ctx, _ := context.WithDeadline(context.Background(), time.Now().Add(5*time.Second))


cancel() always needs to be called (even after the action is completed) otherwise it'll leak resources.

You can also use WithTimeout to avoid adding the time yourself.

gcurtis · 2023-08-17T17:44:07Z

internal/devpkg/package.go

+			dashIndex = i
+		}
+	}
+	return &storePathParts{hash, name, "" /*version*/}


I meant something like storePathParts{hash: hash, name: name}.

gcurtis · 2023-08-17T17:48:46Z

internal/devpkg/package.go

+		return nil
+	}
+
+	sysInfo, err := p.sysInfoIfExists()


I think there are more races here:

this eventually calls nix.Version which could cause multiple nix --version commands to launch at once. It looks like they'd also race to write to the same global version variable.

assuming packages share the same lockfile, it calls p.lockfile.Resolve which can update lock.File.Packages.

the call to nix.System can call ComputeSystem which cachedSystem == "" which runs a Nix command and writes to the global cachedSystem variable.

Gah, you are right.

Thought: Instead of sprinkling locks everywhere, I could ensure these are invoked for all packages prior to starting the go-routines. Then within the goroutines, they'll be in read-only mode. Would that work?

I think that would work. It might be easier to just split apart the HTTP request from everything else. Then you can calculate the store hash and check the map synchronously.

nit we should cache nix.Version the same way we do with nix.System

the call to nix.System can call ComputeSystem

This should be pre-computed. I think the ComputeSystem in there was just out of abundance of caution.

nix.Version is cached already

savil · 2023-08-17T18:20:50Z

Gah, I really dislike the implementation in this PR. It makes the code brittle and harder to reason about. Inclined to try other approaches. Will land, but keep thinking about refactoring this.

…ointer

mikeland73

@savil this is great!

I wonder if we can split out much of the nar cache stuff in devpkg into own file to keep it a bit more separate.

mikeland73 · 2023-08-18T14:55:53Z

internal/devpkg/package.go

+	exists, ok := isNarInfoInCache.status[p.Raw]
+	isNarInfoInCache.lock.RUnlock()
+	if !ok {
+		return false, errors.Errorf("narInfo cache miss: %v. call XYZ before invoking IsInBinaryCache", p.Raw)


Bad error string? XYZ ?

mikeland73 · 2023-08-18T14:56:28Z

internal/devpkg/package.go

+		return nil
+	}
+
+	sysInfo, err := p.sysInfoIfExists()


nit we should cache nix.Version the same way we do with nix.System

mikeland73 · 2023-08-18T14:58:59Z

internal/devpkg/package.go

+		return nil
+	}
+
+	sysInfo, err := p.sysInfoIfExists()


the call to nix.System can call ComputeSystem

This should be pre-computed. I think the ComputeSystem in there was just out of abundance of caution.

## Summary No logic change. Just grouping this code into its own file, ordered by public functions/methods on top. Also fixed this placeholder error message: #1318 (comment) ## How was it tested? compiles

savil requested review from ipince, gcurtis and mikeland73 July 27, 2023 00:45

gcurtis reviewed Jul 31, 2023

View reviewed changes

savil requested a review from gcurtis July 31, 2023 23:37

vercel bot had a problem deploying to Preview July 31, 2023 23:38 Failure

mikeland73 reviewed Jul 31, 2023

View reviewed changes

internal/devpkg/package.go Outdated Show resolved Hide resolved

mikeland73 reviewed Jul 31, 2023

View reviewed changes

jetify-com deleted a comment from vercel bot Aug 2, 2023

savil marked this pull request as draft August 7, 2023 21:03

savil force-pushed the savil/narinfo-in-cache branch from 36c196d to 857dda1 Compare August 15, 2023 00:29

savil requested a review from mikeland73 August 15, 2023 05:09

savil marked this pull request as ready for review August 15, 2023 05:09

gcurtis approved these changes Aug 16, 2023

View reviewed changes

gcurtis requested changes Aug 16, 2023

View reviewed changes

savil added 5 commits August 16, 2023 11:45

[rm nixpkgs] make HEAD request to BinaryCache to ensure binary is cached

94df293

Make newStorePathParts more robust, add TestCases

510fb62

add FillNarInfoCache to make HEAD requests in parallel

cdacb62

refactor for clarity

97efeac

use lock for isNarInfoInCache; and other suggested changes

1afca73

savil force-pushed the savil/narinfo-in-cache branch from cf321c6 to 1afca73 Compare August 16, 2023 22:30

savil commented Aug 16, 2023

View reviewed changes

savil requested a review from gcurtis August 16, 2023 23:12

savil force-pushed the savil/narinfo-in-cache branch 2 times, most recently from 5850449 to d2d0a57 Compare August 17, 2023 00:56

savil marked this pull request as draft August 17, 2023 00:57

savil force-pushed the savil/narinfo-in-cache branch from d2d0a57 to 1afca73 Compare August 17, 2023 00:59

savil force-pushed the savil/narinfo-in-cache branch 2 times, most recently from dcc715b to 4e30ce1 Compare August 17, 2023 03:25

savil marked this pull request as ready for review August 17, 2023 03:58

fix handling error return from fillNarInfoInCache

c3af318

savil force-pushed the savil/narinfo-in-cache branch from 4e30ce1 to c3af318 Compare August 17, 2023 03:59

gcurtis approved these changes Aug 17, 2023

View reviewed changes

Make concurrency safe; cancel context; storePathParts as struct not p…

9c14ebb

…ointer

savil merged commit ff28666 into main Aug 17, 2023

savil deleted the savil/narinfo-in-cache branch August 17, 2023 22:20

mikeland73 reviewed Aug 18, 2023

View reviewed changes

savil mentioned this pull request Aug 21, 2023

[Remove Nixpkgs] move concurrency code to own file #1412

Merged

	if _, ok := isNarInfoInCache[p.Raw]; ok {
	if isNarInfoInCache[p.Raw] {

[rm nixpkgs] make HEAD request to BinaryCache to ensure binary is cached #1318

[rm nixpkgs] make HEAD request to BinaryCache to ensure binary is cached #1318

Uh oh!

Conversation

savil commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

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!

Uh oh!

Uh oh!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

savil commented Aug 7, 2023

Uh oh!

savil commented Aug 15, 2023

Uh oh!

gcurtis 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!

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!

gcurtis 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!

savil left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil commented Aug 16, 2023

Uh oh!

savil commented Aug 17, 2023

Uh oh!

gcurtis 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!

Choose a reason for hiding this comment

savil commented Jul 27, 2023 •

edited

Loading

savil left a comment •

edited

Loading

savil commented Aug 17, 2023 •

edited

Loading