feat: add basic mem store boot-time initialization#103
Conversation
nbhosle
left a comment
nbhosle
left a comment
There was a problem hiding this comment.
Suggested a helper function, to keep the code simplified and easy to read.
Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
Add support for a MEM_PATH envvar. When set, server startup expects groups.yaml, instances.yaml, and clusterdefaults.yaml files under the path from the variable, and pre-populates the memory store with their contents. Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
rainest
force-pushed
the
rainest/mem-from-file
branch
from
fc5fcdd to
7ccbf26
Compare
|
Force push to incorporate the unrelated Go version bump and golangci-lint version bump, as well as incorporate some lint fixes in the test. |
nbhosle
left a comment
nbhosle
left a comment
There was a problem hiding this comment.
Thanks for the context, @rainest. I understand wanting to keep the flow flat for these three files.
However, I've marked this as 'Request Changes' because the current error handling in NewMemStoreFromPath returns the store object alongside an error. This is risky as the caller might attempt to use a partially initialized store if they aren't careful.
If we're sticking with the flat structure, please update those blocks to return nil, err and use the full Path variables in the error strings to make debugging easier.
| } | ||
| err = yaml.Unmarshal(groups, &store.Groups) | ||
| if err != nil { | ||
| return store, fmt.Errorf("error unmarshaling groups.yaml: %w", err) |
There was a problem hiding this comment.
Is this intended to return store object along with an Error? Returning store object on failure is a bit risky for the caller.
Following make more sense, which returns nil along with the error message that including the full path of the file.
return store, fmt.Errorf("error unmarshaling %q: %w", groupsPath, err)2 participants
Description
Provide the initial contents of a memstore from a set of files, using a basic "unmarshal YAML into the structs" strategy. Adds YAML tags matching existing JSON tags.
The newer metadata-service storage system doesn't allow as much direct control over its storage, and adding equivalent functionality upstream would affect other services, so adding it here lets me provide a per-repo implementation.
My manual version of the unit test is to deploy the
local.yamlin example_ci_mem.tar.gz and then create a map to mount:Port-forwarding and
curl -sv http://localhost:27777/admin/groups/computeand so on will then return the loaded resources.Design thoughts in lieu of a proper issue. tl;dr is that this tries to avoid much new UX surface, while still providing a means for static file configuration for test scenarios:
MEM_PATHmirrorsDB_PATHand serves as a de facto switch: it defaults to empty string, and if not setmemretains its existing, empty on start behavior.The limits of using files per resource still work okay for making it easier to reproduce E2E test scenarios (did a set of deploy manifests correctly hook up SMD and cloud-init, does cloud-init serve something to a node in one or more groups, etc.) and don't require any additional types, however, so it largely avoids new UX surface that we'd need to consider for breaking changes.
This still allows room for expansion via cascading configuration in the directory, e.g. by placing a
fancier.yamlin the directory and loading configuration from that rather than the basic resource files.Type of Change
Checklist
make test(or equivalent) locally and all tests passgit commit -s) with my real name and email<filename>.licensesidecarLICENSES/directoryCurrently nothing in this repo has the SPDX stuff, so I did not make the files here the odd ones out.