Skip to content
This repository was archived by the owner on Sep 5, 2019. It is now read-only.

Add Pod affinity to builds#344

Merged
knative-prow-robot merged 3 commits intoknative:masterfrom
shashwathi:pod-affinity
Sep 11, 2018
Merged

Add Pod affinity to builds#344
knative-prow-robot merged 3 commits intoknative:masterfrom
shashwathi:pod-affinity

Conversation

@shashwathi
Copy link
Copy Markdown
Contributor

Fixes #237

Proposed Changes

  • Operator can specify pod affinity for builds.
  • e2e test timesout because of missing pod constraints.

@shashwathi
Copy link
Copy Markdown
Contributor Author

/assign @imjasonh
/assign @pivotal-nader-ziada

Spec: v1alpha1.BuildSpec{
Affinity: &corev1.Affinity{
NodeAffinity: &corev1.NodeAffinity{
// PreferredDuringSchedulingIgnoredDuringExecution: []
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this line

corev1.NodeSelectorTerm{
MatchExpressions: []corev1.NodeSelectorRequirement{
corev1.NodeSelectorRequirement{
Key: "kubernetes.io/e2e-az-name",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you describe in words why this affinity makes the build unscheduleable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@shashwathi
Copy link
Copy Markdown
Contributor Author

@imjasonh : I have addressed your comments.

@nader-ziada
Copy link
Copy Markdown
Member

is it also possible to add a test where the affinity rule matches any of the nodes, just to make sure that doesn't break anything in the future.

@shashwathi
Copy link
Copy Markdown
Contributor Author

I do not think its possible with existing test-infra scripts. We have to add a node to existing test cluster along with a label using some add-on scripts.

If the goal is to test whether the test is failing for right reasons, alternatively we could consider using kubeclient to fetch pod status and verify the reason for "Unschedulable" . I think both #344 and #335 need this additional testing IMO. I am okay to follow up with a PR to add pod assertions once both are merged.
WDYT @pivotal-nader-ziada ?

@nader-ziada
Copy link
Copy Markdown
Member

yeah, maybe a future PR to test-infra can provide this functionality

@shashwathi
Copy link
Copy Markdown
Contributor Author

/test pull-knative-build-integration-tests

1 similar comment
@shashwathi
Copy link
Copy Markdown
Contributor Author

/test pull-knative-build-integration-tests

Copy link
Copy Markdown
Contributor

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot
Copy link
Copy Markdown

[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:

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

@knative-prow-robot knative-prow-robot merged commit 0cf7a92 into knative:master Sep 11, 2018
@shashwathi shashwathi deleted the pod-affinity branch October 9, 2018 18:52
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
* Add pod affinity to container spec

* Add e2e test

* Update codegen
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
* Add pod affinity to container spec

* Add e2e test

* Update codegen
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants