From 0db62c86d25dd149c1bc84637ad6b904fbc6efe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Miguel=20Olmo=20Mart=C3=ADnez?= Date: Tue, 6 Aug 2019 22:16:22 +0200 Subject: [PATCH] Allow to launch pods when memory request is set (but no memory limit) in cluster CRD file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this change the memory limit is only checked if it has been set explicitely in the cluster CRD file. This allow the pod to be created following the Burstable QoS strategy. Before this change an error was raised (error trying to get the memory limit) in the spec.CheckPodMemory function and the pod with only a 'memory request' set, was not able to start. Resolves: #3564 Signed-off-by: Juan Miguel Olmo Martínez jolmomar@redhat.com Signed-off-by: Juan Miguel Olmo Martínez --- pkg/operator/ceph/spec/spec.go | 24 ++++++++-------- pkg/operator/ceph/spec/spec_test.go | 43 +++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/pkg/operator/ceph/spec/spec.go b/pkg/operator/ceph/spec/spec.go index d4dd83b4f..d96c11350 100644 --- a/pkg/operator/ceph/spec/spec.go +++ b/pkg/operator/ceph/spec/spec.go @@ -229,19 +229,21 @@ func CheckPodMemory(resources v1.ResourceRequirements, cephPodMinimumMemory uint return nil } - // This means LIMIT and REQUEST are either identical or different but still we use LIMIT as a reference - if uint64(podMemoryLimit.Value()) < display.MbTob(cephPodMinimumMemory) { - return fmt.Errorf(errorMessage, display.BToMb(uint64(podMemoryLimit.Value())), cephPodMinimumMemory) - } + if !podMemoryLimit.IsZero() { + // This means LIMIT and REQUEST are either identical or different but still we use LIMIT as a reference + if uint64(podMemoryLimit.Value()) < display.MbTob(cephPodMinimumMemory) { + return fmt.Errorf(errorMessage, display.BToMb(uint64(podMemoryLimit.Value())), cephPodMinimumMemory) + } - // This means LIMIT < REQUEST - // Kubernetes will refuse to schedule that pod however it's still valuable to indicate that user's input was incorrect - if uint64(podMemoryLimit.Value()) < uint64(podMemoryRequest.Value()) { - extraErrorLine := `\n - User has specified a pod memory limit %dmb below the pod memory request %dmb in the cluster CR.\n - Rook will create pods that are expected to fail to serve as a more apparent error indicator to the user.` + // This means LIMIT < REQUEST + // Kubernetes will refuse to schedule that pod however it's still valuable to indicate that user's input was incorrect + if uint64(podMemoryLimit.Value()) < uint64(podMemoryRequest.Value()) { + extraErrorLine := `\n + User has specified a pod memory limit %dmb below the pod memory request %dmb in the cluster CR.\n + Rook will create pods that are expected to fail to serve as a more apparent error indicator to the user.` - return fmt.Errorf(extraErrorLine, display.BToMb(uint64(podMemoryLimit.Value())), display.BToMb(uint64(podMemoryRequest.Value()))) + return fmt.Errorf(extraErrorLine, display.BToMb(uint64(podMemoryLimit.Value())), display.BToMb(uint64(podMemoryRequest.Value()))) + } } return nil diff --git a/pkg/operator/ceph/spec/spec_test.go b/pkg/operator/ceph/spec/spec_test.go index 5a519593f..db16771e9 100644 --- a/pkg/operator/ceph/spec/spec_test.go +++ b/pkg/operator/ceph/spec/spec_test.go @@ -17,12 +17,15 @@ limitations under the License. package spec import ( + "math" "testing" "github.com/rook/rook/pkg/operator/ceph/config" "github.com/rook/rook/pkg/operator/k8sutil" "github.com/rook/rook/pkg/operator/test" "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" ) func TestPodVolumes(t *testing.T) { @@ -52,3 +55,43 @@ func TestGenerateLifeCycleCmd(t *testing.T) { cmd = generateLifeCycleCmd("foo") assert.Equal(t, append(config.ContainerPostStartCmd, "foo"), cmd) } + +func TestCheckPodMemory(t *testing.T) { + // This value is in MB + const PodMinimumMemory uint64 = 1024 + + // A value for the memory used in the tests + var memory_value = int64(PodMinimumMemory * 8 * uint64(math.Pow10(6))) + + // Case 1: No memory limits, no memory requested + test_resource := v1.ResourceRequirements{} + + if err := CheckPodMemory(test_resource, PodMinimumMemory); err != nil { + t.Errorf("Error case 1: %s", err.Error()) + } + + // Case 2: memory limit and memory requested + test_resource = v1.ResourceRequirements{ + Limits: v1.ResourceList{ + v1.ResourceMemory: *resource.NewQuantity(memory_value, resource.BinarySI), + }, + Requests: v1.ResourceList{ + v1.ResourceMemory: *resource.NewQuantity(memory_value, resource.BinarySI), + }, + } + + if err := CheckPodMemory(test_resource, PodMinimumMemory); err != nil { + t.Errorf("Error case 2: %s", err.Error()) + } + + // Only memory requested + test_resource = v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceMemory: *resource.NewQuantity(memory_value, resource.BinarySI), + }, + } + + if err := CheckPodMemory(test_resource, PodMinimumMemory); err != nil { + t.Errorf("Error case 3: %s", err.Error()) + } +}