Repository navigation
Remove automaxprocs dependency - #8696
Conversation
This is handled natively by Go since 1.25, so this dependency should no longer be needed. See references below for more information. Only notable difference seems to be that Go sets a minimum value of 2 while the automaxprocs lib has a minimum value of 1. Go seems to account for much more though, so I don't think that difference alone warrants the inclusion of this dependency. Users who really want GOMAXPROCS=1 can always set that themselves. References: - golang/go#73193 - uber-go/automaxprocs#98 Signed-off-by: Anders Eknert <anders.eknert@apple.com>
|
This was what kept me from proposing this earlier: https://github.com/uber-go/automaxprocs/pull/99/changes#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R5-R6 Erring on the side of caution suggest to keep it, what do you think? |
|
Yep, the behavior is slightly different, but OTOH the stdlib one is likely to become the norm / expectation over time, so I think going with that is perhaps the better long-term bet. Have we even documented this anywhere? While that's... not great, it makes the current behavior feel less like a contract to me. So my preference would be to remove, with a note in the release notes for awareness. I doubt it'll happen, but should some users feel strongly about this, we can of course always revert. Or add a few lines of custom code I suppose, if it's only a matter of rounding. But I don't feel strongly about it myself, so if you think we better keep this as is, I don't mind closing. |
|
Thanks for your reply, and I'm with you. It's probably fine. After all, anyone with a custom build can put this into their |
|
My interpretation of the issues is that in Kubernetes pods with a CPU limit of <1, Go 1.25 gives min GOMAXPROCS=2, which can cause more throttling. The alternative is GOMAXPROCS=1, the minimum for automaxprocs, disables all parallelism and can lead to longer pauses while completing GC. Neither is perfect for <1 CPU quotas. My intuition is that for OPA, while both latency and pauses are bad, it feels to me like some throttling latency is better (slowness rather than spikes) as we have a lot of per request objects to clean up. |
Documents issue in memory constrained environments following the change in typical heap size following the change from 1 to 2 max procs in open-policy-agent#8696 Signed-off-by: Charlie Egan <charlie_egan@apple.com>
Documents issue in memory constrained environments following the change in typical heap size following the change from 1 to 2 max procs in #8696 --------- Signed-off-by: Charlie Egan <charlie_egan@apple.com>
This PR reverts the change in #8696 where automaxprocs was removed. This meant the go language default of 2 procs vs 1 was used. In some memory constrained deployments of OPA, this caused OOM issues where the server used more memory than before with 2 processes running. This PR restores the automaxprocs dependency and this behaviour. This PR also follows other data-heavy CNCF projects in pairing this with automemlimit to automate the setting of GOMEMLIMIT based on the cgroup data too. This makes no change for users manually setting these, but hopefully will provide some sensible defaults that work well for most cases, and better than the Go defaults in low resource deployments. <details> <summary>Other projects</summary> * https://github.com/prometheus/prometheus/blob/2ad3a871701531d1485f678acb7523cd0aa31789/go.mod#L11 * https://github.com/istio/istio/blob/4985ae369245dcc91bc107a0c47ea41ded0c1a12/go.mod#L8 * https://github.com/grafana/alloy/blob/e11d0f03e2b2e727161fb2169cade54698e17bd8/go.mod#L19 * https://github.com/thanos-io/thanos/blob/main/go.mod#L9 * https://github.com/kubernetes/kube-state-metrics/blob/main/go.mod#L6 * https://github.com/authzed/spicedb/blob/main/go.mod#L22 </details> --------- Signed-off-by: Charlie Egan <charlie_egan@apple.com>
The memory pressure in low resource containers this PR aimed to fix was actually caused by #8817, which was fixed in #8829. The automaxprocs/automemlimit dependencies are no longer needed as was intended in #8696. This reverts commit 88c01e6 and updates docs to match the current behaviour. --------- Signed-off-by: Charlie Egan <charlie_egan@apple.com>
|
This one came back in #8869 🧟 |
opa 1.17.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>This release contains a mix of new features, performance improvements, and bugfixes. Notably: - A new `future.keywords.not` import that adds improved semantics to the `not` keyword. - Rule Labels in Decision Logs - Published json schema for IR and bundle manifest - Dropped automaxprocs and x/net dependencies ### Improved Negation Semantics ([#8387](open-policy-agent/opa#8387)) This OPA release introduces a new [`future.keywords.not` import](https://www.openpolicyagent.org/docs/policy-reference/keywords/not#improved-negation-semantics) that fixes a long-standing semantic issue with negation in Rego. Without the import, the compiler expands a negated composite expression like `not f(g(input.x))` into a series of sub-expressions evaluated *before* the `not`: ``` __local0__ = input.x g(__local0__, __local1__) not f(__local1__) ``` If any sub-expression fails — for example, `input.x` is undefined or `g` produces an undefined result — the entire rule fails rather than the `not` succeeding. This is unintuitive: the user's intent is "the condition does not hold," but an undefined intermediate value causes a silent failure instead of the expected `not` result. With `import future.keywords.not`, composite-expression negation wraps the full compiler expansion in an implicit body: ``` not { __local0__ = input.x; g(__local0__, __local1__); f(__local1__) } ``` Now, if *any* sub-expression is undefined or fails, the body is unsatisfiable and the `not` expression succeeds; matching the intuition that "the condition does not hold." > **_NOTE:_** > > Users are recommended to import `future.keywords.not` whenever the `not` keyword is used in a policy. Authored by @johanfylling ### Rule Labels in Decision Logs ([#2089](open-policy-agent/opa#2089)) Rule annotations now support a `labels` field. Labels from all successfully evaluated rules are collected and included in each decision log entry as a top-level `rule_labels` array. Each element is the merged label map for one successfully evaluated rule, with inner-scope-wins precedence across the rule's annotation chain (`subpackages` < `package` < `document` < `rule`). Merged maps are deduplicated across rules so that identical label sets collapse to a single entry. ```rego # METADATA # scope: package # labels: # service: authz # severity: info package myapp # METADATA # labels: # severity: low # team: platform allow if input.role == "admin" ``` The resulting decision log entry will contain: ```json {"rule_labels": [{"service": "authz", "severity": "low", "team": "platform"}]} ``` Note how `severity: info` from the package scope is overridden by `severity: low` from the rule scope. Queries against `rule_labels` can now rely on each entry carrying the full label context for a single rule, rather than one entry per contributing scope. Both the runtime and the Go SDK now process metadata annotations by default. Authored by @srenatus, reported by @tsandall ### Runtime, SDK, Tooling - ast: Allow `$ref` in `allOf` in JSON schemas ([#6523](open-policy-agent/opa#6523)) authored by @deeglaze reported by @mosiac1 - bundle: Update bundle roots conflict detection algorithm. ([#8664](open-policy-agent/opa#8664)) authored by @philipaconrad - download: Use oras, not containerd ([#8639](open-policy-agent/opa#8639)) authored by @srenatus - server: Remove dead code (s.partials) ([#8708](open-policy-agent/opa#8708)) authored by @srenatus - server: Wire in response/request metadata for compile handler ([#8650](open-policy-agent/opa#8650)) authored by @srenatus - server/types: generalize request/response metadata ([#8650](open-policy-agent/opa#8650)) authored by @srenatus ### Compiler, Topdown and Rego - builtins: Enable pattern validation in `json.verify_schema` and `json.match_schema` built-in functions ([#6089](open-policy-agent/opa#6089)) authored by @sspaink reported by @ewout8 - ir: Don't capitalize `index` field in `MakeNumberRefStmt` IR statement ([#6266](open-policy-agent/opa#6266)) authored by @sspaink reported by @johanfylling - perf: Avoid allocating in binary and/or operators when possible ([#8689](open-policy-agent/opa#8689)) authored by @anderseknert - rego: Allow per-eval `GenerateJSON` function ([#8690](open-policy-agent/opa#8690)) authored by @anderseknert ### Docs, Website, Ecosystem - ecosystem: add OPA MCP ([#8618](open-policy-agent/opa#8618)) authored by @OrygnsCode - docs: Add explicit address binding to examples ([#8688](open-policy-agent/opa#8688)) authored by @charlieegan3 - docs: Add titles to code blocks in policy-testing ([#8649](open-policy-agent/opa#8649)) authored by @charlieegan3 - docs: Correct OCP SSH key docs ([#8675](open-policy-agent/opa#8675)) authored by @taurelius - docs: Update diagram to match index examples ([#8667](open-policy-agent/opa#8667)) authored by @charlieegan3 ### Miscellaneous - ast,storage/inmem: Add `inmem.NewFromASTObject` and add missing string case to `ast.InternedValue` ([#8707](open-policy-agent/opa#8707)) authored by @anderseknert - build: `go install` -> `go install tool` to control checksums ([#8646](open-policy-agent/opa#8646)) authored by @srenatus - build: Push edge binaries to bucket ([#8668](open-policy-agent/opa#8668)) authored by @charlieegan3 - workflows: Fix benchmarks workflow (replace action, avoid stackoverflow) ([#8655](open-policy-agent/opa#8655)) authored by @srenatus - workflows: Note improvements in benchmark comments ([#8673](open-policy-agent/opa#8673)) authored by @srenatus - Generate a JSON Schema for the IR plan ([#8662](open-policy-agent/opa#8662)) authored by @sspaink reported by @kroekle - Generate a JSON Schema for the bundle manifest ([#8661](open-policy-agent/opa#8661)) authored by @sspaink reported by @kroekle - Dependency updates; notably: - build(deps): Remove automaxprocs dependency ([#8696](open-policy-agent/opa#8696)) authored by @anderseknert - build(deps): Remove direct x/net dependency ([#8697](open-policy-agent/opa#8697)) authored by @anderseknert - build(deps): Bump github.com/bytecodealliance/wasmtime-go from 43.0.2 to 44.0.0 ([8652](open-policy-agent/opa#8652)) authored by @srenatus - build(deps): Bump github.com/fsnotify/fsnotify from 1.9.0 to 1.10.1 - build(deps): Bump github.com/huandu/go-sqlbuilder from 1.40.2 to 1.41.0 - build(deps): Bump github.com/lestrrat-go/jwx/v3 from 3.1.0 to 3.1.1 - build(deps): Bump github.com/vektah/gqlparser/v2 from 2.5.32 to 2.5.33 - build(deps): Bump google.golang.org/grpc from 1.80.0 to 1.81.0 - build(deps): Bump gopkg.in/ini.v1 from 1.67.1 to 1.67.2 </pre> <p>View the full release notes at <a href="https://github.com/open-policy-agent/opa/releases/tag/v1.17.0">https://github.com/open-policy-agent/opa/releases/tag/v1.17.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!9302
This is handled natively by Go since 1.25, so this dependency should no longer be needed. See references below for more information. Only notable difference seems to be that Go sets a minimum value of 2 while the automaxprocs lib has a minimum value of 1. Go seems to account for much more though, so I don't think that difference alone warrants the inclusion of this dependency. Users who really want GOMAXPROCS=1 can always set that themselves.
References: