From 7cd25362c37aab2b5d518829b1a999c1dbccbec3 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Tue, 14 Jan 2025 10:39:03 +0100 Subject: [PATCH 1/4] Use only the path as parameter to LoadRuntimeConfig This is the only information needed. Passing just the path instead of the full CfgVars struct makes it easier to reason about what is happening. Signed-off-by: Tom Wieczorek --- pkg/config/cli.go | 2 +- pkg/config/runtime.go | 6 +++--- pkg/config/runtime_test.go | 20 ++++++-------------- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/pkg/config/cli.go b/pkg/config/cli.go index 874e32149cf6..0dcedda855e6 100644 --- a/pkg/config/cli.go +++ b/pkg/config/cli.go @@ -302,7 +302,7 @@ func GetCmdOpts(cobraCmd command) (*CLIOptions, error) { } // if a runtime config can be loaded, use it to override the k0sVars - if rtc, err := LoadRuntimeConfig(k0sVars); err == nil { + if rtc, err := LoadRuntimeConfig(k0sVars.RuntimeConfigPath); err == nil { k0sVars = rtc.K0sVars } diff --git a/pkg/config/runtime.go b/pkg/config/runtime.go index 0f8ed1aef3da..1de9fc845b6a 100644 --- a/pkg/config/runtime.go +++ b/pkg/config/runtime.go @@ -59,8 +59,8 @@ type RuntimeConfigSpec struct { Pid int `json:"pid"` } -func LoadRuntimeConfig(k0sVars *CfgVars) (*RuntimeConfigSpec, error) { - content, err := os.ReadFile(k0sVars.RuntimeConfigPath) +func LoadRuntimeConfig(path string) (*RuntimeConfigSpec, error) { + content, err := os.ReadFile(path) if err != nil { return nil, err } @@ -98,7 +98,7 @@ func LoadRuntimeConfig(k0sVars *CfgVars) (*RuntimeConfigSpec, error) { } func NewRuntimeConfig(k0sVars *CfgVars) (*RuntimeConfigSpec, error) { - if _, err := LoadRuntimeConfig(k0sVars); err == nil { + if _, err := LoadRuntimeConfig(k0sVars.RuntimeConfigPath); err == nil { return nil, ErrK0sAlreadyRunning } diff --git a/pkg/config/runtime_test.go b/pkg/config/runtime_test.go index aaeda0c02fb4..4f1d0e02337e 100644 --- a/pkg/config/runtime_test.go +++ b/pkg/config/runtime_test.go @@ -18,24 +18,17 @@ package config import ( "os" + "path/filepath" "testing" "github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestLoadRuntimeConfig_K0sNotRunning(t *testing.T) { - // create a temporary file for runtime config - tmpfile, err := os.CreateTemp("", "runtime-config") - assert.NoError(t, err) - defer os.Remove(tmpfile.Name()) - - // prepare k0sVars - k0sVars := &CfgVars{ - RuntimeConfigPath: tmpfile.Name(), - } - // write some content to the runtime config file + rtConfigPath := filepath.Join(t.TempDir(), "runtime-config") content := []byte(`--- apiVersion: k0s.k0sproject.io/v1beta1 kind: RuntimeConfig @@ -43,13 +36,12 @@ spec: nodeConfig: metadata: name: k0s - pid: 9999999 + pid: -1 `) - err = os.WriteFile(k0sVars.RuntimeConfigPath, content, 0644) - assert.NoError(t, err) + require.NoError(t, os.WriteFile(rtConfigPath, content, 0644)) // try to load runtime config and check if it returns an error - spec, err := LoadRuntimeConfig(k0sVars) + spec, err := LoadRuntimeConfig(rtConfigPath) assert.Nil(t, spec) assert.ErrorIs(t, err, ErrK0sNotRunning) } From 9f913bf181e70e67046a3b66a83d046b7e587c7f Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Tue, 14 Jan 2025 11:35:32 +0100 Subject: [PATCH 2/4] Remove unused vars variable and DeepCopy The vars variable was never read, and the DeepCopy method was used just to initialize it. Remove both. The original intent was probably not to list the StartupConfigPath in the file written to disk, but since that never worked as intended and there were no problems with it, I think it's okay to leave things as they are. Signed-off-by: Tom Wieczorek --- pkg/config/cfgvars.go | 14 -------------- pkg/config/runtime.go | 5 ----- 2 files changed, 19 deletions(-) diff --git a/pkg/config/cfgvars.go b/pkg/config/cfgvars.go index 024bbbececd1..3c627021e15a 100644 --- a/pkg/config/cfgvars.go +++ b/pkg/config/cfgvars.go @@ -65,20 +65,6 @@ type CfgVars struct { nodeConfig *v1beta1.ClusterConfig } -func (c *CfgVars) DeepCopy() *CfgVars { - if c == nil { - return nil - } - // Make a copy of the original struct, this works because all the fields are - // primitive types - copy := *c - - copy.nodeConfig = c.nodeConfig.DeepCopy() - - // Return the copied struct - return © -} - type CfgVarOption func(*CfgVars) // Command represents cobra.Command diff --git a/pkg/config/runtime.go b/pkg/config/runtime.go index 1de9fc845b6a..a94c67f8869f 100644 --- a/pkg/config/runtime.go +++ b/pkg/config/runtime.go @@ -107,11 +107,6 @@ func NewRuntimeConfig(k0sVars *CfgVars) (*RuntimeConfigSpec, error) { return nil, fmt.Errorf("load node config: %w", err) } - vars := k0sVars.DeepCopy() - - // don't persist the startup config path in the runtime config - vars.StartupConfigPath = "" - cfg := &RuntimeConfig{ ObjectMeta: metav1.ObjectMeta{ CreationTimestamp: metav1.Now(), From 14fa4c604c44ff138533d0c7c0c95aba2af86fc1 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Tue, 14 Jan 2025 12:25:56 +0100 Subject: [PATCH 3/4] Restructure NewRuntimeConfig test Use a real config file instead of mocking out the config file loading using the private nodeConfig field. Verify that the folder for the runtime config file is created, if it doesn't exist. Signed-off-by: Tom Wieczorek --- pkg/config/runtime_test.go | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/pkg/config/runtime_test.go b/pkg/config/runtime_test.go index 4f1d0e02337e..390a534d6eb1 100644 --- a/pkg/config/runtime_test.go +++ b/pkg/config/runtime_test.go @@ -24,6 +24,7 @@ import ( "github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sigs.k8s.io/yaml" ) func TestLoadRuntimeConfig_K0sNotRunning(t *testing.T) { @@ -47,38 +48,38 @@ spec: } func TestNewRuntimeConfig(t *testing.T) { - // create a temporary directory for k0s files - tempDir, err := os.MkdirTemp("", "k0s") - assert.NoError(t, err) - defer os.RemoveAll(tempDir) + // Create regular configuration file + tempDir := t.TempDir() + startupConfigPath := filepath.Join(tempDir, "startup-config") + startupConfig, err := yaml.Marshal(&v1beta1.ClusterConfig{ + Spec: &v1beta1.ClusterSpec{ + API: &v1beta1.APISpec{Address: "10.0.0.1"}, + }, + }) + require.NoError(t, err) + require.NoError(t, os.WriteFile(startupConfigPath, startupConfig, 0644)) - // create a temporary file for the runtime config - tmpfile, err := os.CreateTemp("", "runtime-config") - assert.NoError(t, err) - tmpfile.Close() - defer os.Remove(tmpfile.Name()) + // Define runtime configuration file in a not yet existing directory + rtConfigPath := filepath.Join(tempDir, "runtime", "config") // prepare k0sVars k0sVars := &CfgVars{ - RuntimeConfigPath: tmpfile.Name(), + StartupConfigPath: startupConfigPath, + RuntimeConfigPath: rtConfigPath, DataDir: tempDir, - nodeConfig: &v1beta1.ClusterConfig{ - Spec: &v1beta1.ClusterSpec{ - API: &v1beta1.APISpec{Address: "10.0.0.1"}, - }, - }, } // create a new runtime config and check if it's valid spec, err := NewRuntimeConfig(k0sVars) assert.NoError(t, err) assert.NotNil(t, spec) - assert.Equal(t, tempDir, spec.K0sVars.DataDir) + assert.Same(t, k0sVars, spec.K0sVars) assert.Equal(t, os.Getpid(), spec.Pid) assert.NotNil(t, spec.NodeConfig) cfg, err := spec.K0sVars.NodeConfig() assert.NoError(t, err) assert.Equal(t, "10.0.0.1", cfg.Spec.API.Address) + assert.FileExists(t, rtConfigPath) // try to create a new runtime config when one is already active and check if it returns an error _, err = NewRuntimeConfig(k0sVars) From 48c01db459f0794b9fff03c746c26497fb8810ff Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Tue, 14 Jan 2025 13:02:54 +0100 Subject: [PATCH 4/4] Don't cache the node config in the CfgVars struct The node config should be read at most once and then reused. Caching the config in this way shouldn't be necessary. Signed-off-by: Tom Wieczorek --- pkg/config/cfgvars.go | 13 +------------ pkg/config/cfgvars_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/pkg/config/cfgvars.go b/pkg/config/cfgvars.go index 3c627021e15a..b21bc2181878 100644 --- a/pkg/config/cfgvars.go +++ b/pkg/config/cfgvars.go @@ -61,8 +61,7 @@ type CfgVars struct { HelmRepositoryCache string HelmRepositoryConfig string - stdin io.Reader - nodeConfig *v1beta1.ClusterConfig + stdin io.Reader } type CfgVarOption func(*CfgVars) @@ -109,10 +108,6 @@ func WithCommand(cmd command) CfgVarOption { } } -func (c *CfgVars) SetNodeConfig(cfg *v1beta1.ClusterConfig) { - c.nodeConfig = cfg -} - func DefaultCfgVars() *CfgVars { vars, _ := NewCfgVars(nil) return vars @@ -219,10 +214,6 @@ func (c *CfgVars) defaultStorageSpec() *v1beta1.StorageSpec { var defaultConfigPath = constant.K0sConfigPathDefault func (c *CfgVars) NodeConfig() (*v1beta1.ClusterConfig, error) { - if c.nodeConfig != nil { - return c.nodeConfig, nil - } - if c.StartupConfigPath == "" { return nil, errors.New("config path is not set") } @@ -260,7 +251,5 @@ func (c *CfgVars) NodeConfig() (*v1beta1.ClusterConfig, error) { nodeConfig.Spec.Storage.Kine = v1beta1.DefaultKineConfig(c.DataDir) } - c.nodeConfig = nodeConfig - return nodeConfig, nil } diff --git a/pkg/config/cfgvars_test.go b/pkg/config/cfgvars_test.go index a785712a5f3c..326fff93e69c 100644 --- a/pkg/config/cfgvars_test.go +++ b/pkg/config/cfgvars_test.go @@ -222,10 +222,10 @@ func TestNodeConfig_Stdin(t *testing.T) { require.NoError(t, err) nodeConfig, err := underTest.NodeConfig() - require.NoError(t, err) + assert.NoError(t, err) assert.Equal(t, "calico", nodeConfig.Spec.Network.Provider) - nodeConfig2, err := underTest.NodeConfig() - require.NoError(t, err) - assert.Same(t, nodeConfig, nodeConfig2) + nodeConfig, err = underTest.NodeConfig() + assert.ErrorContains(t, err, "stdin already grabbed") + assert.Nil(t, nodeConfig) }