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

Massive hack to prevent attempted mount of non-existent PVC 💀 #1069

Closed
wants to merge 1 commit into from
Closed
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
34 changes: 26 additions & 8 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -696,20 +696,38 @@ func TestReconcileWithTimeout(t *testing.T) {
}

func TestReconcileWithoutPVC(t *testing.T) {
rs := []*v1alpha1.PipelineResource{tb.PipelineResource("dance-party", "foo",
tb.PipelineResourceSpec(v1alpha1.PipelineResourceTypeGit),
)}
ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec(
tb.PipelineTask("hello-world-1", "hello-world"),
tb.PipelineTask("hello-world-2", "hello-world"),
tb.PipelineDeclaredResource("party", v1alpha1.PipelineResourceTypeGit),
// Though these use the same resource as input and output, they are not linked with `from` so no PVC is needed
tb.PipelineTask("hello-world-1", "time-output",
tb.PipelineTaskOutputResource("time", "party"),
),
tb.PipelineTask("hello-world-2", "time-input",
tb.PipelineTaskInputResource("time", "party"),
),
))}

prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run", "foo",
tb.PipelineRunSpec("test-pipeline")),
tb.PipelineRunSpec("test-pipeline",
tb.PipelineRunResourceBinding("party", tb.PipelineResourceBindingRef("dance-party")),
)),
}
ts := []*v1alpha1.Task{tb.Task("hello-world", "foo")}
ts := []*v1alpha1.Task{
tb.Task("time-input", "foo", tb.TaskSpec(
tb.TaskInputs(tb.InputsResource("time", v1alpha1.PipelineResourceTypeGit)),
)),
tb.Task("time-output", "foo", tb.TaskSpec(
tb.TaskOutputs(tb.OutputsResource("time", v1alpha1.PipelineResourceTypeGit)),
))}

d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
PipelineResources: rs,
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
}

// create fake recorder for testing
Expand All @@ -730,7 +748,7 @@ func TestReconcileWithoutPVC(t *testing.T) {
t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err)
}

// Check that the expected TaskRun was created
// Check that no PVC was created
for _, a := range clients.Kube.Actions() {
if ca, ok := a.(ktesting.CreateAction); ok {
obj := ca.GetObject()
Expand Down
24 changes: 21 additions & 3 deletions pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"go.uber.org/zap"
"golang.org/x/xerrors"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
)

Expand All @@ -43,8 +45,8 @@ var (
// This function also reads the inputs to check if resources are redeclared in inputs and has any custom
// target directory.
// Steps executed:
// 1. If taskrun has owner reference as pipelinerun then all outputs are copied to parents PVC
// and also runs any custom upload steps (upload to blob store)
// 1. If taskrun has owner reference as pipelinerun and the expected parent PVC exists, then all outputs
// are copied to parents PVC and also runs any custom upload steps (upload to blob store)
// 2. If taskrun does not have pipelinerun as owner reference then all outputs resources execute their custom
// upload steps (like upload to blob store )
//
Expand All @@ -68,8 +70,22 @@ func AddOutputResources(

pvcName := taskRun.GetPipelineRunPVCName()
as, err := artifacts.GetArtifactStorage(pvcName, kubeclient, logger)

if err != nil {
return nil, err
return nil, xerrors.Errorf("couldn't determine artifact storage: %w", err)
}

if as.GetType() == v1alpha1.ArtifactStoragePVCType {
// TODO(#1068) There is probably a race condition here - this is just a hack to work around the bug in 0.5.0 while we
// work on a real long term fix
if _, err := kubeclient.CoreV1().PersistentVolumeClaims(taskRun.Namespace).Get(pvcName, metav1.GetOptions{}); err != nil {
// If there is no "parent" PVC, we shouldn't try to attach it and upload to it
if errors.IsNotFound(err) {
as = &artifacts.ArtifactStorageNone{}
} else {
return nil, xerrors.Errorf("error checking for possible parent PVC volume %s: %w", pvcName, err)
}
}
}

// track resources that are present in input of task cuz these resources will be copied onto PVC
Expand All @@ -91,6 +107,7 @@ func AddOutputResources(
if !ok || resource == nil {
return nil, xerrors.Errorf("failed to get output pipeline Resource for task %q resource %v", taskName, boundResource)
}

var (
resourceContainers []corev1.Container
resourceVolumes []corev1.Volume
Expand Down Expand Up @@ -132,6 +149,7 @@ func AddOutputResources(
}

if allowedOutputResources[resource.GetType()] && taskRun.HasPipelineRunOwnerReference() {

var newSteps []corev1.Container
for _, dPath := range boundResource.Paths {
containers := as.GetCopyToStorageFromContainerSpec(resource.GetName(), sourcePath, dPath)
Expand Down
Loading