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

WIP Add Custom Scheduler Name to Build and BuildRun objects #1770

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
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
12 changes: 12 additions & 0 deletions deploy/crds/shipwright.io_buildruns.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7439,6 +7439,10 @@ spec:
format: duration
type: string
type: object
schedulerName:
description: SchedulerName specifies the scheduler to be used
to dispatch the Pod
type: string
source:
description: |-
Source refers to the location where the source code is,
Expand Down Expand Up @@ -9753,6 +9757,10 @@ spec:
format: duration
type: string
type: object
schedulerName:
description: SchedulerName specifies the scheduler to be used to dispatch
the Pod
type: string
serviceAccount:
description: |-
ServiceAccount refers to the kubernetes serviceaccount
Expand Down Expand Up @@ -11941,6 +11949,10 @@ spec:
format: duration
type: string
type: object
schedulerName:
description: SchedulerName specifies the scheduler to be used
to dispatch the Pod
type: string
source:
description: |-
Source refers to the location where the source code is,
Expand Down
4 changes: 4 additions & 0 deletions deploy/crds/shipwright.io_builds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2818,6 +2818,10 @@ spec:
format: duration
type: string
type: object
schedulerName:
description: SchedulerName specifies the scheduler to be used to dispatch
the Pod
type: string
source:
description: |-
Source refers to the location where the source code is,
Expand Down
3 changes: 3 additions & 0 deletions docs/build.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ A `Build` resource allows the user to define:
- volumes
- nodeSelector
- tolerations
- schedulerName

A `Build` is available within a namespace.

Expand Down Expand Up @@ -97,6 +98,7 @@ To prevent users from triggering `BuildRun`s (_execution of a Build_) that will
| OutputTimestampNotValid | The output timestamp value is not valid. |
| NodeSelectorNotValid | The specified nodeSelector is not valid. |
| TolerationNotValid | The specified tolerations are not valid. |
| SchedulerNameNotValid | The specified schedulerName is not valid. |

## Configuring a Build

Expand Down Expand Up @@ -130,6 +132,7 @@ The `Build` definition supports the following fields:
- `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. If nodeSelectors are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence.
- `spec.tolerations` - Specifies the tolerations for the build pod. Only `key`, `value`, and `operator` are supported. Only `NoSchedule` taint `effect` is supported. If tolerations are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence.
- `spec.schedulerName` - Specifies the scheduler name for the build pod. If schedulerName is specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence.

### Defining the Source

Expand Down
1 change: 1 addition & 0 deletions docs/buildrun.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ The `BuildRun` definition supports the following fields:
- `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. If nodeSelectors are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence.
- `spec.tolerations` - Specifies the tolerations for the build pod. Only `key`, `value`, and `operator` are supported. Only `NoSchedule` taint `effect` is supported. If tolerations are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence.
- `spec.schedulerName` - Specifies the scheduler name for the build pod. If schedulerName is specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence.

**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
7 changes: 6 additions & 1 deletion pkg/apis/build/v1beta1/build_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ const (
NodeSelectorNotValid BuildReason = "NodeSelectorNotValid"
// TolerationNotValid indicates that the Toleration value is not valid
TolerationNotValid BuildReason = "TolerationNotValid"

// SchedulerNameNotValid indicates that the Scheduler name is not valid
SchedulerNameNotValid BuildReason = "SchedulerNameNotValid"
// AllValidationsSucceeded indicates a Build was successfully validated
AllValidationsSucceeded = "all validations succeeded"
)
Expand Down Expand Up @@ -191,6 +192,10 @@ type BuildSpec struct {
// +patchMergeKey=Key
// +patchStrategy=merge
Tolerations []corev1.Toleration `json:"tolerations,omitempty" patchStrategy:"merge" patchMergeKey:"Key"`

// SchedulerName specifies the scheduler to be used to dispatch the Pod
// +optional
SchedulerName string `json:"schedulerName,omitempty"`
}

