Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NodeSelectors to Build and BuildRun objects #1683

Merged
merged 3 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions deploy/crds/shipwright.io_buildruns.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6916,6 +6916,14 @@ spec:
- name
type: object
type: array
nodeSelector:
additionalProperties:
type: string
description: |-
NodeSelector is a selector which must be true for the pod to fit on a node.
Selector which must match a node's labels for the pod to be scheduled on that node.
More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
type: object
output:
description: Output refers to the location where the built
image would be pushed.
Expand Down Expand Up @@ -9120,6 +9128,14 @@ spec:
- name
type: object
type: array
nodeSelector:
additionalProperties:
type: string
description: |-
NodeSelector is a selector which must be true for the pod to fit on a node.
Selector which must match a node's labels for the pod to be scheduled on that node.
More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
type: object
output:
description: |-
Output refers to the location where the generated
Expand Down Expand Up @@ -11161,6 +11177,14 @@ spec:
- name
type: object
type: array
nodeSelector:
additionalProperties:
type: string
description: |-
NodeSelector is a selector which must be true for the pod to fit on a node.
Selector which must match a node's labels for the pod to be scheduled on that node.
More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
type: object
output:
description: Output refers to the location where the built image
would be pushed.
Expand Down
8 changes: 8 additions & 0 deletions deploy/crds/shipwright.io_builds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2488,6 +2488,14 @@ spec:
- name
type: object
type: array
nodeSelector:
additionalProperties:
type: string
description: |-
NodeSelector is a selector which must be true for the pod to fit on a node.
Selector which must match a node's labels for the pod to be scheduled on that node.
More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
type: object
output:
description: Output refers to the location where the built image would
be pushed.
Expand Down
34 changes: 21 additions & 13 deletions docs/build.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,25 @@ SPDX-License-Identifier: Apache-2.0

# Build

- [Overview](#overview)
- [Build Controller](#build-controller)
- [Build Validations](#build-validations)
- [Configuring a Build](#configuring-a-build)
- [Defining the Source](#defining-the-source)
- [Defining the Strategy](#defining-the-strategy)
- [Defining ParamValues](#defining-paramvalues)
- [Defining the Builder or Dockerfile](#defining-the-builder-or-dockerfile)
- [Defining the Output](#defining-the-output)
- [Defining Retention Parameters](#defining-retention-parameters)
- [Defining Volumes](#defining-volumes)
- [Defining Triggers](#defining-triggers)
- [BuildRun deletion](#buildrun-deletion)
- [Build](#build)
- [Overview](#overview)
- [Build Controller](#build-controller)
- [Build Validations](#build-validations)
- [Configuring a Build](#configuring-a-build)
- [Defining the Source](#defining-the-source)
- [Defining the Strategy](#defining-the-strategy)
- [Defining ParamValues](#defining-paramvalues)
- [Example](#example)
- [Defining the Builder or Dockerfile](#defining-the-builder-or-dockerfile)
- [Defining the Output](#defining-the-output)
- [Defining the vulnerabilityScan](#defining-the-vulnerabilityscan)
- [Defining Retention Parameters](#defining-retention-parameters)
- [Defining Volumes](#defining-volumes)
- [Defining Triggers](#defining-triggers)
- [GitHub](#github)
- [Image](#image)
- [Tekton Pipeline](#tekton-pipeline)
- [BuildRun Deletion](#buildrun-deletion)

## Overview

Expand All @@ -33,6 +39,7 @@ A `Build` resource allows the user to define:
- env
- retention
- volumes
- nodeSelector

A `Build` is available within a namespace.

Expand Down Expand Up @@ -118,6 +125,7 @@ The `Build` definition supports the following fields:
- `spec.retention.ttlAfterSucceeded` - Specifies the duration for which a successful buildrun can exist.
- `spec.retention.failedLimit` - Specifies the number of failed buildrun that can exist.
- `spec.retention.succeededLimit` - Specifies the number of successful buildrun can exist.
- `spec.nodeSelector` - Specifies a selector which must match a node's labels for the build pod to be scheduled on that node.

### Defining the Source

Expand Down
42 changes: 23 additions & 19 deletions docs/buildrun.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,28 @@ SPDX-License-Identifier: Apache-2.0

# BuildRun

- [Overview](#overview)
- [BuildRun Controller](#buildrun-controller)
- [Configuring a BuildRun](#configuring-a-buildrun)
- [Defining the Build Reference](#defining-the-build-reference)
- [Defining the Build Specification](#defining-the-build-specification)
- [Defining ParamValues](#defining-paramvalues)
- [Defining the ServiceAccount](#defining-the-serviceaccount)
- [Defining Retention Parameters](#defining-retention-parameters)
- [Defining Volumes](#defining-volumes)
- [Canceling a `BuildRun`](#canceling-a-buildrun)
- [Automatic `BuildRun` deletion](#automatic-buildrun-deletion)
- [Specifying Environment Variables](#specifying-environment-variables)
- [BuildRun Status](#buildrun-status)
- [Understanding the state of a BuildRun](#understanding-the-state-of-a-buildrun)
- [Understanding failed BuildRuns](#understanding-failed-buildruns)
- [Understanding failed git-source step](#understanding-failed-git-source-step)
- [Step Results in BuildRun Status](#step-results-in-buildrun-status)
- [Build Snapshot](#build-snapshot)
- [Relationship with Tekton Tasks](#relationship-with-tekton-tasks)
- [BuildRun](#buildrun)
- [Overview](#overview)
- [BuildRun Controller](#buildrun-controller)
- [Configuring a BuildRun](#configuring-a-buildrun)
- [Defining the Build Reference](#defining-the-build-reference)
- [Defining the Build Specification](#defining-the-build-specification)
- [Defining the Build Source](#defining-the-build-source)
- [Defining ParamValues](#defining-paramvalues)
- [Defining the ServiceAccount](#defining-the-serviceaccount)
- [Defining Retention Parameters](#defining-retention-parameters)
- [Defining Volumes](#defining-volumes)
- [Canceling a `BuildRun`](#canceling-a-buildrun)
- [Automatic `BuildRun` deletion](#automatic-buildrun-deletion)
- [Specifying Environment Variables](#specifying-environment-variables)
- [BuildRun Status](#buildrun-status)
- [Understanding the state of a BuildRun](#understanding-the-state-of-a-buildrun)
- [Understanding failed BuildRuns](#understanding-failed-buildruns)
- [Understanding failed BuildRuns due to VulnerabilitiesFound](#understanding-failed-buildruns-due-to-vulnerabilitiesfound)
- [Understanding failed git-source step](#understanding-failed-git-source-step)
- [Step Results in BuildRun Status](#step-results-in-buildrun-status)
- [Build Snapshot](#build-snapshot)
- [Relationship with Tekton Tasks](#relationship-with-tekton-tasks)

## Overview

Expand Down Expand Up @@ -72,6 +75,7 @@ The `BuildRun` definition supports the following fields:
- `spec.output.timestamp` - Overrides the output timestamp configuration of the referenced build to instruct the build to change the output image creation timestamp to the specified value. When omitted, the respective build strategy tool defines the output image timestamp.
- `spec.output.vulnerabilityScan` - Overrides the output vulnerabilityScan configuration of the referenced build to run the vulnerability scan for the generated image.
- `spec.env` - Specifies additional environment variables that should be passed to the build container. Overrides any environment variables that are specified in the `Build` resource. The available variables depend on the tool used by the chosen build strategy.
- `spec.nodeSelector` - Specifies a selector which must match a node's labels for the build pod to be scheduled on that node.

**Note**: The `spec.build.name` and `spec.build.spec` are mutually exclusive. Furthermore, the overrides for `timeout`, `paramValues`, `output`, and `env` can only be combined with `spec.build.name`, but **not** with `spec.build.spec`.

Expand Down
9 changes: 9 additions & 0 deletions pkg/apis/build/v1beta1/build_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ const (
OutputTimestampNotSupported BuildReason = "OutputTimestampNotSupported"
// OutputTimestampNotValid indicates that the output timestamp value is not valid
OutputTimestampNotValid BuildReason = "OutputTimestampNotValid"
// NodeSelectorNotValid indicates that the nodeSelector value is not valid
NodeSelectorNotValid BuildReason = "NodeSelectorNotValid"

// AllValidationsSucceeded indicates a Build was successfully validated
AllValidationsSucceeded = "all validations succeeded"
Expand Down Expand Up @@ -174,6 +176,13 @@ type BuildSpec struct {
//
// +optional
Volumes []BuildVolume `json:"volumes,omitempty"`

// NodeSelector is a selector which must be true for the pod to fit on a node.
// Selector which must match a node's labels for the pod to be scheduled on that node.
// More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
//
// +optional
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
}

// BuildVolume is a volume that will be mounted in build pod during build step
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/build/v1beta1/buildrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ type BuildRunSpec struct {
// to be overridden. Must only contain volumes that exist in the corresponding BuildStrategy
// +optional
Volumes []BuildVolume `json:"volumes,omitempty"`

// NodeSelector is a selector which must be true for the pod to fit on a node.
// Selector which must match a node's labels for the pod to be scheduled on that node.
// More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
//
// +optional
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
}

// BuildRunRequestedState defines the buildrun state the user can provide to override whatever is the current state.
Expand Down
14 changes: 14 additions & 0 deletions pkg/apis/build/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/reconciler/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ var validationTypes = [...]string{
validate.BuildName,
validate.Envs,
validate.Triggers,
validate.NodeSelector,
}

// ReconcileBuild reconciles a Build object
Expand Down
16 changes: 16 additions & 0 deletions pkg/reconciler/build/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package build_test
import (
"context"
"fmt"
"strings"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -613,5 +614,20 @@ var _ = Describe("Reconcile Build", func() {
Expect(err).ToNot(HaveOccurred())
})
})

Context("when nodeSelector is specified", func() {
It("should fail to validate when the nodeSelector is invalid", func() {
// set nodeSelector to be invalid
buildSample.Spec.NodeSelector = map[string]string{strings.Repeat("s", 64): "amd64"}
buildSample.Spec.Output.PushSecret = nil

statusCall := ctl.StubFunc(corev1.ConditionFalse, build.NodeSelectorNotValid, "name part must be no more than 63 characters")
statusWriter.UpdateCalls(statusCall)

_, err := reconciler.Reconcile(context.TODO(), request)
Expect(err).To(BeNil())
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})
})
})
1 change: 1 addition & 0 deletions pkg/reconciler/buildrun/buildrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
validate.NewSourceRef(build),
validate.NewBuildName(build),
validate.NewEnv(build),
validate.NewNodeSelector(build),
)

// an internal/technical error during validation happened
Expand Down
14 changes: 14 additions & 0 deletions pkg/reconciler/buildrun/buildrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1631,5 +1631,19 @@ var _ = Describe("Reconcile BuildRun", func() {
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})

Context("when nodeSelector is specified", func() {
It("fails when the nodeSelector is invalid", func() {
// set nodeSelector to be invalid
buildSample.Spec.NodeSelector = map[string]string{strings.Repeat("s", 64): "amd64"}

statusCall := ctl.StubFunc(corev1.ConditionFalse, build.NodeSelectorNotValid, "must be no more than 63 characters")
statusWriter.UpdateCalls(statusCall)

_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)
Expect(err).To(BeNil())
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})
})
})
9 changes: 9 additions & 0 deletions pkg/reconciler/buildrun/resources/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/shipwright-io/build/pkg/env"
"github.com/shipwright-io/build/pkg/reconciler/buildrun/resources/steps"
"github.com/shipwright-io/build/pkg/volumes"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod"
pipelineapi "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
)

Expand Down Expand Up @@ -234,6 +235,14 @@ func GenerateTaskRun(
},
}

// Merge Build and BuildRun NodeSelectors, giving preference to BuildRun NodeSelector
taskRunNodeSelector := mergeMaps(build.Spec.NodeSelector, buildRun.Spec.NodeSelector)
if len(taskRunNodeSelector) > 0 {
expectedTaskRun.Spec.PodTemplate = &pod.PodTemplate{
NodeSelector: taskRunNodeSelector,
}
}
Comment on lines +240 to +244
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit (not blocking): we should initialize the expectedTaskRun pod template to an empty instance earlier than here. This will help when future contributors add the other parts of SHIP-0039.


// assign the annotations from the build strategy, filter out those that should not be propagated
taskRunAnnotations := make(map[string]string)
for key, value := range strategy.GetAnnotations() {
Expand Down
22 changes: 22 additions & 0 deletions pkg/reconciler/buildrun/resources/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -632,5 +632,27 @@ var _ = Describe("GenerateTaskrun", func() {
Expect(paramOutputImageFound).To(BeTrue())
})
})

Context("when the build and buildrun both specify a nodeSelector", func() {
BeforeEach(func() {
build, err = ctl.LoadBuildYAML([]byte(test.MinimalBuildRunWithNodeSelector))
Expect(err).To(BeNil())

buildRun, err = ctl.LoadBuildRunFromBytes([]byte(test.MinimalBuildRunWithNodeSelector))
Expect(err).To(BeNil())

buildStrategy, err = ctl.LoadBuildStrategyFromBytes([]byte(test.ClusterBuildStrategyNoOp))
Expect(err).To(BeNil())
})

JustBeforeEach(func() {
got, err = resources.GenerateTaskRun(config.NewDefaultConfig(), build, buildRun, serviceAccountName, buildStrategy)
Expect(err).To(BeNil())
})

It("should give precedence to the nodeSelector specified in the buildRun", func() {
Expect(got.Spec.PodTemplate.NodeSelector).To(Equal(buildRun.Spec.NodeSelector))
})
})
})
})
42 changes: 42 additions & 0 deletions pkg/validate/nodeselector.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright The Shipwright Contributors
//
// SPDX-License-Identifier: Apache-2.0

package validate

import (
"context"
"strings"

"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/utils/ptr"

build "github.com/shipwright-io/build/pkg/apis/build/v1beta1"
)

// NodeSelectorRef contains all required fields
// to validate a node selector
type NodeSelectorRef struct {
Build *build.Build // build instance for analysis
}

func NewNodeSelector(build *build.Build) *NodeSelectorRef {
return &NodeSelectorRef{build}
}

// ValidatePath implements BuildPath interface and validates
// that NodeSelector keys/values are valid labels
func (b *NodeSelectorRef) ValidatePath(_ context.Context) error {
for key, value := range b.Build.Spec.NodeSelector {
if errs := validation.IsQualifiedName(key); len(errs) > 0 {
b.Build.Status.Reason = ptr.To(build.NodeSelectorNotValid)
b.Build.Status.Message = ptr.To(strings.Join(errs, ", "))
}
if errs := validation.IsValidLabelValue(value); len(errs) > 0 {
b.Build.Status.Reason = ptr.To(build.NodeSelectorNotValid)
b.Build.Status.Message = ptr.To(strings.Join(errs, ", "))
}
}

return nil
}
Loading