diff --git a/pipeline/builder/builder.go b/pipeline/builder/builder.go index 35f72b1..ad7165f 100644 --- a/pipeline/builder/builder.go +++ b/pipeline/builder/builder.go @@ -21,7 +21,7 @@ import ( ) const ( - errParseResourceList = "unable to parse resource limits for container: %s" + errOverrideResource = "could not override %s for container: %s" ) var ( @@ -333,23 +333,19 @@ func (b *Builder) buildDeployEmbeddedManifestStage(index int, s config.Stage) (* } for ii, specContainer := range d.Spec.Template.Spec.Containers { for _, overrideContainer := range maniStage.ContainerOverrides { - if specContainer.Name != overrideContainer.Name { + if specContainer.Name != overrideContainer.Name || overrideContainer.Resources == nil { continue } - if overrideContainer.Resources.Requests.Memory != "" || overrideContainer.Resources.Requests.CPU != "" { - requestsList, err := parseResourceList(overrideContainer.Resources.Requests.Memory, overrideContainer.Resources.Requests.CPU) - if err != nil { - return nil, errors.Wrapf(err, fmt.Sprintf(errParseResourceList, overrideContainer.Name)) - } - d.Spec.Template.Spec.Containers[ii].Resources.Requests = requestsList + requests, err := overrideResource(specContainer.Resources.Requests, overrideContainer.Resources.Requests) + if err != nil { + return nil, errors.Wrapf(err, errOverrideResource, "requests", overrideContainer.Name) } - if overrideContainer.Resources.Limits.Memory != "" || overrideContainer.Resources.Limits.CPU != "" { - limitsList, err := parseResourceList(overrideContainer.Resources.Limits.Memory, overrideContainer.Resources.Limits.CPU) - if err != nil { - return nil, errors.Wrapf(err, fmt.Sprintf(errParseResourceList, overrideContainer.Name)) - } - d.Spec.Template.Spec.Containers[ii].Resources.Limits = limitsList + d.Spec.Template.Spec.Containers[ii].Resources.Requests = requests + limits, err := overrideResource(specContainer.Resources.Limits, overrideContainer.Resources.Limits) + if err != nil { + return nil, errors.Wrapf(err, errOverrideResource, "limits", overrideContainer.Name) } + d.Spec.Template.Spec.Containers[ii].Resources.Limits = limits } } objs[i] = d.DeepCopyObject() @@ -451,20 +447,29 @@ func (b *Builder) buildDeleteEmbeddedManifestStage(index int, s config.Stage) (* return stage, nil } -// parseResourceList converts memory and cpu string to a resourceList -func parseResourceList(memory string, cpu string) (corev1.ResourceList, error) { - memoryQty, err := resource.ParseQuantity(memory) - if err != nil { - return nil, errors.Wrapf(err, "could not parse memory") +func overrideResource(resourceList corev1.ResourceList, override *config.Resource) (corev1.ResourceList, error) { + if override == nil { + return resourceList, nil } - cpuQty, err := resource.ParseQuantity(cpu) - if err != nil { - return nil, errors.Wrapf(err, "could not parse cpu") + result := make(corev1.ResourceList) + if resourceList != nil { + result = resourceList + } + if override.Memory != "" { + memoryQty, err := resource.ParseQuantity(override.Memory) + if err != nil { + return nil, errors.Wrapf(err, "could not parse memory") + } + result[corev1.ResourceMemory] = memoryQty + } + if override.CPU != "" { + cpuQty, err := resource.ParseQuantity(override.CPU) + if err != nil { + return nil, errors.Wrapf(err, "could not parse memory") + } + result[corev1.ResourceCPU] = cpuQty } - return map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceMemory: memoryQty, - corev1.ResourceCPU: cpuQty, - }, nil + return result, nil } func (b *Builder) defaultManifestStage(index int, s config.Stage) *types.ManifestStage { diff --git a/pipeline/builder/builder_test.go b/pipeline/builder/builder_test.go index 7d7f631..e755bf3 100644 --- a/pipeline/builder/builder_test.go +++ b/pipeline/builder/builder_test.go @@ -320,7 +320,7 @@ func TestBuilderPipelineStages(t *testing.T) { assert.Equal(t, int64(360000), spinnaker.Stages[0].(*types.ManifestStage).StageTimeoutMS) assert.Equal(t, true, spinnaker.Stages[0].(*types.ManifestStage).OverrideTimeout) }) - t.Run("Overrides container overrides", func(t *testing.T) { + t.Run("Overrides container overrides limits and requests", func(t *testing.T) { pipeline := &config.Pipeline{ Stages: []config.Stage{ { @@ -353,6 +353,180 @@ func TestBuilderPipelineStages(t *testing.T) { assert.Equal(t, "100", container.Resources.Requests.Memory().String()) assert.Equal(t, "200", container.Resources.Requests.Cpu().String()) }) + t.Run("Overrides container overrides only requests", func(t *testing.T) { + pipeline := &config.Pipeline{ + Stages: []config.Stage{ + { + Name: "Test DeployEmbeddedManifests Stage", + DeployEmbeddedManifests: &config.DeployEmbeddedManifests{ + Files: []config.ManifestFile{ + { + File: file, + }, + }, + ContainerOverrides: []*config.ContainerOverrides{ + { + Name: "test-container", + Resources: &config.Resources{ + Requests: &config.Resource{Memory: "100", CPU: "200"}, + }, + }, + }, + }, + }, + }, + } + builder := builder.New(pipeline) + spinnaker, err := builder.Pipeline() + require.NoError(t, err, "error generating pipeline json") + container := spinnaker.Stages[0].(*types.ManifestStage).Manifests[0].(*v1.Deployment).Spec.Template.Spec.Containers[0] + assert.Equal(t, "100", container.Resources.Requests.Memory().String()) + assert.Equal(t, "200", container.Resources.Requests.Cpu().String()) + }) + }) + t.Run("Overrides container overrides only limits", func(t *testing.T) { + pipeline := &config.Pipeline{ + Stages: []config.Stage{ + { + Name: "Test DeployEmbeddedManifests Stage", + DeployEmbeddedManifests: &config.DeployEmbeddedManifests{ + Files: []config.ManifestFile{ + { + File: file, + }, + }, + ContainerOverrides: []*config.ContainerOverrides{ + { + Name: "test-container", + Resources: &config.Resources{ + Limits: &config.Resource{Memory: "300Mi", CPU: "400m"}, + }, + }, + }, + }, + }, + }, + } + builder := builder.New(pipeline) + spinnaker, err := builder.Pipeline() + require.NoError(t, err, "error generating pipeline json") + container := spinnaker.Stages[0].(*types.ManifestStage).Manifests[0].(*v1.Deployment).Spec.Template.Spec.Containers[0] + assert.Equal(t, "300Mi", container.Resources.Limits.Memory().String()) + assert.Equal(t, "400m", container.Resources.Limits.Cpu().String()) + }) + t.Run("Overrides container overrides only cpu with resources set in deployment", func(t *testing.T) { + pipeline := &config.Pipeline{ + Stages: []config.Stage{ + { + Name: "Test DeployEmbeddedManifests Stage", + DeployEmbeddedManifests: &config.DeployEmbeddedManifests{ + Files: []config.ManifestFile{ + { + File: file, + }, + }, + ContainerOverrides: []*config.ContainerOverrides{ + { + Name: "test-container", + Resources: &config.Resources{ + Requests: &config.Resource{CPU: "400m"}, + }, + }, + }, + }, + }, + }, + } + builder := builder.New(pipeline) + spinnaker, err := builder.Pipeline() + require.NoError(t, err, "error generating pipeline json") + container := spinnaker.Stages[0].(*types.ManifestStage).Manifests[0].(*v1.Deployment).Spec.Template.Spec.Containers[0] + assert.Equal(t, "4", container.Resources.Requests.Memory().String()) + assert.Equal(t, "400m", container.Resources.Requests.Cpu().String()) + }) + t.Run("Overrides container overrides only cpu with resources not set in deployment", func(t *testing.T) { + pipeline := &config.Pipeline{ + Stages: []config.Stage{ + { + Name: "Test DeployEmbeddedManifests Stage", + DeployEmbeddedManifests: &config.DeployEmbeddedManifests{ + Files: []config.ManifestFile{ + { + File: file, + }, + }, + ContainerOverrides: []*config.ContainerOverrides{ + { + Name: "test-container-no-resources", + Resources: &config.Resources{ + Requests: &config.Resource{CPU: "400m"}, + }, + }, + }, + }, + }, + }, + } + builder := builder.New(pipeline) + spinnaker, err := builder.Pipeline() + require.NoError(t, err, "error generating pipeline json") + container := spinnaker.Stages[0].(*types.ManifestStage).Manifests[0].(*v1.Deployment).Spec.Template.Spec.Containers[1] + assert.Equal(t, "0", container.Resources.Requests.Memory().String()) + assert.Equal(t, "400m", container.Resources.Requests.Cpu().String()) + }) + t.Run("fails to override requests", func(t *testing.T) { + pipeline := &config.Pipeline{ + Stages: []config.Stage{ + { + Name: "Test DeployEmbeddedManifests Stage", + DeployEmbeddedManifests: &config.DeployEmbeddedManifests{ + Files: []config.ManifestFile{ + { + File: file, + }, + }, + ContainerOverrides: []*config.ContainerOverrides{ + { + Name: "test-container", + Resources: &config.Resources{ + Requests: &config.Resource{Memory: "bad-memory", CPU: "bad-cpu"}, + }, + }, + }, + }, + }, + }, + } + builder := builder.New(pipeline) + _, err := builder.Pipeline() + assert.Error(t, err, "Failed to buildDeployEmbeddedManifestStage with error: could not override requests for container: test-container: could not parse memory: quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'") + }) + t.Run("fails to override limits", func(t *testing.T) { + pipeline := &config.Pipeline{ + Stages: []config.Stage{ + { + Name: "Test DeployEmbeddedManifests Stage", + DeployEmbeddedManifests: &config.DeployEmbeddedManifests{ + Files: []config.ManifestFile{ + { + File: file, + }, + }, + ContainerOverrides: []*config.ContainerOverrides{ + { + Name: "test-container", + Resources: &config.Resources{ + Limits: &config.Resource{Memory: "bad-memory", CPU: "bad-cpu"}, + }, + }, + }, + }, + }, + }, + } + builder := builder.New(pipeline) + _, err := builder.Pipeline() + assert.Error(t, err, "Failed to buildDeployEmbeddedManifestStage with error: could not override limits for container: test-container: could not parse memory: quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'") }) t.Run("Deploy stage is parsed correctly", func(t *testing.T) { diff --git a/pipeline/builder/kubernetes_test.go b/pipeline/builder/kubernetes_test.go index c581802..5d8d890 100644 --- a/pipeline/builder/kubernetes_test.go +++ b/pipeline/builder/kubernetes_test.go @@ -60,7 +60,7 @@ func TestContainersFromManifests(t *testing.T) { require.NoError(t, err, "error on retrieving the deployment manifests") - assert.Len(t, group.Containers, 1) + assert.Len(t, group.Containers, 2) assert.Len(t, group.InitContainers, 1) assert.Len(t, group.Annotations, 2) assert.Equal(t, "fake-namespace", group.Namespace) diff --git a/pipeline/builder/testdata/deployment.full.yml b/pipeline/builder/testdata/deployment.full.yml index 83e139f..25948b2 100644 --- a/pipeline/builder/testdata/deployment.full.yml +++ b/pipeline/builder/testdata/deployment.full.yml @@ -45,11 +45,14 @@ spec: readOnly: true resources: limits: - cpu: "1" - memory: "2" + cpu: 1 + memory: 2 requests: cpu: "3" memory: "4" + - name: test-container-no-resources + command: + - echo volumes: - name: configmap-volume configMap: diff --git a/test-pipeline.yml b/test-pipeline.yml index 3ce512f..57c1cb0 100644 --- a/test-pipeline.yml +++ b/test-pipeline.yml @@ -39,6 +39,11 @@ stages: - file: test-configurator2.yml env: superOps - file: test-configurator.yml + containerOverrides: + - name: nginx + resources: + requests: + memory: 512Mi - account: ops-k8s name: "Scale Up" scaleManifest: