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

Add example config TOML #871

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

sondavidb
Copy link
Contributor

@sondavidb sondavidb commented Oct 16, 2023

Issue #, if available:
#758

Description of changes:
Added an example TOML in config/config.toml documenting most of the available variables that can be changed in the TOML file. I omitted some variables that are known to be unused (particularly the Config struct in in service/plugin/plugin.go), but there are probably still some variables in this example that have no effect. If any of them are known to do nothing, they can be changed in a later PR.

I additionally left some comments attempting to clarify what certain variables do. For example, MaxWaitMsec=0 under [http] will actually set the variable to the default value stored in the codebase for MaxWaitMsec, which in this case would be 300,000. This makes sense in this context (as waiting 0ms would never allow it to happen), but for something like min_layer_size=0 under [snapshotter], where a user might want no minimum layer size, it could cause some confusion. I did my best to outline this in the comments, but perhaps this confusion could be cleared in the future (e.g. by defaulting numeric values to -1 instead of 0).

Testing performed:
Created a new integration test in run_test.go called TestRunWithDefaultConfig, which creates a snapshotter base shell and reboots containerd with the example TOML. If it cannot parse the example TOML, it fails.

I also verified the TOML structure by getting the config and marshalling it with gotoml (right before line 178 in cmd/soci-snapshotter-grps/main.go here), then printing to stdout.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sondavidb
Copy link
Contributor Author

Created #872 to try and clean up TOML variable usage, as it's pretty unclear what is and is not actually used.

@sondavidb sondavidb force-pushed the add-example-config branch 3 times, most recently from d22fd87 to 9e50419 Compare October 19, 2023 23:31
Signed-off-by: David Son <[email protected]>
@sondavidb
Copy link
Contributor Author

After making some changes I realized that disable_verification being forced in the function header for getSnapshotterConfigToml causes TestRunWithDefaultConfig to fail. I Just commented it out in the example and left a comment that it can be enabled when used in /etc/soci-snapshotter-grpc-config.toml.

Perhaps another case to prioritize #851 a bit more?

@turan18
Copy link
Contributor

turan18 commented Oct 23, 2023

LGTM. I do think we need a config.md that describes what each config does, as well as other nuanced things like the fact that 0 is used to as a placeholder for some configs.

Similar to https://github.com/containerd/containerd/blob/main/docs/man/containerd-config.toml.5.md.

@sondavidb sondavidb merged commit 114d5dd into awslabs:main Oct 23, 2023
@sondavidb
Copy link
Contributor Author

Created #884 to properly document this change

I noted this already but for greater transparency — this implementation has one bug I managed to do a workaround for. TestRunWithDefaultConfig cannot run unless I comment out the disable_verification variable because otherwise it believes it has been assigned multiple times. Not sure exactly how to fix it elegantly but this is worth noting since it could be a little confusing.

I also quite frankly don't like how I implemented the test (copying the file from x location) but as the alternative is maintaining two identical copies of the same logic I suppose this is the best way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants