diff --git a/pkg/apis/machineconfiguration.openshift.io/v1/types.go b/pkg/apis/machineconfiguration.openshift.io/v1/types.go index 8cfa3ba29d..d2e83ef035 100644 --- a/pkg/apis/machineconfiguration.openshift.io/v1/types.go +++ b/pkg/apis/machineconfiguration.openshift.io/v1/types.go @@ -484,11 +484,11 @@ type ContainerRuntimeConfiguration struct { // logSizeMax specifies the Maximum size allowed for the container log file. // Negative numbers indicate that no size limit is imposed. // If it is positive, it must be >= 8192 to match/exceed conmon's read buffer. - LogSizeMax resource.Quantity `json:"logSizeMax,omitempty"` + LogSizeMax *resource.Quantity `json:"logSizeMax,omitempty"` // overlaySize specifies the maximum size of a container image. // This flag can be used to set quota on the size of container images. - OverlaySize resource.Quantity `json:"overlaySize,omitempty"` + OverlaySize *resource.Quantity `json:"overlaySize,omitempty"` // defaultRuntime is the name of the OCI runtime to be used as the default. DefaultRuntime ContainerRuntimeDefaultRuntime `json:"defaultRuntime,omitempty"` diff --git a/pkg/apis/machineconfiguration.openshift.io/v1/zz_generated.deepcopy.go b/pkg/apis/machineconfiguration.openshift.io/v1/zz_generated.deepcopy.go index 1e34177448..a43328eb02 100644 --- a/pkg/apis/machineconfiguration.openshift.io/v1/zz_generated.deepcopy.go +++ b/pkg/apis/machineconfiguration.openshift.io/v1/zz_generated.deepcopy.go @@ -148,8 +148,16 @@ func (in *ContainerRuntimeConfiguration) DeepCopyInto(out *ContainerRuntimeConfi *out = new(int64) **out = **in } - out.LogSizeMax = in.LogSizeMax.DeepCopy() - out.OverlaySize = in.OverlaySize.DeepCopy() + if in.LogSizeMax != nil { + in, out := &in.LogSizeMax, &out.LogSizeMax + x := (*in).DeepCopy() + *out = &x + } + if in.OverlaySize != nil { + in, out := &in.OverlaySize, &out.OverlaySize + x := (*in).DeepCopy() + *out = &x + } return } diff --git a/pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go b/pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go index 7d7c437c28..1a70827093 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go @@ -39,7 +39,7 @@ func RunContainerRuntimeBootstrap(templateDir string, crconfigs []*mcfgv1.Contai var configFileList []generatedConfigFile ctrcfg := cfg.Spec.ContainerRuntimeConfig - if !ctrcfg.OverlaySize.IsZero() { + if ctrcfg.OverlaySize != nil && !ctrcfg.OverlaySize.IsZero() { storageTOML, err := mergeConfigChanges(originalStorageIgn, cfg, updateStorageConfig) if err != nil { glog.V(2).Infoln(cfg, err, "error merging user changes to storage.conf: %v", err) @@ -48,7 +48,7 @@ func RunContainerRuntimeBootstrap(templateDir string, crconfigs []*mcfgv1.Contai } } // Create the cri-o drop-in files - if ctrcfg.LogLevel != "" || ctrcfg.PidsLimit != nil || !ctrcfg.LogSizeMax.IsZero() || ctrcfg.DefaultRuntime != mcfgv1.ContainerRuntimeDefaultRuntimeEmpty { + if ctrcfg.LogLevel != "" || ctrcfg.PidsLimit != nil || (ctrcfg.LogSizeMax != nil && !ctrcfg.LogSizeMax.IsZero()) || ctrcfg.DefaultRuntime != mcfgv1.ContainerRuntimeDefaultRuntimeEmpty { crioFileConfigs := createCRIODropinFiles(cfg) configFileList = append(configFileList, crioFileConfigs...) } diff --git a/pkg/controller/container-runtime-config/container_runtime_config_controller.go b/pkg/controller/container-runtime-config/container_runtime_config_controller.go index fc88cf6b12..13d3d8231e 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_controller.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_controller.go @@ -604,7 +604,7 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error { var configFileList []generatedConfigFile ctrcfg := cfg.Spec.ContainerRuntimeConfig - if !ctrcfg.OverlaySize.IsZero() { + if ctrcfg.OverlaySize != nil && !ctrcfg.OverlaySize.IsZero() { storageTOML, err := mergeConfigChanges(originalStorageIgn, cfg, updateStorageConfig) if err != nil { glog.V(2).Infoln(cfg, err, "error merging user changes to storage.conf: %v", err) @@ -616,7 +616,7 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error { } // Create the cri-o drop-in files - if ctrcfg.LogLevel != "" || ctrcfg.PidsLimit != nil || !ctrcfg.LogSizeMax.IsZero() || ctrcfg.DefaultRuntime != mcfgv1.ContainerRuntimeDefaultRuntimeEmpty { + if ctrcfg.LogLevel != "" || ctrcfg.PidsLimit != nil || (ctrcfg.LogSizeMax != nil && !ctrcfg.LogSizeMax.IsZero()) || ctrcfg.DefaultRuntime != mcfgv1.ContainerRuntimeDefaultRuntimeEmpty { crioFileConfigs := createCRIODropinFiles(cfg) configFileList = append(configFileList, crioFileConfigs...) } diff --git a/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go b/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go index 81eb27141a..bf8f33b3ef 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go @@ -9,6 +9,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" + "github.com/clarketm/json" "github.com/golang/glog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -480,10 +481,13 @@ func TestContainerRuntimeConfigCreate(t *testing.T) { f := newFixture(t) f.newController() + nine := resource.MustParse("9k") + three := resource.MustParse("3G") + cc := newControllerConfig(ctrlcommon.ControllerConfigName, platform) mcp := helpers.NewMachineConfigPool("master", nil, helpers.MasterSelector, "v0") mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0") - ctrcfg1 := newContainerRuntimeConfig("set-log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug", LogSizeMax: resource.MustParse("9k"), OverlaySize: resource.MustParse("3G")}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) + ctrcfg1 := newContainerRuntimeConfig("set-log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug", LogSizeMax: &nine, OverlaySize: &three}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) ctrCfgKey, _ := getManagedKeyCtrCfg(mcp, f.client, ctrcfg1) mcs1 := helpers.NewMachineConfig(getManagedKeyCtrCfgDeprecated(mcp), map[string]string{"node-role": "master"}, "dummy://", []ign3types.File{{}}) mcs2 := mcs1.DeepCopy() @@ -517,10 +521,13 @@ func TestContainerRuntimeConfigUpdate(t *testing.T) { f := newFixture(t) f.newController() + nine := resource.MustParse("9k") + three := resource.MustParse("3G") + cc := newControllerConfig(ctrlcommon.ControllerConfigName, platform) mcp := helpers.NewMachineConfigPool("master", nil, helpers.MasterSelector, "v0") mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0") - ctrcfg1 := newContainerRuntimeConfig("set-log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug", LogSizeMax: resource.MustParse("9k"), OverlaySize: resource.MustParse("3G")}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) + ctrcfg1 := newContainerRuntimeConfig("set-log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug", LogSizeMax: &nine, OverlaySize: &three}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) keyCtrCfg, _ := getManagedKeyCtrCfg(mcp, f.client, ctrcfg1) mcs := helpers.NewMachineConfig(getManagedKeyCtrCfgDeprecated(mcp), map[string]string{"node-role": "master"}, "dummy://", []ign3types.File{{}}) mcsUpdate := mcs.DeepCopy() @@ -1221,6 +1228,8 @@ func TestContainerRuntimeConfigOptions(t *testing.T) { validPidsLimit int64 = 2048 validZerolimit int64 = 0 invalidNegLimit int64 = -10 + three = resource.MustParse("3k") + ten = resource.MustParse("10k") ) failureTests := []struct { name string @@ -1241,7 +1250,7 @@ func TestContainerRuntimeConfigOptions(t *testing.T) { { name: "inalid value of max log size", config: &mcfgv1.ContainerRuntimeConfiguration{ - LogSizeMax: resource.MustParse("3k"), + LogSizeMax: &three, }, }, { @@ -1277,7 +1286,7 @@ func TestContainerRuntimeConfigOptions(t *testing.T) { { name: "valid max log size", config: &mcfgv1.ContainerRuntimeConfiguration{ - LogSizeMax: resource.MustParse("10k"), + LogSizeMax: &ten, }, }, { @@ -1313,6 +1322,74 @@ func TestContainerRuntimeConfigOptions(t *testing.T) { } } +func TestMarshalResourceQuantityOptionsJSON(t *testing.T) { + var ( + validLogSizeMax = resource.MustParse("10k") + validOverlaySize = resource.MustParse("10G") + ) + + emptyValueTests := []struct { + name string + config *mcfgv1.ContainerRuntimeConfiguration + }{ + { + name: "valid log level, overlaySize/logsizeMax should not appear in json", + config: &mcfgv1.ContainerRuntimeConfiguration{ + LogLevel: "debug", + }, + }, + { + name: "valid value of default runtimeļ¼Œ overlaySize/logsizeMax should not appear in json", + config: &mcfgv1.ContainerRuntimeConfiguration{ + DefaultRuntime: "crun", + }, + }, + } + + successTests := []struct { + name string + config *mcfgv1.ContainerRuntimeConfiguration + expectStr string + }{ + { + name: "valid max log size should appear in json", + config: &mcfgv1.ContainerRuntimeConfiguration{ + LogSizeMax: &validLogSizeMax, + LogLevel: "debug", + }, + expectStr: "\"logSizeMax\":\"10k\"", + }, + { + name: "valid max overlay size should appear in json", + config: &mcfgv1.ContainerRuntimeConfiguration{ + OverlaySize: &validOverlaySize, + LogLevel: "debug", + }, + expectStr: "\"overlaySize\":\"10G\"", + }, + } + + // Successful Tests + for _, test := range successTests { + ctrcfg := newContainerRuntimeConfig(test.name, test.config, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "", "")) + data, err := json.Marshal(ctrcfg) + if err != nil { + t.Errorf("%s: failed with %v. should have succeeded", test.name, err) + } + require.Contains(t, string(data), test.expectStr) + } + + for _, test := range emptyValueTests { + ctrcfg := newContainerRuntimeConfig(test.name, test.config, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "", "")) + data, err := json.Marshal(ctrcfg) + if err != nil { + t.Errorf("%s: failed with %v. should have succeeded", test.name, err) + } + require.NotContains(t, string(data), "\"overlaySize\"", "\"overlaySize\"") + require.NotContains(t, string(data), "\"logSizeMax\"", "\"logSizeMax\"") + } +} + func getKey(config *mcfgv1.ContainerRuntimeConfig, t *testing.T) string { key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(config) if err != nil { diff --git a/pkg/controller/container-runtime-config/helpers.go b/pkg/controller/container-runtime-config/helpers.go index cc03a98034..92010750b8 100644 --- a/pkg/controller/container-runtime-config/helpers.go +++ b/pkg/controller/container-runtime-config/helpers.go @@ -294,12 +294,14 @@ func updateStorageConfig(data []byte, internal *mcfgv1.ContainerRuntimeConfigura return nil, fmt.Errorf("error decoding crio config: %w", err) } - if internal.OverlaySize.Value() < 0 { - return nil, fmt.Errorf("invalid overlaySize config %q: the overlaySize should be larger than 0", internal.OverlaySize.String()) - } + if internal.OverlaySize != nil { + if internal.OverlaySize.Value() < 0 { + return nil, fmt.Errorf("invalid overlaySize config %q: the overlaySize should be larger than 0", internal.OverlaySize.String()) + } - if internal.OverlaySize.Value() != 0 { - tomlConf.Storage.Options.Size = internal.OverlaySize.String() + if internal.OverlaySize.Value() != 0 { + tomlConf.Storage.Options.Size = internal.OverlaySize.String() + } } var newData bytes.Buffer @@ -347,7 +349,7 @@ func createCRIODropinFiles(cfg *mcfgv1.ContainerRuntimeConfig) []generatedConfig glog.V(2).Infoln(cfg, err, "error updating user changes for pids-limit to crio.conf.d: %v", err) } } - if ctrcfg.LogSizeMax.Value() != 0 { + if ctrcfg.LogSizeMax != nil && ctrcfg.LogSizeMax.Value() != 0 { tomlConf := tomlConfigCRIOLogSizeMax{} tomlConf.Crio.Runtime.LogSizeMax = ctrcfg.LogSizeMax.Value() generatedConfigFileList, err = addTOMLgeneratedConfigFile(generatedConfigFileList, crioDropInFilePathLogSizeMax, tomlConf) @@ -487,11 +489,11 @@ func validateUserContainerRuntimeConfig(cfg *mcfgv1.ContainerRuntimeConfig) erro return fmt.Errorf("invalid PidsLimit %v", *ctrcfg.PidsLimit) } - if ctrcfg.LogSizeMax.Value() > 0 && ctrcfg.LogSizeMax.Value() <= minLogSize { + if ctrcfg.LogSizeMax != nil && ctrcfg.LogSizeMax.Value() > 0 && ctrcfg.LogSizeMax.Value() <= minLogSize { return fmt.Errorf("invalid LogSizeMax %q, cannot be less than 8kB", ctrcfg.LogSizeMax.String()) } - if ctrcfg.OverlaySize.Value() < 0 { + if ctrcfg.OverlaySize != nil && ctrcfg.OverlaySize.Value() < 0 { return fmt.Errorf("invalid overlaySize %q, cannot be less than 0", ctrcfg.OverlaySize.String()) } diff --git a/pkg/controller/container-runtime-config/helpers_test.go b/pkg/controller/container-runtime-config/helpers_test.go index 3215193109..1cb0b0f54e 100644 --- a/pkg/controller/container-runtime-config/helpers_test.go +++ b/pkg/controller/container-runtime-config/helpers_test.go @@ -12,10 +12,14 @@ import ( "github.com/containers/image/v5/pkg/sysregistriesv2" signature "github.com/containers/image/v5/signature" "github.com/containers/image/v5/types" + storageconfig "github.com/containers/storage/pkg/config" apicfgv1 "github.com/openshift/api/config/v1" apioperatorsv1alpha1 "github.com/openshift/api/operator/v1alpha1" + mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" ) @@ -866,3 +870,121 @@ func TestGetValidBlockAndAllowedRegistries(t *testing.T) { }) } } + +func TestCreateCRIODropinFiles(t *testing.T) { + zeroLogSizeMax := resource.MustParse("0k") + validLogSizeMax := resource.MustParse("10G") + + // Test zero value of logSizeMax will not be applied + zeroValueTests := []struct { + name string + cfg *mcfgv1.ContainerRuntimeConfiguration + filepath string + }{ + { + name: "01-ctrcfg-logSizeMax will not be created if logSizeMax is zero", + cfg: &mcfgv1.ContainerRuntimeConfiguration{ + LogSizeMax: &zeroLogSizeMax, + }, + filepath: crioDropInFilePathLogSizeMax, + }, + } + + // Test valid value of logSizeMax will be applied to the drop-in file + validValueTests := []struct { + name string + cfg *mcfgv1.ContainerRuntimeConfiguration + filepath string + want []byte + }{ + { + name: "01-ctrcfg-logSizeMax created for valid logSizeMax", + cfg: &mcfgv1.ContainerRuntimeConfiguration{ + LogSizeMax: &validLogSizeMax, + }, + filepath: crioDropInFilePathLogSizeMax, + want: []byte(`[crio] + [crio.runtime] + log_size_max = 10000000000 +`), + }, + } + + for _, test := range zeroValueTests { + ctrcfg := newContainerRuntimeConfig(test.name, test.cfg, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "", "")) + files := createCRIODropinFiles(ctrcfg) + for _, file := range files { + if file.filePath == test.filepath { + t.Errorf("%s: failed. should not have created dropin file", test.name) + } + } + } + + for _, test := range validValueTests { + ctrcfg := newContainerRuntimeConfig(test.name, test.cfg, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "", "")) + files := createCRIODropinFiles(ctrcfg) + for _, file := range files { + if file.filePath == test.filepath { + require.Equal(t, test.want, file.data, "createCRIODropinFiles() Diff, want %v, got %v", test.want, string(file.data)) + } + } + } +} + +func TestUpdateStorageConfig(t *testing.T) { + templateStorageConfig := tomlConfigStorage{} + buf := bytes.Buffer{} + err := toml.NewEncoder(&buf).Encode(templateStorageConfig) + require.NoError(t, err) + templateBytes := buf.Bytes() + + zeroOverLayerSize := resource.MustParse("0k") + validOverLaySize := resource.MustParse("10G") + + tests := []struct { + name string + cfg *mcfgv1.ContainerRuntimeConfiguration + want tomlConfigStorage + }{ + { + name: "not apply zero value of overlaySize", + cfg: &mcfgv1.ContainerRuntimeConfiguration{ + OverlaySize: &zeroOverLayerSize, + }, + want: tomlConfigStorage{}, + }, + { + name: "apply valid overlaySize", + cfg: &mcfgv1.ContainerRuntimeConfiguration{ + OverlaySize: &validOverLaySize, + }, + want: tomlConfigStorage{ + Storage: struct { + Driver string "toml:\"driver\"" + RunRoot string "toml:\"runroot\"" + GraphRoot string "toml:\"graphroot\"" + Options struct{ storageconfig.OptionsConfig } "toml:\"options\"" + }{ + Options: struct{ storageconfig.OptionsConfig }{ + storageconfig.OptionsConfig{ + Size: "10G", + }, + }, + }, + }, + }, + } + + for _, test := range tests { + got, err := updateStorageConfig(templateBytes, test.cfg) + require.NoError(t, err) + gotConf := tomlConfigStorage{} + if _, err := toml.Decode(string(got), &gotConf); err != nil { + t.Errorf("error unmarshalling result: %v", err) + return + } + if !reflect.DeepEqual(gotConf, test.want) { + t.Errorf("%s: failed. got %v, want %v", test.name, got, test.want) + } + } +}