Allow build to specify node selectors

[shashwathi]

Addresses #116

Proposed Changes

• Allow Builds to specify node selectors

[shashwathi]

/assign @imjasonh

[imjasonh]

Does this also allow builds to specify node affinity/anti-affinity? That would probably be useful to support too, but I'm a bit worried about adding both of these as top-level fields to the Build resource.

It could also be useful to add an integration test which demonstrates this somehow, though adding a selector that requires disktype: ssd would require the test cluster to start at least one node that has that feature in order to schedule the build.

I'm not sure what happens when the cluster can't schedule a pod because of its selectors, does it eventually fail the pod and therefore the build? Maybe the integration test could have a node selector for some unsatisfiable requirement, and expect: Failure?

[shashwathi]

Does this also allow builds to specify node affinity/anti-affinity?

I submitted another PR for node affinity/anti-affinity.

I'm not sure what happens when the cluster can't schedule a pod because of its selectors, does it eventually fail the pod and therefore the build?

Yes it fails the build since pod does not get created when nodes with selectors are not found.

Maybe the integration test could have a node selector for some unsatisfiable requirement, and expect: Failure?

Sure. I can add that.
I am concerned about happy path integration case as it might require test setup to create nodes with disktype and might have infrastructure limitations.

That would probably be useful to support too, but I'm a bit worried about adding both of these as top-level fields to the Build resource.

I am leaning towards using top level field because the usage of both nodeselector and node-affinity is obvious pod spec and does not need nesting to explain the configuration meaning. I do not have much experience with this aspect so let me know.

@imjasonh

[shashwathi]

@dprotaso @mattmoor thoughts or concerns?

[shashwathi]

/test all

[shashwathi]

@imjasonh :

I tried to add build yaml with a selector which is unavailable

apiVersion: build.knative.dev/v1alpha1
kind: Build
metadata:
  name: failed-build
  labels:
    expect: failed
spec:
  steps:
  - name: write
    image: ubuntu:latest
    command: ["/bin/bash"]
    args: ["-c", "does not matter > /workspace/stuff"]
  nodeSelector:
    disktype: fakessd

This above example does not fall into "Build Failure" case because build k8s resources(pods) never get allocated and build is in PENDING status forever. This status on build triggers the asynchronous wait loop in build controller.

We need to surface the resource error at build level.

[imjasonh]

Worth noting that a build that specifies nodeSelector with the google builder implementation (calling out to GCB) will not have their node selector respected. That's probably fine to just ignore it, but it might be confusing if users start actually using the google builder.

Anyway,
/lgtm
/approve

[google-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ImJasonH]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment