Add support for ignite configuration file #601

darkowlzz · 2020-05-31T22:29:21Z

This adds a new API type Configuration which is used to define the
default configurations of ignite. A Configuration object can be defined
in a config file as:

apiVersion: ignite.weave.works/v1alpha3
kind: Configuration
metadata:
  name: test-config
spec:
  runtime: containerd
  networkPlugin: cni
  vmDefaults:
    memory: "2GB"
    diskSize: "3GB"
    cpus: 2
    sandbox:
      oci: weaveworks/ignite:dev
    kernel:
      oci: weaveworks/ignite-kernel:4.19.47
    ssh: true

By default, ignite looks for the config file at /etc/ignite/config.yaml.
If this file is found, ignite uses it to configure the ignite defaults.
--ignite-config flag can be used to explicitly pass a configuration
file.

Changes the flag override behavior when using a VM config file. The flags are no longer ignored and override the VM configuration.
Debug log for the configuration used:
Default config:

$ sudo bin/ignite run weaveworks/ignite-ubuntu --name my-vm --ssh --log-level=debug
DEBU[0000] Using ignite default configurations

With global config:

$ sudo bin/ignite run weaveworks/ignite-ubuntu --name my-vm --ssh --log-level=debug
DEBU[0000] Found default ignite configuration file /etc/ignite/config.yaml
DEBU[0000] Using ignite configuration file /etc/ignite/config.yaml

Adds e2e tests to verify the behavior.
Adds new document docs/ignite-configuration.md with examples of the ignite configuration.

Related to #315

luxas · 2020-06-11T11:01:19Z

I'd add this to a new v1alpha3 API version, in order to not break v1alpha2 compatibility. ref: #355

darkowlzz · 2020-06-28T17:03:34Z

Rebased on v1alpha3 API.

cmd/ignite/cmd/root.go

twelho

LGTM 👍 some structural change requests, but overall this is looking very good

cmd/ignite/cmd/root.go

cmd/ignite/run/create.go

cmd/ignite/run/create_test.go

twelho · 2020-06-29T15:44:42Z

cmd/ignite/cmd/root.go

@@ -115,9 +153,27 @@ func addGlobalFlags(fs *pflag.FlagSet) {
 logflag.LogLevelFlagVar(fs, &logLevel)
 runtimeflag.RuntimeVar(fs, &providers.RuntimeName)
 networkflag.NetworkPluginVar(fs, &providers.NetworkPluginName)
+ fs.StringVar(&configPath, "ignite-config", "", "Ignite configuration path")


More descriptive note on what this flag does

I've added a document about this in docs/ignite-configuration.md. I hope this will make it more clear.
Should this still be more descriptive?

pkg/apis/ignite/v1alpha3/types.go

twelho · 2020-06-29T16:19:03Z

cmd/ignite/cmd/root.go

+ }
+ }
+
+ if configFilePath != "" {


Also handle the config for ignited?

Maybe in a follow-up PR?

e2e/config_test.go

This adds a new API type `Configuration` which is used to define the default configurations of ignite. A Configuration object can be defined in a config file as: ```yaml apiVersion: ignite.weave.works/v1alpha3 kind: Configuration metadata: name: test-config spec: runtime: containerd networkPlugin: cni vmDefaults: memory: "2GB" diskSize: "3GB" cpus: 2 sandbox: oci: weaveworks/ignite:dev kernel: oci: weaveworks/ignite-kernel:4.19.47 ssh: true ``` By default, ignite looks for the config file at /etc/ignite/config.yaml. If this file is found, ignite uses it to configure the ignite defaults. `--ignite-config` flag can be used to explicitly pass a configuration file.

- Flag overrides are applied on VM even if a VM config file is provided.

twelho · 2020-07-06T15:24:18Z

cmd/ignite/run/create_test.go

- err: true,
- },
- {
- name: "with sandbox image",


This is already tested in e2e so it's fine to exclude it here (tricky to implement).

twelho · 2020-07-06T15:26:09Z

cmd/ignite/run/create_test.go

 wantSSH: api.SSH{
 Generate: true,
 PublicKey: "some-pub-key",
 },
 },
- {
- name: "with no VM name and --require-name flag set",


This was tested twice, check https://github.com/weaveworks/ignite/pull/601/files#diff-d0bf10b7da8bd0e055593597d507f004R306

twelho · 2020-07-06T15:30:16Z

cmd/ignite/run/create_test.go

 }

 for _, rt := range tests {
 t.Run(rt.name, func(t *testing.T) {
- err := rt.createFlag.constructVMFromCLI(rt.args)
+ vm := rt.createFlag.VM
+ fs := flag.NewFlagSet("test", flag.ExitOnError)


This does not test flag validity since that would need the flag set to be populated (needs additional logic), but the flag handling will need to be refactored at some point anyway so this is fine.

twelho · 2020-07-06T15:35:48Z

cmd/ignite/run/create_test.go

+ if err != nil {
+ t.Fatalf("failed to create storage for ignite: %v", err)
+ }
+ defer os.RemoveAll(dir)


Catch this error with util.DeferErr (or assert) (will be done in a separate PR)

twelho · 2020-07-06T15:38:39Z

cmd/ignite/run/create_test.go

+ }
+ defer os.RemoveAll(dir)
+
+ storage := cache.NewCache(


This should use Ignite's configured providers to have consistency between the tests and built binary, can be done in a separate PR.

make api-docs

twelho

LGTM 👍 amazing work 💯

stealthybox

🚢 🚀

darkowlzz requested a review from stealthybox as a code owner May 31, 2020 22:29

darkowlzz force-pushed the componentconfig branch from 7902818 to 4b897ce Compare June 1, 2020 06:58

darkowlzz added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 5, 2020

darkowlzz force-pushed the componentconfig branch 3 times, most recently from d6d08d7 to b5ab200 Compare June 6, 2020 12:22

darkowlzz mentioned this pull request Jun 8, 2020

[POC] Add ignite configuration file support #555

Closed

luxas mentioned this pull request Jun 11, 2020

Remove v1alpha1 in v0.8.0 #355

Closed

darkowlzz force-pushed the componentconfig branch from b5ab200 to 59ac3bc Compare June 15, 2020 13:46

darkowlzz added the do-not-merge/wip The PR is still work in progress label Jun 15, 2020

darkowlzz force-pushed the componentconfig branch 2 times, most recently from 89cbaeb to c05c2cc Compare June 28, 2020 16:52

darkowlzz removed the do-not-merge/wip The PR is still work in progress label Jun 28, 2020

twelho reviewed Jun 29, 2020

View reviewed changes

cmd/ignite/cmd/root.go Show resolved Hide resolved

stealthybox reviewed Jun 29, 2020

View reviewed changes

cmd/ignite/cmd/root.go Outdated Show resolved Hide resolved

twelho reviewed Jun 29, 2020

View reviewed changes

darkowlzz added 2 commits July 6, 2020 00:15

Add e2e test for ignite configuration

97d85f4

darkowlzz force-pushed the componentconfig branch from 0f9bea6 to 241c223 Compare July 5, 2020 18:46

darkowlzz added 2 commits July 6, 2020 00:18

Fix flag overrides

d1d0036

- Flag overrides are applied on VM even if a VM config file is provided.

Add more unit test and improve e2e tests

2da7ffe

darkowlzz force-pushed the componentconfig branch 2 times, most recently from c3fa727 to 96dc1f6 Compare July 6, 2020 11:16

Add docs/ignite-configuration.md

4679153

darkowlzz force-pushed the componentconfig branch from 96dc1f6 to 4679153 Compare July 6, 2020 11:25

twelho reviewed Jul 6, 2020

View reviewed changes

Generate docs

3a37adb

make api-docs

twelho approved these changes Jul 6, 2020

View reviewed changes

stealthybox approved these changes Jul 6, 2020

View reviewed changes

stealthybox merged commit 52a0fce into weaveworks:master Jul 6, 2020

darkowlzz deleted the componentconfig branch July 6, 2020 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ignite configuration file #601

Add support for ignite configuration file #601

darkowlzz commented May 31, 2020 •

edited

Loading

luxas commented Jun 11, 2020

darkowlzz commented Jun 28, 2020

twelho left a comment

twelho Jun 29, 2020

darkowlzz Jul 6, 2020

twelho Jun 29, 2020

darkowlzz Jul 5, 2020

twelho Jul 6, 2020

twelho Jul 6, 2020

twelho Jul 6, 2020

twelho Jul 6, 2020 •

edited

Loading

twelho Jul 6, 2020

twelho left a comment

stealthybox left a comment

Add support for ignite configuration file #601

Add support for ignite configuration file #601

Conversation

darkowlzz commented May 31, 2020 • edited Loading

luxas commented Jun 11, 2020

darkowlzz commented Jun 28, 2020

twelho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twelho Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twelho left a comment

Choose a reason for hiding this comment

stealthybox left a comment

Choose a reason for hiding this comment

darkowlzz commented May 31, 2020 •

edited

Loading

twelho Jul 6, 2020 •

edited

Loading