// BuildVolume is a volume that will be mounted in build pod during build step
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/build/v1beta1/buildrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ type BuildRunSpec struct {
// +patchMergeKey=Key
// +patchStrategy=merge
Tolerations []corev1.Toleration `json:"tolerations,omitempty" patchStrategy:"merge" patchMergeKey:"Key"`

// SchedulerName specifies the scheduler to be used to dispatch the Pod
// +optional
SchedulerName string `json:"schedulerName,omitempty"`
}

// BuildRunRequestedState defines the buildrun state the user can provide to override whatever is the current state.
Expand Down
1 change: 1 addition & 0 deletions pkg/reconciler/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ var validationTypes = [...]string{
validate.Triggers,
validate.NodeSelector,
validate.Tolerations,
validate.SchedulerName,
}

// ReconcileBuild reconciles a Build object
Expand Down
15 changes: 15 additions & 0 deletions pkg/reconciler/build/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -645,5 +645,20 @@ var _ = Describe("Reconcile Build", func() {
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})

Context("when SchedulerName is specified", func() {
It("should fail to validate when the SchedulerName is invalid", func() {
// set SchedulerName to be invalid
buildSample.Spec.SchedulerName = strings.Repeat("s", 64)
buildSample.Spec.Output.PushSecret = nil

statusCall := ctl.StubFunc(corev1.ConditionFalse, build.SchedulerNameNotValid, "Scheduler name not valid: name part "+validation.MaxLenError(63))
statusWriter.UpdateCalls(statusCall)

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

// an internal/technical error during validation happened
Expand Down Expand Up @@ -310,6 +311,15 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
return reconcile.Result{}, nil
}

// Validate the schedulerName
valid, reason, message = validate.BuildRunSchedulerName(buildRun.Spec.SchedulerName)
if !valid {
if err := resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, message, reason); err != nil {
return reconcile.Result{}, err
}
return reconcile.Result{}, nil
}

// Create the TaskRun, this needs to be the last step in this block to be idempotent
generatedTaskRun, err := r.createTaskRun(ctx, svcAccount, strategy, build, buildRun)
if err != nil {
Expand Down
59 changes: 59 additions & 0 deletions pkg/reconciler/buildrun/buildrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1660,5 +1660,64 @@ var _ = Describe("Reconcile BuildRun", func() {
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})

Context("when SchedulerName is specified", func() {
It("should fail to validate when the SchedulerName is invalid", func() {
// set SchedulerName to be invalid
buildRunSample.Spec.SchedulerName = strings.Repeat("s", 64)

statusCall := ctl.StubFunc(corev1.ConditionFalse, build.SchedulerNameNotValid, validation.MaxLenError(64))
statusWriter.UpdateCalls(statusCall)

_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)
Expect(err).To(BeNil())
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})

Context("when a buildrun has a buildSpec defined and overrides nodeSelector", func() {
BeforeEach(func() {
buildRunSample = ctl.BuildRunWithNodeSelectorOverride(buildRunName, buildName, map[string]string{"Key": "testkey", "Value": "testvalue"})
})

It("should fail to register", func() {
statusCall := ctl.StubFunc(corev1.ConditionFalse, build.BuildReason(resources.BuildRunBuildFieldOverrideForbidden), "cannot use 'nodeSelector' override and 'buildSpec' simultaneously")
statusWriter.UpdateCalls(statusCall)

_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)
Expect(err).To(BeNil())
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})

Context("when a buildrun has a buildSpec defined and overrides Tolerations", func() {
BeforeEach(func() {
buildRunSample = ctl.BuildRunWithTolerationsOverride(buildRunName, buildName, []corev1.Toleration{{Key: "testkey", Value: "testvalue", Operator: corev1.TolerationOpEqual, Effect: corev1.TaintEffectNoSchedule}})
})

It("should fail to register", func() {
statusCall := ctl.StubFunc(corev1.ConditionFalse, build.BuildReason(resources.BuildRunBuildFieldOverrideForbidden), "cannot use 'tolerations' override and 'buildSpec' simultaneously")
statusWriter.UpdateCalls(statusCall)

_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)
Expect(err).To(BeNil())
Expect(statusWriter.UpdateCallCount()).To(Equal(1))
})
})

Context("when a buildrun has a buildSpec defined and overrides schedulerName", func() {
BeforeEach(func() {
buildRunSample = ctl.BuildRunWithSchedulerNameOverride(buildRunName, buildName, "testSchedulerName")
})

It("should fail to register", func() {
statusCall := ctl.StubFunc(corev1.ConditionFalse, build.BuildReason(resources.BuildRunBuildFieldOverrideForbidden), "cannot use 'schedulerName' override and 'buildSpec' simultaneously")
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 @@ -255,6 +255,15 @@ func GenerateTaskRun(
taskRunPodTemplate.Tolerations = taskRunTolerations
}

// Set custom scheduler name if specified, giving preference to BuildRun values
if buildRun.Spec.SchedulerName != "" {
taskRunPodTemplate.SchedulerName = buildRun.Spec.SchedulerName
} else {
if build.Spec.SchedulerName != "" {
taskRunPodTemplate.SchedulerName = build.Spec.SchedulerName
}
}

if !(taskRunPodTemplate.Equals(&pod.PodTemplate{})) {
expectedTaskRun.Spec.PodTemplate = taskRunPodTemplate
}
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 @@ -678,5 +678,27 @@ var _ = Describe("GenerateTaskrun", func() {
Expect(got.Spec.PodTemplate.Tolerations[0].Value).To(Equal(buildRun.Spec.Tolerations[0].Value))
})
})

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

buildRun, err = ctl.LoadBuildRunFromBytes([]byte(test.MinimalBuildRunWithSchedulerName))
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 SchedulerName value specified in the buildRun", func() {
Expect(got.Spec.PodTemplate.SchedulerName).To(Equal(buildRun.Spec.SchedulerName))
})
})
})
})
14 changes: 4 additions & 10 deletions pkg/validate/nodeselector.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,11 @@ func NewNodeSelector(build *build.Build) *NodeSelectorRef {
// 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(fmt.Sprintf("Node selector key not valid: %v", 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(fmt.Sprintf("Node selector value not valid: %v", strings.Join(errs, ", ")))
}
ok, reason, msg := BuildRunNodeSelector(b.Build.Spec.NodeSelector)
if !ok {
b.Build.Status.Reason = ptr.To(build.BuildReason(reason))
b.Build.Status.Message = ptr.To(msg)
}

