[release-4.14]OCPBUGS-27178: use *resource.Quantity to not automatically set 0

pkg/apis/machineconfiguration.openshift.io/v1/types.go

@@ -532,11 +532,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"`

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 && !ctrcfg.OverlaySize.IsZero() {
 				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.LogSizeMax.IsZero()) || ctrcfg.DefaultRuntime != mcfgv1.ContainerRuntimeDefaultRuntimeEmpty {
 				crioFileConfigs := createCRIODropinFiles(cfg)
 				configFileList = append(configFileList, crioFileConfigs...)
 			}

pkg/controller/container-runtime-config/container_runtime_config_controller.go

@@ -610,7 +610,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 {
 				klog.V(2).Infoln(cfg, err, "error merging user changes to storage.conf: %v", err)
@@ -622,7 +622,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...)
 		}

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/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/klog/v2"
@@ -494,10 +495,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()
@@ -531,10 +535,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()
@@ -1237,6 +1244,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
@@ -1257,7 +1266,7 @@ func TestContainerRuntimeConfigOptions(t *testing.T) {
 		{
 			name: "inalid value of max log size",
 			config: &mcfgv1.ContainerRuntimeConfiguration{
-				LogSizeMax: resource.MustParse("3k"),
+				LogSizeMax: &three,
 			},
 		},
 		{
@@ -1293,7 +1302,7 @@ func TestContainerRuntimeConfigOptions(t *testing.T) {
 		{
 			name: "valid max log size",
 			config: &mcfgv1.ContainerRuntimeConfiguration{
-				LogSizeMax: resource.MustParse("10k"),
+				LogSizeMax: &ten,
 			},
 		},
 		{
@@ -1329,6 +1338,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 {

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
 			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 +497,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())
 	}
 

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"
 )
 
@@ -910,3 +914,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)
+		}
+	}
+}