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.

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

fix: occasional jsonnet timeouts #800

Merged
merged 4 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ jobs:
- name: golangci-lint
uses: golangci/golangci-lint-action@v3
with:
version: v1.55.2
version: v1.59.1
skip-go-installation: true
args: --timeout 5m
- name: Install cockroach DB
Expand Down
3 changes: 1 addition & 2 deletions .golangci.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
linters:
enable:
- gosec
- vet
- govet
disable-all: true

run:
skip-files:
- "migrate_files.go" # go-bindata
- ".+_test.go"
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ licenses: .bin/licenses node_modules # checks open-source licenses
GOBIN=$(shell pwd)/.bin go install golang.org/x/tools/cmd/goimports@latest

.bin/golangci-lint: Makefile
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b .bin v1.55.2
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b .bin v1.59.1

.bin/licenses: Makefile
curl https://raw.githubusercontent.com/ory/ci/master/licenses/install | sh
Expand Down
3 changes: 1 addition & 2 deletions cmdx/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,8 @@ func NewClient(cmd *cobra.Command) (*http.Client, *url.URL, error) {

rt := httpx.NewTransportWithHeader(header)
rt.RoundTripper = &http.Transport{
//#nosec G402 -- This is a false positive
TLSClientConfig: &tls.Config{
InsecureSkipVerify: skipVerify,
InsecureSkipVerify: skipVerify, //nolint:gosec // This is a false positive
},
}
hc.Transport = rt
Expand Down
3 changes: 1 addition & 2 deletions httpx/gzip_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ func (c *CompressionRequestReader) ServeHTTP(w http.ResponseWriter, r *http.Requ
return
}

/* #nosec G110 - FIXME */
if _, err := io.Copy(&b, reader); err != nil {
if _, err := io.Copy(&b, reader); err != nil { //nolint:gosec // FIXME
c.ErrHandler(w, r, err)
return
}
Expand Down
23 changes: 4 additions & 19 deletions jsonnetsecure/jsonnet.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,11 @@ import (
"path"
"runtime"
"testing"
"time"

"github.com/google/go-jsonnet"
)

type (
evaluationOptions struct {
evalTimeout time.Duration
}

EvaluationOptionModifier func(*evaluationOptions)

VM interface {
EvaluateAnonymousSnippet(filename string, snippet string) (json string, formattedErr error)
ExtCode(key string, val string)
Expand All @@ -38,11 +31,10 @@ type (
}

ProcessVM struct {
ctx context.Context
path string
args []string
execTimeout time.Duration
params processParameters
ctx context.Context
path string
args []string
params processParameters
}

vmOptions struct {
Expand All @@ -51,7 +43,6 @@ type (
args []string
ctx context.Context
pool *pool
execTimeout time.Duration
}

Option func(o *vmOptions)
Expand Down Expand Up @@ -79,12 +70,6 @@ func WithProcessIsolatedVM(ctx context.Context) Option {
}
}

func WithExecTimeout(timeout time.Duration) Option {
Copy link
Member

Choose a reason for hiding this comment

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

If you remove this it needs an update in depending services as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grepped our code bases and found no uses. :)

return func(o *vmOptions) {
o.execTimeout = timeout
}
}

func WithJsonnetBinary(jsonnetBinaryPath string) Option {
return func(o *vmOptions) {
o.jsonnetBinaryPath = jsonnetBinaryPath
Expand Down
42 changes: 24 additions & 18 deletions jsonnetsecure/jsonnet_pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package jsonnetsecure

import (
"bufio"
"cmp"
"context"
"encoding/json"
"io"
Expand All @@ -24,12 +23,11 @@ import (

type (
processPoolVM struct {
path string
args []string
ctx context.Context
params processParameters
execTimeout time.Duration
pool *pool
path string
args []string
ctx context.Context
params processParameters
pool *pool
}
Pool interface {
Close()
Expand Down Expand Up @@ -67,6 +65,10 @@ func NewProcessPool(size int) Pool {
if err != nil {
panic(err) // this should never happen, see implementation of puddle.NewPool
}
for range size {
// warm pool
go pud.CreateResource(context.Background())
}
go func() {
for {
time.Sleep(10 * time.Second)
Expand Down Expand Up @@ -143,12 +145,19 @@ func newWorker(ctx context.Context) (_ worker, err error) {
errs := make(chan string)
go scan(errs, stderr)

return worker{
w := worker{
cmd: cmd,
stdin: in,
stdout: out,
stderr: errs,
}, nil
}
_, err = w.eval(ctx, []byte("{}")) // warm up
if err != nil {
w.destroy()
return worker{}, errors.Wrap(err, "newWorker: warm up failed")
}

return w, nil
}

func (w worker) destroy() {
Expand Down Expand Up @@ -184,10 +193,6 @@ func (vm *processPoolVM) EvaluateAnonymousSnippet(filename string, snippet strin
ctx, span := tracer.Start(vm.ctx, "jsonnetsecure.processPoolVM.EvaluateAnonymousSnippet", trace.WithAttributes(attribute.String("filename", filename)))
defer otelx.End(span, &err)

// TODO: maybe leave the timeout to the caller?
ctx, cancel := context.WithTimeout(ctx, cmp.Or(vm.execTimeout, 1*time.Second))
defer cancel()

params := vm.params
params.Filename = filename
params.Snippet = snippet
Expand All @@ -203,6 +208,8 @@ func (vm *processPoolVM) EvaluateAnonymousSnippet(filename string, snippet strin
return "", errors.Wrap(err, "jsonnetsecure: acquire")
}

ctx, cancel := context.WithTimeout(ctx, 1*time.Second)
defer cancel()
result, err := worker.Value().eval(ctx, pp)
if err != nil {
worker.Destroy()
Expand All @@ -224,11 +231,10 @@ func NewProcessPoolVM(opts *vmOptions) VM {
ctx = context.Background()
}
return &processPoolVM{
path: opts.jsonnetBinaryPath,
args: opts.args,
ctx: ctx,
pool: opts.pool,
execTimeout: opts.execTimeout,
path: opts.jsonnetBinaryPath,
args: opts.args,
ctx: ctx,
pool: opts.pool,
}
}

Expand Down
11 changes: 5 additions & 6 deletions jsonnetsecure/jsonnet_processvm.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package jsonnetsecure

import (
"bytes"
"cmp"
"context"
"encoding/json"
"fmt"
Expand All @@ -24,10 +23,9 @@ import (

func NewProcessVM(opts *vmOptions) VM {
return &ProcessVM{
path: opts.jsonnetBinaryPath,
args: opts.args,
ctx: opts.ctx,
execTimeout: opts.execTimeout,
path: opts.jsonnetBinaryPath,
args: opts.args,
ctx: opts.ctx,
}
}

Expand All @@ -37,11 +35,12 @@ func (p *ProcessVM) EvaluateAnonymousSnippet(filename string, snippet string) (_
defer otelx.End(span, &err)

// We retry the process creation, because it sometimes times out.
const processVMTimeout = 1 * time.Second
return backoff.RetryWithData(func() (_ string, err error) {
ctx, span := tracer.Start(ctx, "jsonnetsecure.ProcessVM.EvaluateAnonymousSnippet.run")
defer otelx.End(span, &err)

ctx, cancel := context.WithTimeout(ctx, cmp.Or(p.execTimeout, 1*time.Second))
ctx, cancel := context.WithTimeout(ctx, processVMTimeout)
defer cancel()

var (
Expand Down
2 changes: 0 additions & 2 deletions jsonnetsecure/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"os"
"runtime"
"testing"
"time"
)

type (
Expand Down Expand Up @@ -45,7 +44,6 @@ func (p *TestProvider) JsonnetVM(ctx context.Context) (VM, error) {
WithProcessIsolatedVM(ctx),
WithProcessPool(p.pool),
WithJsonnetBinary(p.jsonnetBinary),
WithExecTimeout(time.Second*5),
), nil
}

Expand Down
Loading