From 1c196bc938cf9035098b538683a056fbc1264445 Mon Sep 17 00:00:00 2001 From: Liran Rotenberg Date: Thu, 9 Nov 2023 20:31:13 +0200 Subject: [PATCH 1/3] Make the filesystem overhead configurable This patch will allow the user to configure the filesystem overhead. Currently it defaults to 10%. The overhead is mainly required for ext4 and the root partition. Signed-off-by: Liran Rotenberg --- operator/config/manager/manager.yaml | 2 ++ .../forkliftcontroller/defaults/main.yml | 1 + .../controller/deployment-controller.yml.j2 | 4 ++++ .../plan/adapter/openstack/builder.go | 2 +- pkg/controller/plan/adapter/ovirt/builder.go | 3 ++- pkg/controller/plan/util/BUILD.bazel | 1 + pkg/controller/plan/util/utils.go | 10 ++++++--- pkg/settings/logging.go | 2 +- pkg/settings/migration.go | 22 ++++++++++++------- pkg/settings/policy.go | 4 ++-- pkg/settings/profiler.go | 2 +- pkg/settings/settings.go | 22 +++++++++++++++---- 12 files changed, 54 insertions(+), 21 deletions(-) diff --git a/operator/config/manager/manager.yaml b/operator/config/manager/manager.yaml index 55f747dbb..5313cc2bd 100644 --- a/operator/config/manager/manager.yaml +++ b/operator/config/manager/manager.yaml @@ -64,6 +64,8 @@ spec: value: ${SNAPSHOT_STATUS_CHECK_RATE} - name: CDI_EXPORT_TOKEN_TTL value: ${CDI_EXPORT_TOKEN_TTL} + - name: FILESYSTEM_OVERHEAD + value: ${FILESYSTEM_OVERHEAD} - name: POPULATOR_CONTROLLER_IMAGE value: ${POPULATOR_CONTROLLER_IMAGE} - name: OVIRT_POPULATOR_IMAGE diff --git a/operator/roles/forkliftcontroller/defaults/main.yml b/operator/roles/forkliftcontroller/defaults/main.yml index 0268c2acc..3cb13c740 100644 --- a/operator/roles/forkliftcontroller/defaults/main.yml +++ b/operator/roles/forkliftcontroller/defaults/main.yml @@ -34,6 +34,7 @@ controller_snapshot_status_check_rate_seconds: 10 controller_vsphere_incremental_backup: true controller_ovirt_warm_migration: true controller_max_vm_inflight: 20 +controller_filesystem_overhead: 10 profiler_volume_path: "/var/cache/profiler" inventory_volume_path: "/var/cache/inventory" diff --git a/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 b/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 index 7d33203f5..a33993371 100644 --- a/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 +++ b/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 @@ -76,6 +76,10 @@ spec: - name: CDI_EXPORT_TOKEN_TTL value: "{{ CDI_EXPORT_TOKEN_TTL }}" {% endif %} +{% if controller_filesystem_overhead is number %} + - name: FILESYSTEM_OVERHEAD + value: "{{ controller_filesystem_overhead }}" +{% endif %} {% if controller_vsphere_incremental_backup|bool %} - name: FEATURE_VSPHERE_INCREMENTAL_BACKUP value: "true" diff --git a/pkg/controller/plan/adapter/openstack/builder.go b/pkg/controller/plan/adapter/openstack/builder.go index cee69b830..0c6956adc 100644 --- a/pkg/controller/plan/adapter/openstack/builder.go +++ b/pkg/controller/plan/adapter/openstack/builder.go @@ -1100,7 +1100,7 @@ func (r *Builder) persistentVolumeClaimWithSourceRef(image model.Image, storageC } if *volumeMode == core.PersistentVolumeFilesystem { - virtualSize = utils.CalculateSpaceWithOverhead(virtualSize, 0.1) + virtualSize = utils.CalculateSpaceWithOverhead(virtualSize) } // The image might be a VM Snapshot Image and has no volume associated to it diff --git a/pkg/controller/plan/adapter/ovirt/builder.go b/pkg/controller/plan/adapter/ovirt/builder.go index 5ab159d97..be73a286a 100644 --- a/pkg/controller/plan/adapter/ovirt/builder.go +++ b/pkg/controller/plan/adapter/ovirt/builder.go @@ -783,6 +783,7 @@ func (r *Builder) persistentVolumeClaimWithSourceRef(diskAttachment model.XDiskA annotations map[string]string) (pvc *core.PersistentVolumeClaim, err error) { // We add 10% overhead because of the fsOverhead in CDI, around 5% to ext4 and 5% for root partition. + // This value is configurable using `FILESYSTEM_OVERHEAD` diskSize := diskAttachment.Disk.ProvisionedSize var accessModes []core.PersistentVolumeAccessMode @@ -796,7 +797,7 @@ func (r *Builder) persistentVolumeClaimWithSourceRef(diskAttachment model.XDiskA // Accounting for fsOverhead is only required for `volumeMode: Filesystem`, as we may not have enough space // after creating a filesystem on an underlying block device if *volumeMode == core.PersistentVolumeFilesystem { - diskSize = utils.CalculateSpaceWithOverhead(diskSize, 0.1) + diskSize = utils.CalculateSpaceWithOverhead(diskSize) } annotations[AnnImportDiskId] = diskAttachment.ID diff --git a/pkg/controller/plan/util/BUILD.bazel b/pkg/controller/plan/util/BUILD.bazel index 98fe77c46..54588add3 100644 --- a/pkg/controller/plan/util/BUILD.bazel +++ b/pkg/controller/plan/util/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//pkg/apis/forklift/v1beta1", "//pkg/controller/provider/web/openstack", "//pkg/controller/provider/web/ovirt", + "//pkg/settings", "//vendor/k8s.io/api/core/v1:core", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:meta", ], diff --git a/pkg/controller/plan/util/utils.go b/pkg/controller/plan/util/utils.go index 3aa1af9d3..782fe210f 100644 --- a/pkg/controller/plan/util/utils.go +++ b/pkg/controller/plan/util/utils.go @@ -1,6 +1,10 @@ package util -import "math" +import ( + "math" + + "github.com/konveyor/forklift-controller/pkg/settings" +) // Disk alignment size used to align FS overhead, // its a multiple of all known hardware block sizes 512/4k/8k/32k/64k @@ -16,8 +20,8 @@ func roundUp(requestedSpace, multiple int64) int64 { return int64(partitions) * multiple } -func CalculateSpaceWithOverhead(requestedSpace int64, filesystemOverhead float64) int64 { +func CalculateSpaceWithOverhead(requestedSpace int64) int64 { alignedSize := roundUp(requestedSpace, DefaultAlignBlockSize) - spaceWithOverhead := int64(math.Ceil(float64(alignedSize) / (1 - filesystemOverhead))) + spaceWithOverhead := int64(math.Ceil(float64(alignedSize) / (1 - float64(settings.Settings.FileSystemOverhead)/100))) return spaceWithOverhead } diff --git a/pkg/settings/logging.go b/pkg/settings/logging.go index 139d0f0c8..3f402b6d3 100644 --- a/pkg/settings/logging.go +++ b/pkg/settings/logging.go @@ -21,6 +21,6 @@ type Logging struct { // Load settings. func (r *Logging) Load() error { r.Development = getEnvBool(LogDevelopment, false) - r.Level, _ = getEnvLimit(LogLevel, 0) + r.Level, _ = getPositiveEnvLimit(LogLevel, 0) return nil } diff --git a/pkg/settings/migration.go b/pkg/settings/migration.go index ba233926c..6abc47e7b 100644 --- a/pkg/settings/migration.go +++ b/pkg/settings/migration.go @@ -18,6 +18,7 @@ const ( SnapshotRemovalTimeout = "SNAPSHOT_REMOVAL_TIMEOUT" SnapshotStatusCheckRate = "SNAPSHOT_STATUS_CHECK_RATE" CDIExportTokenTTL = "CDI_EXPORT_TOKEN_TTL" + FileSystemOverhead = "FILESYSTEM_OVERHEAD" ) // Default virt-v2v image. @@ -46,31 +47,33 @@ type Migration struct { VirtV2vDontRequestKVM bool // OCP Export token TTL minutes CDIExportTokenTTL int + // FileSystem overhead in percantage + FileSystemOverhead int } // Load settings. func (r *Migration) Load() (err error) { - r.MaxInFlight, err = getEnvLimit(MaxVmInFlight, 20) + r.MaxInFlight, err = getPositiveEnvLimit(MaxVmInFlight, 20) if err != nil { err = liberr.Wrap(err) } - r.HookRetry, err = getEnvLimit(HookRetry, 3) + r.HookRetry, err = getPositiveEnvLimit(HookRetry, 3) if err != nil { err = liberr.Wrap(err) } - r.ImporterRetry, err = getEnvLimit(ImporterRetry, 3) + r.ImporterRetry, err = getPositiveEnvLimit(ImporterRetry, 3) if err != nil { err = liberr.Wrap(err) } - r.PrecopyInterval, err = getEnvLimit(PrecopyInterval, 60) + r.PrecopyInterval, err = getPositiveEnvLimit(PrecopyInterval, 60) if err != nil { err = liberr.Wrap(err) } - r.SnapshotRemovalTimeout, err = getEnvLimit(SnapshotRemovalTimeout, 120) + r.SnapshotRemovalTimeout, err = getPositiveEnvLimit(SnapshotRemovalTimeout, 120) if err != nil { err = liberr.Wrap(err) } - r.SnapshotStatusCheckRate, err = getEnvLimit(SnapshotStatusCheckRate, 10) + r.SnapshotStatusCheckRate, err = getPositiveEnvLimit(SnapshotStatusCheckRate, 10) if err != nil { err = liberr.Wrap(err) } @@ -87,8 +90,11 @@ func (r *Migration) Load() (err error) { r.VirtV2vImageWarm = DefaultVirtV2vImage } r.VirtV2vDontRequestKVM = getEnvBool(VirtV2vDontRequestKVM, false) - - r.CDIExportTokenTTL, err = getEnvLimit(CDIExportTokenTTL, 0) + r.CDIExportTokenTTL, err = getPositiveEnvLimit(CDIExportTokenTTL, 0) + if err != nil { + err = liberr.Wrap(err) + } + r.FileSystemOverhead, err = getNonNegativeEnvLimit(FileSystemOverhead, 10) if err != nil { err = liberr.Wrap(err) } diff --git a/pkg/settings/policy.go b/pkg/settings/policy.go index 1a40954f5..3b102485e 100644 --- a/pkg/settings/policy.go +++ b/pkg/settings/policy.go @@ -41,11 +41,11 @@ func (r *PolicyAgent) Load() (err error) { } else { r.TLS.CA = ServiceCAFile } - r.Limit.Worker, err = getEnvLimit(PolicyAgentWorkerLimit, 10) + r.Limit.Worker, err = getPositiveEnvLimit(PolicyAgentWorkerLimit, 10) if err != nil { return err } - r.SearchInterval, err = getEnvLimit(PolicyAgentSearchInterval, 600) + r.SearchInterval, err = getPositiveEnvLimit(PolicyAgentSearchInterval, 600) if err != nil { return err } diff --git a/pkg/settings/profiler.go b/pkg/settings/profiler.go index 4818c9e14..33558245b 100644 --- a/pkg/settings/profiler.go +++ b/pkg/settings/profiler.go @@ -27,7 +27,7 @@ type Profiler struct { // Load settings. func (r *Profiler) Load() error { - minutes, _ := getEnvLimit(ProfileDuration, 0) + minutes, _ := getPositiveEnvLimit(ProfileDuration, 0) r.Duration = time.Duration(minutes) * time.Minute if s, found := os.LookupEnv(ProfilePath); found { r.Path = s diff --git a/pkg/settings/settings.go b/pkg/settings/settings.go index 682067524..83b8baac7 100644 --- a/pkg/settings/settings.go +++ b/pkg/settings/settings.go @@ -1,9 +1,11 @@ package settings import ( - liberr "github.com/konveyor/forklift-controller/pkg/lib/error" + "fmt" "os" "strconv" + + liberr "github.com/konveyor/forklift-controller/pkg/lib/error" ) // Global @@ -69,15 +71,27 @@ func (r *ControllerSettings) Load() error { // Get positive integer limit from the environment // using the specified variable name and default. -func getEnvLimit(name string, def int) (int, error) { +func getPositiveEnvLimit(name string, def int) (int, error) { + return getEnvLimit(name, def, 1) +} + +// Get non-negative integer limit from the environment +// using the specified variable name and default. +func getNonNegativeEnvLimit(name string, def int) (int, error) { + return getEnvLimit(name, def, 0) +} + +// Get an integer limit from the environment +// using the specified variable name and default. +func getEnvLimit(name string, def, minimum int) (int, error) { limit := 0 if s, found := os.LookupEnv(name); found { n, err := strconv.Atoi(s) if err != nil { return 0, liberr.New(name + " must be an integer") } - if n < 1 { - return 0, liberr.New(name + " must be >= 1") + if n < minimum { + return 0, liberr.New(fmt.Sprintf(name+" must be >= %d", minimum)) } limit = n } else { From 992c0fdc398f211c769e619eeb1552d4604218f2 Mon Sep 17 00:00:00 2001 From: Liran Rotenberg Date: Sun, 12 Nov 2023 12:21:33 +0200 Subject: [PATCH 2/3] Fix CDI export token config Signed-off-by: Liran Rotenberg --- .../templates/controller/deployment-controller.yml.j2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 b/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 index a33993371..a1bc486df 100644 --- a/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 +++ b/operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2 @@ -74,7 +74,7 @@ spec: {% endif %} {% if controller_cdi_export_token_ttl is number %} - name: CDI_EXPORT_TOKEN_TTL - value: "{{ CDI_EXPORT_TOKEN_TTL }}" + value: "{{ controller_cdi_export_token_ttl }}" {% endif %} {% if controller_filesystem_overhead is number %} - name: FILESYSTEM_OVERHEAD From a751cbf2ed440fec0147eeef1bb1679a64418d42 Mon Sep 17 00:00:00 2001 From: Liran Rotenberg Date: Sun, 12 Nov 2023 12:28:22 +0200 Subject: [PATCH 3/3] Return when there is an error in the config When having an error due wrong configuration of the controller, return the error. Signed-off-by: Liran Rotenberg --- pkg/settings/migration.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/settings/migration.go b/pkg/settings/migration.go index 6abc47e7b..ff9cfdc5c 100644 --- a/pkg/settings/migration.go +++ b/pkg/settings/migration.go @@ -55,27 +55,27 @@ type Migration struct { func (r *Migration) Load() (err error) { r.MaxInFlight, err = getPositiveEnvLimit(MaxVmInFlight, 20) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } r.HookRetry, err = getPositiveEnvLimit(HookRetry, 3) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } r.ImporterRetry, err = getPositiveEnvLimit(ImporterRetry, 3) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } r.PrecopyInterval, err = getPositiveEnvLimit(PrecopyInterval, 60) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } r.SnapshotRemovalTimeout, err = getPositiveEnvLimit(SnapshotRemovalTimeout, 120) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } r.SnapshotStatusCheckRate, err = getPositiveEnvLimit(SnapshotStatusCheckRate, 10) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } if virtV2vImage, ok := os.LookupEnv(VirtV2vImage); ok { if cold, warm, found := strings.Cut(virtV2vImage, "|"); found { @@ -92,11 +92,11 @@ func (r *Migration) Load() (err error) { r.VirtV2vDontRequestKVM = getEnvBool(VirtV2vDontRequestKVM, false) r.CDIExportTokenTTL, err = getPositiveEnvLimit(CDIExportTokenTTL, 0) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } r.FileSystemOverhead, err = getNonNegativeEnvLimit(FileSystemOverhead, 10) if err != nil { - err = liberr.Wrap(err) + return liberr.Wrap(err) } return