From 0bbdc1569d6aa1ecb4f9a91d8b55fb40e51deadb Mon Sep 17 00:00:00 2001 From: Qi Wang Date: Fri, 21 Jul 2023 18:14:37 -0400 Subject: [PATCH 1/2] OCPBUGS-15934: use *resource.Quantity to not automatically set 0 close: https://issues.redhat.com/browse/OCPBUGS-15934 Signed-off-by: Qi Wang --- .../v1/types.go | 4 +- .../v1/zz_generated.deepcopy.go | 12 ++- .../container_runtime_config_bootstrap.go | 4 +- .../container_runtime_config_controller.go | 4 +- ...ontainer_runtime_config_controller_test.go | 92 +++++++++++++++++-- .../container-runtime-config/helpers.go | 10 +- 6 files changed, 105 insertions(+), 21 deletions(-) diff --git a/pkg/apis/machineconfiguration.openshift.io/v1/types.go b/pkg/apis/machineconfiguration.openshift.io/v1/types.go index 3c780e7d5b..b36c8494ee 100644 --- a/pkg/apis/machineconfiguration.openshift.io/v1/types.go +++ b/pkg/apis/machineconfiguration.openshift.io/v1/types.go @@ -492,11 +492,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 c5d873ca80..bf278b71a6 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go @@ -40,7 +40,7 @@ func RunContainerRuntimeBootstrap(templateDir string, crconfigs []*mcfgv1.Contai var configFileList []generatedConfigFile ctrcfg := cfg.Spec.ContainerRuntimeConfig - if !ctrcfg.OverlaySize.IsZero() { + if ctrcfg.OverlaySize != nil { storageTOML, err := mergeConfigChanges(originalStorageIgn, cfg, updateStorageConfig) if err != nil { klog.V(2).Infoln(cfg, err, "error merging user changes to storage.conf: %v", err) @@ -49,7 +49,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.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 ea7f8570c7..6bf6dba78e 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_controller.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_controller.go @@ -611,7 +611,7 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error { var configFileList []generatedConfigFile ctrcfg := cfg.Spec.ContainerRuntimeConfig - if !ctrcfg.OverlaySize.IsZero() { + if ctrcfg.OverlaySize != nil { storageTOML, err := mergeConfigChanges(originalStorageIgn, cfg, updateStorageConfig) if err != nil { klog.V(2).Infoln(cfg, err, "error merging user changes to storage.conf: %v", err) @@ -623,7 +623,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.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 7b2218027b..5c6b34aac8 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 @@ -2,6 +2,7 @@ package containerruntimeconfig import ( "context" + "encoding/json" "fmt" "reflect" "testing" @@ -494,10 +495,13 @@ func TestContainerRuntimeConfigCreate(t *testing.T) { f := newFixture(t) f.newController() + logSizeMax := resource.MustParse("9k") + overlaySize := 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: &logSizeMax, OverlaySize: &overlaySize}, 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() @@ -531,10 +535,13 @@ func TestContainerRuntimeConfigUpdate(t *testing.T) { f := newFixture(t) f.newController() + logSizeMax := resource.MustParse("9k") + overlaySize := 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: &logSizeMax, OverlaySize: &overlaySize}, 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() @@ -1228,10 +1235,12 @@ func TestRegistriesValidation(t *testing.T) { // for the options in containerruntime config func TestContainerRuntimeConfigOptions(t *testing.T) { var ( - invalidPidsLimit int64 = 10 - validPidsLimit int64 = 2048 - validZerolimit int64 = 0 - invalidNegLimit int64 = -10 + invalidPidsLimit int64 = 10 + validPidsLimit int64 = 2048 + invalidLogSizeMax = resource.MustParse("3k") + validLogSizeMax = resource.MustParse("10k") + validZerolimit int64 = 0 + invalidNegLimit int64 = -10 ) failureTests := []struct { name string @@ -1252,7 +1261,7 @@ func TestContainerRuntimeConfigOptions(t *testing.T) { { name: "inalid value of max log size", config: &mcfgv1.ContainerRuntimeConfiguration{ - LogSizeMax: resource.MustParse("3k"), + LogSizeMax: &invalidLogSizeMax, }, }, { @@ -1288,7 +1297,7 @@ func TestContainerRuntimeConfigOptions(t *testing.T) { { name: "valid max log size", config: &mcfgv1.ContainerRuntimeConfiguration{ - LogSizeMax: resource.MustParse("10k"), + LogSizeMax: &validLogSizeMax, }, }, { @@ -1324,6 +1333,73 @@ 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\"") + } +} + 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 58c632cab8..f1250e63e2 100644 --- a/pkg/controller/container-runtime-config/helpers.go +++ b/pkg/controller/container-runtime-config/helpers.go @@ -294,11 +294,11 @@ func updateStorageConfig(data []byte, internal *mcfgv1.ContainerRuntimeConfigura return nil, fmt.Errorf("error decoding crio config: %w", err) } - if internal.OverlaySize.Value() < 0 { + if internal.OverlaySize != nil && 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 { + if internal.OverlaySize != nil && internal.OverlaySize.Value() != 0 { tomlConf.Storage.Options.Size = internal.OverlaySize.String() } @@ -347,7 +347,7 @@ func createCRIODropinFiles(cfg *mcfgv1.ContainerRuntimeConfig) []generatedConfig klog.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) @@ -495,11 +495,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()) } From 9729803e36d756b090d9093df69067db2942de5e Mon Sep 17 00:00:00 2001 From: Qi Wang Date: Wed, 2 Aug 2023 01:29:15 -0400 Subject: [PATCH 2/2] test zero value overlaysize&logsizemax Signed-off-by: Qi Wang --- ...ontainer_runtime_config_controller_test.go | 1 + .../container-runtime-config/helpers_test.go | 122 ++++++++++++++++++ 2 files changed, 123 insertions(+) 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 5c6b34aac8..590f3552c1 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 @@ -1397,6 +1397,7 @@ func TestMarshalResourceQuantityOptionsJSON(t *testing.T) { 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\"") } } diff --git a/pkg/controller/container-runtime-config/helpers_test.go b/pkg/controller/container-runtime-config/helpers_test.go index 6172320148..b52bddd21e 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" ) @@ -834,3 +838,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) + } + } +}