Repository navigation
Conversation
128faa7 to
4258422
Compare
alexandernorth
left a comment
There was a problem hiding this comment.
lgtm, one minor suggestion
1ef4e22 to
518f4d1
Compare
25487e3 to
3846890
Compare
d12d108 to
533ae01
Compare
533ae01 to
dc979ab
Compare
Pins to sdcio/sdc-protos@f894efb (feat(config-read): define local ConfigReadService for config-server Config reads), the sdc-protos#134 rebase of the dangling config-server-cache-backend branch's a8f3da0 onto the now-merged-ready #123 tip (fd8bd02). Swap for a tagged sdc-protos release once #123 and #134 merge upstream. Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Defines LocalConfigReader (Get-by-name / List-by-target), the Target, Document, and ConfigBlob types per the ADR's field-mapping table, and an in-memory FakeLocalConfigReader seedable with fixtures. Lets the config-server-backed cache.Client (ticket 06) be built and unit-tested now, decoupled from the real cross-repo gRPC transport (ticket 01/07/08). Ticket 05 of docs/adr/0001-config-server-backed-cache-client.md; no existing file touched. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
…fake
Builds ConfigServerCache (pkg/cache/configserver.go), the new cache.Client
for Cache.Type: config-server, behind ticket 05's LocalConfigReader seam:
real-intent reads map onto the seam's Get/List and wrap results as
importer.IntentAdapter via a new configserver.NewImportAdapter
(pkg/cache/configserver/importer.go, merge.go) per the ADR's field-mapping
table; writes on real intents are unconditional no-ops; "running" is served
from its own independent in-memory, per-instance store, entirely separate
from the seam.
Re-derived against ticket 05's actual (post-34728c2) IntentAdapter/
ImportConfigAdapter split rather than the single ImportConfigAdapter shape
this was originally written against.
Per ticket 05's follow-up note, InstanceCreate seeds "running" with an
empty &tree_persist.Intent{} instead of nil, and InstanceRunningGet has no
ErrRunningNotFound/nil-guard: a freshly created instance's running store
simply reads back as an empty adapter, mirroring LocalCache's on-disk
running store, which also starts out with no data written to it.
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Implements ticket 08: GRPCConfigReader (pkg/cache/configserver/grpc.go) implements the local-read seam by calling the generated config_read.ConfigReadServiceClient (sdc-protos ticket 01), replacing ticket 05's fake behind ticket 06's backend. Connection settings are caller-supplied (grpc.ClientConnInterface / Dial(address)), never hardcoded. Unit-tested against a new generated mock (mocks/mockconfigread) of the real client. Pins sdc-protos to the sibling local checkout (untagged commit a8f3da0, which adds config_read) until that branch is pushed/tagged upstream. No wiring into Server.createCacheClient/CacheConfig — that's ticket 09. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Add the case "config-server" branch to createCacheClient, dialing the colocated config-server controller and constructing ConfigServerCache against it. Extend CacheConfig with Namespace and require both Address and Namespace for Cache.Type: config-server; local/remote behavior is unchanged. Closes ticket 09. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
…rver backend Adds splitDatastoreName + ErrMalformedDatastoreName so ConfigServerCache can later derive per-call target namespace/name from the datastore name instead of a fixed, deployment-wide namespace (ticket 01). Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
target() now decodes each call's cacheInstanceName via splitDatastoreName instead of trusting a single, deployment-wide namespace field, so one data-server process correctly serves datastores across multiple Kubernetes namespaces and config-server's ConfigReadService receives the bare target name. Every target() caller, plus InstanceCreate, propagates/surfaces ErrMalformedDatastoreName on a malformed datastore name. Constructors drop their namespace parameter. Also re-derived against ticket 05's noopIntentWriter capability split: ConfigServerCache no longer implements InstanceIntentModify/ InstanceIntentDelete itself — NewConfigServerClient composes it with the generic noopIntentWriter into a full Client instead, per noop_intent_writer.go's own doc comment. Server.createConfigServerCacheClient and the corresponding tests now go through NewConfigServerClient rather than asserting a concrete *ConfigServerCache. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Namespace was never meaningful once ConfigServerCache derives it per-call from the datastore name (ticket 02); config-server cache type now requires only Address (ticket 03). Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Config-server stores intents as a single JSON object at the YANG root path, which mergeConfigBlobs rejected during LoadAllButRunningIntents and broke performRevert after sync. Re-derived against this ticket's package layout: the branch this was written against had since moved this code to pkg/tree/importer/configserver; here it's still pkg/cache/configserver (ticket 06's original location), so the new tests reference the local ConfigBlob type directly instead of an imported csreader.ConfigBlob alias. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Config-server keys TransactionSet intents as namespace.name, but the config-server cache was returning bare Config names. That mismatch made DeviationWatcher split intent owners incorrectly and panic the controller. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
s.cacheClient is already statically typed cache.Client (Server.cacheClient's field type), so the explicit var _ cache.Client = s.cacheClient assertion added alongside 7b1a78a is redundant — staticcheck (QF1011) flags it. Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Use make(map[string]*Document, len(docs)) when seeding a new target map; map composite literals are fine elsewhere, but the prior review fix used make with a composite-literal type suffix, which fails typecheck. Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
Run 05-cache-backend against cache_type=config-server and fail the workflow when GitHub release lookups return empty values. Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
dc979ab to
e2add12
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
| if len(rest) == 0 { | ||
| // A keyed element with nothing after it has only its own key leaves | ||
| // to contribute — already seeded by findOrCreateListEntry. | ||
| return nil |
There was a problem hiding this comment.
is it possible that here the values can be lost in case a path in the config ends with a list entry?
Example config:
`apiVersion: config.sdcio.dev/v1alpha1
kind: Config
metadata:
name: intf-uplink
namespace: default
labels:
config.sdcio.dev/targetName: dev1
config.sdcio.dev/targetNamespace: default
spec:
priority: 10
config:
- path: /interface[name=ethernet-1/1]
value:
admin-state: enable
description: uplink`
And what data server builds from it:
{"interface":[{"name":"ethernet-1/1"}]}
Config-server itself still sends the full intent with TransactionSet, so the first apply to the device is correct. The lost leaves only affect intents that data-server reads back through this backend:
Next change to the intent: the old version data-server reads back is missing those leaves, so leaves that were removed in the new spec are never deleted from the device.
Rollback: the restored intent is missing those leaves.
BlameConfig / GetIntent: the leaves have no owner.
Deviations: the leaves look like running values that no intent manages ("unhandled").
Changes to other intents: a transaction for another intent on the same device rebuilds the tree from the cache. Leaves only this intent set lose their owner, and may be treated as deleted or no longer managed.
There was a problem hiding this comment.
As far as I understood config-server will never send that type of config, so the path will only be / so up to you if you see this as an issue for the future or not
Ticket 06 of the #460 + #471 PR-split reorg
(see
graph.md/index.mdin the tracking gist for the full stack). Base:cache-interface-prep(#495).Scope
Read-only
ConfigServerCache(Cache.Type: config-server): local-read seam,fake for tests, real gRPC client, and wiring into
Server.createCacheClient.No writes —
IntentWriterstays a no-op (noopIntentWriter, composed viaNewConfigServerClient) until the next ticket in the stack.Cross-repo dependency
Depends on
sdcio/sdc-protos#134, a fresh rebase of the danglingconfig-server-cache-backendbranch'sconfig_read.proto/ConfigReadServicecommit onto the tip of
sdcio/sdc-protos#123(the sensitive-flag rebasesdcio/data-server#493already pinned to).go.modis pinned to thatcommit; swap for a tagged
sdc-protosrelease once#123/#134merge.What shipped
configserver.LocalConfigReaderseam (Get/List) +FakeLocalConfigReaderfor tests,
Document/Target/ConfigBlobtypes per the ADR's field-mappingtable.
ConfigServerCache(pkg/cache/configserver.go): real-intent reads gothrough the seam and wrap into
importer.IntentAdapter; writes areunconditional no-ops;
runningis served from its own independentin-memory, per-instance store, entirely separate from the seam.
GRPCConfigReader: real seam implementation over the generatedconfig_read.ConfigReadServiceClient, dialed byServer.createConfigServerCacheClient.Cache.Type: config-servernow requires onlyAddress(Namespaceconfigfield deleted) —
ConfigServerCachederives the per-call targetnamespace/name from the datastore name itself (
splitDatastoreName),correctly serving datastores across multiple Kubernetes namespaces from one
process.
<namespace>.<name>), matching howconfig-server keys
TransactionSetowners, with a lookup-side strip(
lookupConfigName) soGet/Existsstill work off either form.mergeConfigBlobsnow handles config-server's root-path ("/") blob shapeused by
LoadAllButRunningIntents/revert-after-sync.Adapted from the source branch
IntentAdapter/ImportConfigAdaptersplit andnoopIntentWritercomposition (both fromthe prior
cache-interface-prepticket), rather than the singleImportConfigAdaptershape / concrete-ConfigServerCache-implements-`IntentWriter shape some of the original commits were written against.
InstanceCreateseedsrunningwith an empty
&tree_persist.Intent{}(not nil) from the start, andInstanceRunningGethas noErrRunningNotFound/nil-guard — built incorrectly rather than reintroduced-then-fixed.
(
706ecb6,2e8ffc6): both renameConfigReadService→ConfigSnapshotServiceand touch a
pkg/cache/configsnapshot/package that belongs to the nextticket's write-backend work (
ConfigSnapshotServicegainsModify/Deletethere) — premature here, where the service is still read-only
ConfigReadService.8bebc69(GetIntent redaction-scope doc comment): stale relative tothe already-shipped always-union hardening from the sensitive-redaction
ticket; the current comments already describe that behavior correctly.
.scratch/planning artifacts and doc-only "mark ticket done"commits, consistent with prior tickets in this stack.
Verification
go build ./...,go vet ./...,go test ./...(full repo), andgolangci-lint run ./...all pass.