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

fix: Fixed artifact retrieval when templateRef in use. Fixes #9631, #9644. #9648

Merged
merged 2 commits into from
Sep 21, 2022
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
3 changes: 2 additions & 1 deletion server/artifacts/artifact_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
artifact "github.com/argoproj/argo-workflows/v3/workflow/artifacts"
"github.com/argoproj/argo-workflows/v3/workflow/artifacts/common"
"github.com/argoproj/argo-workflows/v3/workflow/hydrator"
"github.com/argoproj/argo-workflows/v3/workflow/util"
)

type ArtifactServer struct {
Expand Down Expand Up @@ -390,7 +391,7 @@ func (a *ArtifactServer) getArtifactAndDriver(ctx context.Context, nodeId, artif
// 3. Workflow spec defines artifactRepositoryRef which is a ConfigMap which defines the location
// 4. Template defines ArchiveLocation

templateName := wf.Status.Nodes[nodeId].TemplateName
templateName := util.GetTemplateFromNode(wf.Status.Nodes[nodeId])
template := wf.GetTemplateByName(templateName)
if template == nil {
return nil, nil, fmt.Errorf("no template found by the name of '%s' (which is the template associated with nodeId '%s'??", templateName, nodeId)
Expand Down
78 changes: 75 additions & 3 deletions server/artifacts/artifact_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (a *fakeArtifactDriver) Load(_ *wfv1.Artifact, path string) error {
}

var bucketsOfKeys = map[string][]string{
"my-bucket": []string{
"my-bucket": {
"my-wf/my-node-1/my-s3-input-artifact.tgz",
"my-wf/my-node-1/my-s3-artifact-directory",
"my-wf/my-node-1/my-s3-artifact-directory/a.txt",
Expand All @@ -64,12 +64,15 @@ var bucketsOfKeys = map[string][]string{
"my-wf/my-node-1/my-oss-artifact.zip",
"my-wf/my-node-1/my-s3-artifact.tgz",
},
"my-bucket-2": []string{
"my-bucket-2": {
"my-wf/my-node-2/my-s3-artifact-bucket-2",
},
"my-bucket-3": []string{
"my-bucket-3": {
"my-wf/my-node-2/my-s3-artifact-bucket-3",
},
"my-bucket-4": {
"my-wf/my-node-3/my-s3-artifact.tgz",
},
}

func (a *fakeArtifactDriver) OpenStream(artifact *wfv1.Artifact) (io.ReadCloser, error) {
Expand Down Expand Up @@ -278,9 +281,46 @@ func newServer() *ArtifactServer {
},
},
},

"my-node-3": wfv1.NodeStatus{
TemplateRef: &wfv1.TemplateRef{
Name: "my-template",
Template: "template-3",
},
Outputs: &wfv1.Outputs{
Artifacts: wfv1.Artifacts{
{
Name: "my-s3-artifact",
ArtifactLocation: wfv1.ArtifactLocation{
S3: &wfv1.S3Artifact{
// S3 is a configured artifact repo, so does not need key
Key: "my-wf/my-node-3/my-s3-artifact.tgz",
S3Bucket: wfv1.S3Bucket{
Bucket: "my-bucket-4",
Endpoint: "minio:9000",
},
},
},
},
},
},
},
// a node without input/output artifacts
"my-node-no-artifacts": wfv1.NodeStatus{},
},
StoredTemplates: map[string]wfv1.Template{
"namespaced/my-template/template-3": {
Name: "template-3",
Outputs: wfv1.Outputs{
Artifacts: wfv1.Artifacts{
{
Name: "my-s3-artifact",
Path: "my-s3-artifact.tgz",
},
},
},
},
},
},
}
argo := fakewfv1.NewSimpleClientset(wf, &wfv1.Workflow{
Expand Down Expand Up @@ -448,6 +488,38 @@ func TestArtifactServer_GetOutputArtifact(t *testing.T) {
}
}

func TestArtifactServer_GetOutputArtifactWithTemplate(t *testing.T) {
s := newServer()

tests := []struct {
fileName string
artifactName string
}{
{
fileName: "my-s3-artifact.tgz",
artifactName: "my-s3-artifact",
},
}

for _, tt := range tests {
t.Run(tt.artifactName, func(t *testing.T) {
r := &http.Request{}
r.URL = mustParse(fmt.Sprintf("/artifacts/my-ns/my-wf/my-node-3/%s", tt.artifactName))
recorder := httptest.NewRecorder()

s.GetOutputArtifact(recorder, r)
if assert.Equal(t, 200, recorder.Result().StatusCode) {
assert.Equal(t, fmt.Sprintf(`filename="%s"`, tt.fileName), recorder.Header().Get("Content-Disposition"))
all, err := io.ReadAll(recorder.Result().Body)
if err != nil {
panic(fmt.Sprintf("failed to read http body: %v", err))
}
assert.Equal(t, "my-data", string(all))
}
})
}
}

func TestArtifactServer_GetInputArtifact(t *testing.T) {
s := newServer()

Expand Down
4 changes: 2 additions & 2 deletions workflow/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ func getDescendantNodeIDs(wf *wfv1.Workflow, node wfv1.NodeStatus) []string {
}

func deletePodNodeDuringRetryWorkflow(wf *wfv1.Workflow, node wfv1.NodeStatus, deletedPods map[string]bool, podsToDelete []string) (map[string]bool, []string) {
templateName := getTemplateFromNode(node)
templateName := GetTemplateFromNode(node)
version := GetWorkflowPodNameVersion(wf)
podName := PodName(wf.Name, node.Name, templateName, node.ID, version)
if _, ok := deletedPods[podName]; !ok {
Expand Down Expand Up @@ -999,7 +999,7 @@ func resetNode(node wfv1.NodeStatus) wfv1.NodeStatus {
return node
}

func getTemplateFromNode(node wfv1.NodeStatus) string {
func GetTemplateFromNode(node wfv1.NodeStatus) string {
if node.TemplateRef != nil {
return node.TemplateRef.Template
}
Expand Down
2 changes: 1 addition & 1 deletion workflow/util/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1083,7 +1083,7 @@ func TestGetTemplateFromNode(t *testing.T) {
}

for _, tc := range cases {
actual := getTemplateFromNode(tc.inputNode)
actual := GetTemplateFromNode(tc.inputNode)
assert.Equal(t, tc.expectedTemplateName, actual)
}
}
Expand Down