Skip to content

docs(rfc): Settings Service — hierarchical runtime configuration - #7775

Merged
pingsutw merged 11 commits into
mainfrom
rfc-settings-service
Sep 2, 2026
Merged

pingsutw merged 11 commits into
mainfrom
rfc-settings-service

Conversation

@pingsutw

@pingsutw pingsutw commented Aug 4, 2026

Copy link
Copy Markdown
Member

Why

The settings API (flyteidl2/settings/settings_service.proto, settings_definition.proto) was designed, reviewed, and merged in #7127 — with generated clients for Go, TypeScript, Python, and Rust — but no server implements it yet. This RFC proposes implementing the SettingsService: instance/domain/project-scoped, strongly-typed runtime configuration (default queue, task resource guardrails, env vars, labels/annotations, service account, storage paths) with well-defined inheritance semantics.

What this RFC covers

  • Why we need it — today every runtime knob is either static server config (global, redeploy to change) or repeated in user code; there is no way to set per-domain defaults or enforce platform guardrails centrally.
  • When we need it — concrete examples: per-domain env var defaults, org-wide resource ceilings, default queue routing, and UNSET blocking inheritance.
  • Phased implementation plan — ~15 small, mostly independent tasks across 4 phases (persistence → resolution engine + service → applying settings on the run path → CLI/UI/docs), each with an existing in-repo pattern to copy from, so many contributors can pick up pieces in parallel. Phase 1 tasks are good first issues.

The existing walkthrough doc flyteidl2/settings/settings_customer_flow.md doubles as the acceptance-test spec: Phase 2 is done when every example in it reproduces against a running server.

The only proto change proposed is a single new pod_template_name StringSetting (restores per-project/domain PodTemplate selection, which single-namespace Flyte 2 lost); everything else is server/consumer implementation work.

Proposes implementing the already-merged flyteidl2/settings IDL (#7127):
org/domain/project-scoped typed settings with inheritance, phased into
small independent tasks for parallel contribution.

Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>
Copilot AI lite review requested due to automatic review settings August 4, 2026 19:58
Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an RFC document proposing a server-side implementation of Flyte 2’s existing (already-merged) strongly-typed Settings API, describing motivation, inheritance semantics, storage approach, resolution/validation logic, service wiring, and a phased delivery plan.

Changes:

  • Introduces a new RFC detailing hierarchical org/domain/project runtime configuration via SettingsService.
  • Documents scope inheritance rules (INHERIT/VALUE/UNSET) with concrete examples for env vars, resource guardrails, and queue routing.
  • Outlines an incremental implementation plan (persistence → resolution/service → run-path application → user surfaces) with pointers to in-repo patterns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>
Copilot AI review requested due to automatic review settings August 4, 2026 20:05
Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/rfcs/7775-settings-service.md:199

  • The RFC proposes a text key format "v1:{org}:{domain}:{project}", but (per flyteidl2/common/identifier.proto) domain/project identifiers are not constrained to exclude :. Without either escaping or an explicit restriction, : inside a segment would make the encoding ambiguous and hard to parse reliably. Consider specifying/validating a delimiter-safe encoding (e.g., URL-escape segments) or explicitly rejecting : in org/domain/project at the API boundary.
Validation is hand-written in the service (the generated `Validate()` from
protoc-gen-validate does not enforce the `buf.validate` annotations used in
the settings protos): key shape (org required; project requires domain),
quantities parse via `resource.ParseQuantity`, and

OSS has no organization concept; normalize empty SettingsKey.org to
secret.DefaultOrganization ("flyte"), matching the secret and app
services. Org-level settings become instance-wide defaults.

Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>
Copilot AI review requested due to automatic review settings August 4, 2026 20:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/rfcs/7775-settings-service.md:172

  • For consistency with existing runs/migrations/sql/*.sql (e.g. runs/migrations/sql/20260408110000_init_schema.sql uses DEFAULT CURRENT_TIMESTAMP), consider using CURRENT_TIMESTAMP instead of NOW() in this schema example so future migrations follow the established pattern.
    created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
    updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()

Flyte 2 runs all task pods in one namespace, so v1's implicit
per-project-domain PodTemplate (via per-namespace resources) no longer
works. Propose a pod_template_name StringSetting: resolved per scope,
stamped onto tasks without an explicit template, looked up by the
existing PodTemplateStore.

Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>
Copilot AI review requested due to automatic review settings August 4, 2026 20:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/rfcs/7775-settings-service.md:232

  • The validation bound uses MaxUint32 but the doc doesn’t explain why that upper limit exists. Since the setting is later applied to RunSpec.max_action_concurrency (a uint32 in flyteidl2/task/run.proto), it would be clearer to either cite that type constraint or spell out the numeric upper bound so readers don’t have to infer it.
`max_action_concurrency` ∈ {0} ∪ [2, MaxUint32] (a cap of 1 would deadlock
any run with more than one action).

Copilot AI review requested due to automatic review settings August 4, 2026 20:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/rfcs/20260804_settings_service.md:180

  • This section says OSS clients can omit org and rely on server-side normalization, but SettingsKey.org in flyteidl2/settings/settings_definition.proto is annotated with (buf.validate.field).string.min_len = 1, so clients (or future server middleware) that enforce buf.validate will reject an empty org. The RFC should either propose loosening that IDL constraint, or clarify that OSS clients should still send the placeholder org ("flyte").
**Org handling in OSS.** OSS deployments have no organization concept, so an
empty `org` is normalized server-side to the existing placeholder
`DefaultOrganization = "flyte"`
(`flyteplugins/go/tasks/pluginmachinery/secret/embedded_secret_manager.go`) —
the same convention the secret and app services already use. Org-level

Copilot AI review requested due to automatic review settings August 4, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/rfcs/20260804_settings_service.md:177

  • This section says OSS clients never need to send an org and can send an empty org that the server normalizes to "flyte", but the IDL currently declares SettingsKey.org with (buf.validate.field).string.min_len = 1 (see flyteidl2/settings/settings_definition.proto:74). Even if the server chooses to accept empty orgs, the RFC should acknowledge this mismatch (or recommend clients send "flyte" explicitly) to avoid surprising client / gateway-side validation failures.
`DefaultOrganization = "flyte"`
(`flyteplugins/go/tasks/pluginmachinery/secret/embedded_secret_manager.go`) —
the same convention the secret and app services already use. Org-level
settings therefore act as **instance-wide defaults**; clients never need to
send an org.

OSS has no org concept; keep 'org' only where the wire format and
storage key require it, with the default-org normalization note.

Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>
Copilot AI review requested due to automatic review settings August 4, 2026 21:11
flyte-sdk already supports editing settings, so phase 4 is end-to-end
verification plus docs; no UI work. Remove the Alternatives section.

Claude-Session: https://claude.ai/code/session_015eKZQGuUBZ5PaRmHp6PdFH
Signed-off-by: Kevin Su <pingsutw@apache.org>
Copilot AI review requested due to automatic review settings August 4, 2026 21:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

docs/rfcs/20260804_settings_service.md:199

  • The optimistic-locking UPDATE example only increments version; as written it doesn’t mention updating updated_at, which would otherwise remain at its insert-time value unless the repository layer explicitly sets it. Clarifying this in the RFC will avoid implementers copying an UPDATE that never bumps updated_at.
- `version` implements optimistic locking: `UPDATE ... SET version = version + 1
  WHERE key = $1 AND version = $2`; zero rows affected ⇒ the caller loses the
  race and must re-read (same pattern as the existing trigger repository).

docs/rfcs/20260804_settings_service.md:164

  • In the IDL (flyteidl2/settings/settings_service.proto) the RPC is described as an upsert, but here it’s described only as an optimistic-locking update. Since the version field is required for updates (and the customer-flow doc indicates Create vs Update depending on record existence), it would help to explicitly state whether UpdateSettings is expected to fail when the record doesn’t exist vs support a create-on-missing path and what version should be used in that case.
service SettingsService {
  rpc GetSettings(GetSettingsRequest) returns (GetSettingsResponse);            // merged, effective values
  rpc GetSettingsForEdit(GetSettingsForEditRequest) returns (GetSettingsForEditResponse); // unmerged, one record per scope level
  rpc CreateSettings(CreateSettingsRequest) returns (CreateSettingsResponse);
  rpc UpdateSettings(UpdateSettingsRequest) returns (UpdateSettingsResponse);   // optimistic locking via version
}

Copilot AI review requested due to automatic review settings August 4, 2026 21:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/rfcs/20260804_settings_service.md:228

  • The motivation for rejecting max_action_concurrency = 1 is described as a “deadlock”, but with a simple concurrency cap a value of 1 would typically just serialize action execution (slow), not deadlock. This reads as inaccurate and could mislead future implementers/readers; consider rephrasing the rationale while keeping the proposed bounds clear (also makes the negative/invalid-case handling more explicit).
the settings protos): key shape (empty org defaults to `"flyte"`; project
requires domain),
quantities parse via `resource.ParseQuantity`, and
`max_action_concurrency` ∈ {0} ∪ [2, MaxUint32] (a cap of 1 would deadlock
any run with more than one action).

@Sovietaced

Sovietaced commented Aug 4, 2026

Copy link
Copy Markdown
Member

This makes sense to me.

pingsutw pushed a commit that referenced this pull request Aug 17, 2026
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
pingsutw pushed a commit that referenced this pull request Aug 21, 2026
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
@davidlin20dev

davidlin20dev commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

I'm currently implementing 2.3 Validator, and wanted to check the reasoning behind the bounds.
I understand the formula: we reject 1 and only allow 0 or any number greater than 1
My question is: why would a cap of 1 deadlock? and what does 0 mean in here?

{0} ∪ [2, MaxUint32] (a cap of 1 would deadlock any run with more than one action)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants