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

Validate TaskRun compatibility with the Affinity Assistant #2885

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
29 changes: 29 additions & 0 deletions pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,12 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1
return nil, nil, controller.NewPermanentError(err)
}

if err := validateWorkspaceCompatibilityWithAffinityAssistant(tr); err != nil {
logger.Errorf("TaskRun %q workspaces are invalid: %v", tr.Name, err)
tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
return nil, nil, controller.NewPermanentError(err)
}

// Initialize the cloud events if at least a CloudEventResource is defined
// and they have not been initialized yet.
// FIXME(afrittoli) This resource specific logic will have to be replaced
Expand Down Expand Up @@ -702,3 +708,26 @@ func storeTaskSpec(ctx context.Context, tr *v1beta1.TaskRun, ts *v1beta1.TaskSpe
}
return nil
}

// validateWorkspaceCompatibilityWithAffinityAssistant validates the TaskRun's compatibility
// with the Affinity Assistant - if associated with an Affinity Assistant.
// No more than one PVC-backed workspace can be used for a TaskRun that is associated with an
// Affinity Assistant.
func validateWorkspaceCompatibilityWithAffinityAssistant(tr *v1beta1.TaskRun) error {
_, isAssociatedWithAnAffinityAssistant := tr.Annotations[workspace.AnnotationAffinityAssistantName]
if !isAssociatedWithAnAffinityAssistant {
return nil
}

pvcWorkspaces := 0
for _, w := range tr.Spec.Workspaces {
if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil {
pvcWorkspaces++
}
}

if pvcWorkspaces > 1 {
return fmt.Errorf("TaskRun mounts more than one PersistentVolumeClaim - that is forbidden when the Affinity Assistant is enabled")
Copy link

Choose a reason for hiding this comment

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

Should this be "mounts more than one writeable PVC"? I thought it was OK to receive multiple PVCs in read-only? Or will that also trip into potential deadlock scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good question @sbwsg

TLDR; as the message is written - it matches the current implementation of the Affinity Assistant.

The Affinity Assistant makes so that TaskRun pods are scheduled to the same Node. This actually helps to avoid two different problems:

  1. All pods sharing a PVC (e.g. with accessMode ReadWriteOnce) can now execute in parallel - this may be possible with other accessModes e.g. ReadWriteMany even without the Affinity Assistant.
  2. All pods sharing a PVC (e.g. with a zonal StorageClass) is scheduled to the same AZ, so it will not deadlock the pipeline - this may be possible with regional StorageClasses even without the Affinity Assistant - but on e.g. GCP regional storageClass volumes is only available on two of three AZs - so it may still be a problem. If using accessMode ReadWriteMany pods can execute in parallel within an AZ - but for regional clusters they additionally need to be regional StorageClass to avoid the problems that the Affinity Assistant solves.

The guidelines about "at most one writeable workspace" is for Tasks - e.g. they don't say if it is a PVC or emptyDir or regional StorageClass. With "writeable" I also meant PVCs - e.g. using a Secret or ConfigMap-workspace is still fine for Tasks - in addition to a PVC workspace.

I may add a new section to the workspace documentation to document more about this - it is a bit complicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand - using a ReadWriteOnce in readOnly mode and regional storageClass - should work - but is currently not supported/implemented by the Affinity Assistant. We may want to implement it - but it is a bit complicated too - and probably a rarely used use case.

}
return nil
}
68 changes: 68 additions & 0 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
"github.com/tektoncd/pipeline/pkg/system"
"github.com/tektoncd/pipeline/pkg/timeout"
"github.com/tektoncd/pipeline/pkg/workspace"
test "github.com/tektoncd/pipeline/test"
"github.com/tektoncd/pipeline/test/diff"
"github.com/tektoncd/pipeline/test/names"
Expand Down Expand Up @@ -2820,6 +2821,73 @@ func TestReconcileTaskResourceResolutionAndValidation(t *testing.T) {
}
}

// TestReconcileWithWorkspacesIncompatibleWithAffinityAssistant tests that a TaskRun used with an associated
// Affinity Assistant is validated and that the validation fails for a TaskRun that is incompatible with
// Affinity Assistant; e.g. using more than one PVC-backed workspace.
func TestReconcileWithWorkspacesIncompatibleWithAffinityAssistant(t *testing.T) {
taskWithTwoWorkspaces := tb.Task("test-task-two-workspaces", tb.TaskNamespace("foo"),
tb.TaskSpec(
tb.TaskWorkspace("ws1", "task workspace", "", true),
tb.TaskWorkspace("ws2", "another workspace", "", false),
))
taskRun := tb.TaskRun("taskrun-with-two-workspaces", tb.TaskRunNamespace("foo"), tb.TaskRunSpec(
tb.TaskRunTaskRef(taskWithTwoWorkspaces.Name, tb.TaskRefAPIVersion("a1")),
tb.TaskRunWorkspacePVC("ws1", "", "pvc1"),
tb.TaskRunWorkspaceVolumeClaimTemplate("ws2", "", &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "pvc2",
},
Spec: corev1.PersistentVolumeClaimSpec{},
}),
))

// associate the TaskRun with a dummy Affinity Assistant
taskRun.Annotations[workspace.AnnotationAffinityAssistantName] = "dummy-affinity-assistant"

d := test.Data{
Tasks: []*v1beta1.Task{taskWithTwoWorkspaces},
TaskRuns: []*v1beta1.TaskRun{taskRun},
ClusterTasks: nil,
PipelineResources: nil,
}
names.TestingSeed()
testAssets, cancel := getTaskRunController(t, d)
defer cancel()
clients := testAssets.Clients

t.Logf("Creating SA %s in %s", "default", "foo")
if _, err := clients.Kube.CoreV1().ServiceAccounts("foo").Create(&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "default",
Namespace: "foo",
},
}); err != nil {
t.Fatal(err)
}

_ = testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(taskRun))

_, err := clients.Pipeline.TektonV1beta1().Tasks(taskRun.Namespace).Get(taskWithTwoWorkspaces.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("krux: %v", err)
}

ttt, err := clients.Pipeline.TektonV1beta1().TaskRuns(taskRun.Namespace).Get(taskRun.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("expected TaskRun %s to exist but instead got error when getting it: %v", taskRun.Name, err)
}

if len(ttt.Status.Conditions) != 1 {
t.Errorf("unexpected number of Conditions, expected 1 Condition")
}

for _, cond := range ttt.Status.Conditions {
if cond.Reason != podconvert.ReasonFailedValidation {
t.Errorf("unexpected Reason on the Condition, expected: %s, got: %s", podconvert.ReasonFailedValidation, cond.Reason)
}
}
}

// TestReconcileWorkspaceWithVolumeClaimTemplate tests a reconcile of a TaskRun that has
// a Workspace with VolumeClaimTemplate and check that it is translated to a created PersistentVolumeClaim.
func TestReconcileWorkspaceWithVolumeClaimTemplate(t *testing.T) {
Expand Down