[Feature] Specify resource requests and limits on profiling container (…

…#34)

Users can specify container resource and limits with the following flags:
`cpu.requests`, `cpu.limits`, `mem.requests`, `mem.limits`. All these
flags are optional.

In order to allow graceful modification of the Job spec, a new type is
added: `JobDetails`, which can be used to pass configuration options
from the CLI to the Job spec. Right now only the `Resources` field of the
Job API is targeted, but this can be extended in the future.

In order to keep the list of function arguments small, a new container
type `FlameConfig` is introduced which holds `TargetDetails`, `JobDetails`
and CLI options. All functions are passed a pointer to this config.

Also extends error handling, adds comments and tests.

cli/cmd/data/flame.go

@@ -0,0 +1,9 @@
+package data
+
+import "k8s.io/cli-runtime/pkg/genericclioptions"
+
+type FlameConfig struct {
+	TargetConfig *TargetDetails
+	JobConfig    *JobDetails
+	ConfigFlags  *genericclioptions.ConfigFlags
+}

cli/cmd/data/job.go

@@ -0,0 +1,75 @@
+package data
+
+import (
+	"fmt"
+
+	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+// JobDetails holds configuration options for the profiling job that is launched
+// by kubectl-flame.
+type JobDetails struct {
+	// RequestConfig configures resource requests for the job that is started.
+	RequestConfig ResourceConfig
+
+	// LimitConfig configures resource limits for the job that is started.
+	LimitConfig ResourceConfig
+}
+
+// ResourceConfig holds resource configuration for either requests or limits.
+type ResourceConfig struct {
+	CPU    string
+	Memory string
+}
+
+// ToResourceRequirements parses JobDetails into an apiv1.ResourceRequirements
+// map which can be passed to a container spec.
+func (jd *JobDetails) ToResourceRequirements() (apiv1.ResourceRequirements, error) {
+	var out apiv1.ResourceRequirements
+
+	requests, err := jd.RequestConfig.ParseResources()
+	if err != nil {
+		return out, fmt.Errorf("unable to generate container requests: %w", err)
+	}
+
+	limits, err := jd.LimitConfig.ParseResources()
+	if err != nil {
+		return out, fmt.Errorf("unable to generate container limits: %w", err)
+	}
+
+	out.Requests = requests
+	out.Limits = limits
+
+	return out, nil
+}
+
+// ParseResources parses the ResourceConfig and returns an apiv1.ResourceList
+// which can be used in a apiv1.ResourceRequirements map.
+func (rc ResourceConfig) ParseResources() (apiv1.ResourceList, error) {
+	if rc.CPU == "" && rc.Memory == "" {
+		return nil, nil
+	}
+
+	list := make(apiv1.ResourceList)
+
+	if rc.CPU != "" {
+		cpu, err := resource.ParseQuantity(rc.CPU)
+		if err != nil {
+			return nil, fmt.Errorf("unable to parse CPU value %q: %w", rc.CPU, err)
+		}
+
+		list[apiv1.ResourceCPU] = cpu
+	}
+
+	if rc.Memory != "" {
+		mem, err := resource.ParseQuantity(rc.Memory)
+		if err != nil {
+			return nil, fmt.Errorf("unable to parse memory value %q: %w", rc.Memory, err)
+		}
+
+		list[apiv1.ResourceMemory] = mem
+	}
+
+	return list, nil
+}

cli/cmd/data/job_test.go

@@ -0,0 +1,237 @@
+package data
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+func TestResourceConfig_ParseResources(t *testing.T) {
+	tt := []struct {
+		name       string
+		resConf    ResourceConfig
+		want       apiv1.ResourceList
+		wantErrMsg string
+	}{
+		{
+			name: "empty config yields no resource list",
+		},
+		{
+			name: "invalid CPU yields error",
+			resConf: ResourceConfig{
+				CPU: "test",
+			},
+			wantErrMsg: "unable to parse CPU value",
+		},
+		{
+			name: "invalid memory yields error",
+			resConf: ResourceConfig{
+				Memory: "test",
+			},
+			wantErrMsg: "unable to parse memory value",
+		},
+		{
+			name: "invalid CPU shortcircuits",
+			resConf: ResourceConfig{
+				CPU:    "test",
+				Memory: "200Mi",
+			},
+			wantErrMsg: "unable to parse CPU value",
+		},
+		{
+			name: "only CPU is parsed correctly",
+			resConf: ResourceConfig{
+				CPU: "200m",
+			},
+			want: apiv1.ResourceList{
+				apiv1.ResourceCPU: resource.MustParse("200m"),
+			},
+		},
+		{
+			name: "only memory is parsed correctly",
+			resConf: ResourceConfig{
+				Memory: "200Mi",
+			},
+			want: apiv1.ResourceList{
+				apiv1.ResourceMemory: resource.MustParse("200Mi"),
+			},
+		},
+		{
+			name: "both CPU and memory are parsed correctly",
+			resConf: ResourceConfig{
+				CPU:    "200m",
+				Memory: "200Mi",
+			},
+			want: apiv1.ResourceList{
+				apiv1.ResourceCPU:    resource.MustParse("200m"),
+				apiv1.ResourceMemory: resource.MustParse("200Mi"),
+			},
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := tc.resConf.ParseResources()
+
+			if tc.wantErrMsg != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tc.wantErrMsg)
+			} else {
+				require.NoError(t, err)
+			}
+
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+func TestJobDetails_ToResourceRequirements(t *testing.T) {
+	tt := []struct {
+		name       string
+		jobDetails *JobDetails
+		want       apiv1.ResourceRequirements
+		wantErrMsg string
+	}{
+		{
+			name:       "empty resources yields empty requirements",
+			jobDetails: &JobDetails{},
+			want:       apiv1.ResourceRequirements{},
+		},
+		{
+			name: "invalid request CPU yields error",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU: "test",
+				},
+			},
+			wantErrMsg: "unable to generate container requests",
+		},
+		{
+			name: "invalid request mem yields error",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					Memory: "test",
+				},
+			},
+			wantErrMsg: "unable to generate container requests",
+		},
+		{
+			name: "valid requests yields requests only",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+			},
+			want: apiv1.ResourceRequirements{
+				Requests: apiv1.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("100m"),
+					apiv1.ResourceMemory: resource.MustParse("200Mi"),
+				},
+			},
+		},
+		{
+			name: "valid requests & invalid cpu limits yields error",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+			},
+			want: apiv1.ResourceRequirements{
+				Requests: apiv1.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("100m"),
+					apiv1.ResourceMemory: resource.MustParse("200Mi"),
+				},
+			},
+		},
+		{
+			name: "valid requests & invalid memory limits yields error",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+				LimitConfig: ResourceConfig{
+					CPU: "test",
+				},
+			},
+			wantErrMsg: "unable to generate container limits",
+		},
+		{
+			name: "valid requests & invalid memory limits yields error",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+				LimitConfig: ResourceConfig{
+					Memory: "test",
+				},
+			},
+			wantErrMsg: "unable to generate container limits",
+		},
+		{
+			name: "valid requests & memory yields both correctly",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+				LimitConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+			},
+			want: apiv1.ResourceRequirements{
+				Requests: apiv1.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("100m"),
+					apiv1.ResourceMemory: resource.MustParse("200Mi"),
+				},
+				Limits: apiv1.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("100m"),
+					apiv1.ResourceMemory: resource.MustParse("200Mi"),
+				},
+			},
+		},
+		{
+			name: "missing cpu limits yields requirements without cpu limits",
+			jobDetails: &JobDetails{
+				RequestConfig: ResourceConfig{
+					CPU:    "100m",
+					Memory: "200Mi",
+				},
+				LimitConfig: ResourceConfig{
+					Memory: "200Mi",
+				},
+			},
+			want: apiv1.ResourceRequirements{
+				Requests: apiv1.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("100m"),
+					apiv1.ResourceMemory: resource.MustParse("200Mi"),
+				},
+				Limits: apiv1.ResourceList{
+					apiv1.ResourceMemory: resource.MustParse("200Mi"),
+				},
+			},
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := tc.jobDetails.ToResourceRequirements()
+
+			if tc.wantErrMsg != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tc.wantErrMsg)
+			} else {
+				require.NoError(t, err)
+			}
+
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}

cli/cmd/data/target.go

@@ -3,16 +3,17 @@
 package data
 
 import (
-	"github.com/VerizonMedia/kubectl-flame/api"
 	"time"
+
+	"github.com/VerizonMedia/kubectl-flame/api"
 )
 
 type TargetDetails struct {
 	Namespace     string
 	PodName       string
 	ContainerName string
 	ContainerId   string
-	Event	      api.ProfilingEvent
+	Event         api.ProfilingEvent
 	Duration      time.Duration
 	Id            string
 	FileName      string