return nil
}

Expand Down
69 changes: 69 additions & 0 deletions pkg/validate/nodeselector_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright The Shipwright Contributors
//
// SPDX-License-Identifier: Apache-2.0

package validate_test

import (
"context"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

corev1 "k8s.io/apimachinery/pkg/apis/meta/v1"

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

var _ = Describe("ValidateNodeSelector", func() {
var ctx context.Context

BeforeEach(func() {
ctx = context.TODO()
})

var validate = func(build *Build) {
GinkgoHelper()

var validator = &validate.NodeSelectorRef{Build: build}
Expect(validator.ValidatePath(ctx)).To(Succeed())
}

var sampleBuild = func(key string, value string) *Build {
return &Build{
ObjectMeta: corev1.ObjectMeta{
Namespace: "foo",
Name: "bar",
},
Spec: BuildSpec{
NodeSelector: map[string]string{"Key": key, "Value": value},
},
}
}

Context("when node selector is specified", func() {
It("should pass an empty key and value", func() {
build := sampleBuild("", "")
validate(build)
Expect(build.Status.Reason).To(BeNil())
Expect(build.Status.Message).To(BeNil())
})

It("should pass an empty key and valid value", func() {
build := sampleBuild("", "validvalue")
validate(build)
Expect(build.Status.Reason).To(BeNil())
Expect(build.Status.Message).To(BeNil())
})

It("should fail an empty key and invalid value", func() {
build := sampleBuild("", "invalidvalue!")
validate(build)
Expect(*build.Status.Reason).To(Equal(NodeSelectorNotValid))
Expect(*build.Status.Message).To(ContainSubstring("Node selector key not valid"))
})

// TODO: add the rest of the combinations
})
})
Loading
Loading