dashboard/app: add a helper to fix up build arches#7409
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new admin endpoint patchBuildArchs to update build architectures for a specified namespace. The review feedback highlights critical improvements for the implementation: optimizing the Datastore query by filtering on the architecture directly to prevent potential OOMs, postponing HTTP header writes until after fallible operations to ensure proper error handling, and simplifying the update logic by removing the temporary mapping.
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | ||
| fmt.Fprintf(w, "Patching builds for namespace %q (dry_run=%v)\n", ns, dryRun) | ||
|
|
||
| var builds []*Build | ||
| keys, err := db.NewQuery("Build"). | ||
| Filter("Namespace=", ns). | ||
| GetAll(ctx, &builds) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to query builds: %w", err) | ||
| } | ||
|
|
||
| var toUpdate []*db.Key | ||
| newArchs := make(map[string]string) | ||
|
|
||
| for i, build := range builds { | ||
| if build.Arch != "x86_64" { | ||
| continue | ||
| } | ||
|
|
||
| targetArch := "amd64" | ||
| if arm64Re.MatchString(build.Manager) { | ||
| targetArch = "arm64" | ||
| } | ||
| if targetArch == build.Arch { | ||
| continue | ||
| } | ||
|
|
||
| key := keys[i] | ||
| toUpdate = append(toUpdate, key) | ||
| newArchs[key.String()] = targetArch | ||
|
|
||
| fmt.Fprintf(w, "Build %q (Manager: %q) %q -> %q\n", build.ID, build.Manager, build.Arch, targetArch) | ||
| } | ||
|
|
||
| fmt.Fprintf(w, "Found %d builds to patch.\n", len(toUpdate)) | ||
|
|
||
| if dryRun { | ||
| fmt.Fprintf(w, "Dry run, skipping datastore update.\n") | ||
| return nil | ||
| } | ||
|
|
||
| return updateBatch(ctx, toUpdate, func(key *db.Key, b *Build) { | ||
| targetArch, ok := newArchs[key.String()] | ||
| if !ok { | ||
| panic("unknown build key") | ||
| } | ||
| b.Arch = targetArch | ||
| }) |
There was a problem hiding this comment.
There are a few significant improvement opportunities in this helper function:
- Efficiency: Instead of querying all builds for the namespace and filtering them in memory, you can filter by
Arch = "x86_64"directly in the Datastore query. For namespaces with a large history (likeupstream), loading all builds can cause Out Of Memory (OOM) errors or request timeouts. - Correctness: Setting headers and writing to the response writer before performing fallible operations (like Datastore queries) is an anti-pattern. If the query fails, the error handler will try to write an HTML error page, but the
Content-Typeheader is already set totext/plain, causing the browser to render raw HTML. - Simplicity: The
newArchsmap and key stringification can be completely eliminated by re-evaluating the target architecture inside theupdateBatchcallback.
var builds []*Build
keys, err := db.NewQuery("Build").
Filter("Namespace=", ns).
Filter("Arch=", "x86_64").
GetAll(ctx, &builds)
if err != nil {
return fmt.Errorf("failed to query builds: %w", err)
}
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
fmt.Fprintf(w, "Patching builds for namespace %q (dry_run=%v)\n", ns, dryRun)
var toUpdate []*db.Key
for i, build := range builds {
targetArch := "amd64"
if arm64Re.MatchString(build.Manager) {
targetArch = "arm64"
}
key := keys[i]
toUpdate = append(toUpdate, key)
fmt.Fprintf(w, "Build %q (Manager: %q) %q -> %q\n", build.ID, build.Manager, build.Arch, targetArch)
}
fmt.Fprintf(w, "Found %d builds to patch.\n", len(toUpdate))
if dryRun {
fmt.Fprintf(w, "Dry run, skipping datastore update.\n")
return nil
}
return updateBatch(ctx, toUpdate, func(key *db.Key, b *Build) {
if arm64Re.MatchString(b.Manager) {
b.Arch = "arm64"
} else {
b.Arch = "amd64"
}
})
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md