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.

Sign up for GitHub

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

Jump to bottom

fix: use the execWf spec to determine artifactgc strategy #10066

Merged
merged 9 commits into from Nov 22, 2022
Merged

fix: use the execWf spec to determine artifactgc strategy #10066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ef10af9
fix: use the execWf spec to determine artifactgc strategy
juliev0 Nov 19, 2022
fa90417
fix: update test
juliev0 Nov 19, 2022
761011d
fix: empty commit
juliev0 Nov 19, 2022
8765cdf
fix: artifact gc with referred template
juliev0 Nov 19, 2022
c05cf27
fix: remove comment
juliev0 Nov 19, 2022
7135f3f
fix: empty commit
juliev0 Nov 19, 2022
ade005d
chore: merge master
juliev0 Nov 19, 2022
309dad0
fix: empty commit
juliev0 Nov 19, 2022
f79b57c
Merge branch 'master' into 10015-artifactgctemplate
sarabala1979 Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions test/e2e/artifacts_test.go
View file
Open in desktop
Expand Up @@ -69,6 +69,7 @@ func (s *ArtifactsSuite) TestArtifactGC() {

s.Given().
WorkflowTemplate("@testdata/artifactgc/artgc-template.yaml").
WorkflowTemplate("@testdata/artifactgc/artgc-template-2.yaml").
When().
CreateWorkflowTemplates()

Expand Down Expand Up @@ -96,6 +97,14 @@ func (s *ArtifactsSuite) TestArtifactGC() {
artifactState{"on-deletion", "my-bucket-2", false, true},
},
},
{
workflowFile: "@testdata/artifactgc/artgc-from-template-2.yaml",
expectedGCPodsOnWFCompletion: 1,
expectedArtifacts: []artifactState{
artifactState{"on-completion", "my-bucket-2", true, false},
artifactState{"on-deletion", "my-bucket-2", false, true},
},
},
{
workflowFile: "@testdata/artifactgc/artgc-step-wf-tmpl.yaml",
expectedGCPodsOnWFCompletion: 1,
Expand All @@ -104,6 +113,14 @@ func (s *ArtifactsSuite) TestArtifactGC() {
artifactState{"on-deletion", "my-bucket-2", false, true},
},
},
{
workflowFile: "@testdata/artifactgc/artgc-step-wf-tmpl-2.yaml",
expectedGCPodsOnWFCompletion: 1,
expectedArtifacts: []artifactState{
artifactState{"on-completion", "my-bucket-2", true, false},
artifactState{"on-deletion", "my-bucket-2", false, false},
},
},
} {
// for each test make sure that:
// 1. the finalizer gets added
Expand Down
8 changes: 8 additions & 0 deletions test/e2e/testdata/artifactgc/artgc-from-template-2.yaml
View file
Open in desktop
@@ -0,0 +1,8 @@
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: artgc-from-template-2-
spec:
workflowTemplateRef:
name: artgc-template-2
clusterScope: false
15 changes: 15 additions & 0 deletions test/e2e/testdata/artifactgc/artgc-step-wf-tmpl-2.yaml
View file
Open in desktop
@@ -0,0 +1,15 @@
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: artgc-step-wf-tmpl-
spec:
entrypoint: artgc-step-wf-tmpl
templates:
- name: artgc-step-wf-tmpl
steps:
- - name: call-template
templateRef:
name: artgc-template-2
template: artgc-template
clusterScope: false

52 changes: 52 additions & 0 deletions test/e2e/testdata/artifactgc/artgc-template-2.yaml
View file
Open in desktop
@@ -0,0 +1,52 @@
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
name: artgc-template-2
spec:
artifactGC:
strategy: OnWorkflowDeletion
workflowMetadata:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unlike the WorkflowTemplate test that was already in there, this one includes a strategy specified on the Workflow level

labels:
workflows.argoproj.io/test: "true"
entrypoint: artgc-template
templates:
- name: artgc-template
container:
image: argoproj/argosay:v2
command:
- sh
- -c
args:
- |
echo "hello world"
echo "hello world" > /tmp/message
outputs:
artifacts:
- name: on-completion
path: /tmp/message
s3:
key: on-completion
bucket: my-bucket-2
endpoint: minio:9000
insecure: true
accessKeySecret:
name: my-minio-cred
key: accesskey
secretKeySecret:
name: my-minio-cred
key: secretkey
artifactGC:
strategy: OnWorkflowCompletion
- name: on-deletion
path: /tmp/message
s3:
key: on-deletion
bucket: my-bucket-2
endpoint: minio:9000
insecure: true
accessKeySecret:
name: my-minio-cred
key: accesskey
secretKeySecret:
name: my-minio-cred
key: secretkey
34 changes: 26 additions & 8 deletions workflow/controller/artifact_gc.go
View file
Open in desktop
Expand Up @@ -104,11 +104,7 @@ func (woc *wfOperationCtx) processArtifactGCStrategy(ctx context.Context, strate
woc.log.Debugf("processing Artifact GC Strategy %s", strategy)

// Search for artifacts
artifactSearchResults := woc.wf.SearchArtifacts(&wfv1.ArtifactSearchQuery{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just change to wow.execWF.SearchArtifacts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that at first but the execWf doesn’t seem to include the Workflow Status (for node status). Should it?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct, you must inspect woc.execWF.Spec and woc.wf.Status. The former is what to run, the latter what to update at the end of reconciliation.

You should not query across the two.

Copy link
Contributor Author

@juliev0 juliev0 Nov 20, 2022 •
edited

Choose a reason for hiding this comment

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

Does this look okay then?

Copy link
Member

Choose a reason for hiding this comment

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

@juliev0 Make sure you include the test for workflowdefautls, workflow template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarabala1979 I just ran a couple of tests locally to test WorkflowDefaults. In one, I just used a Workflow by itself and verified that it took the default from WorkflowDefaults. In the second, I used a WorkflowTemplate as well. As discussed, since the workflow-controller-configmap is not being dynamically reloaded, it's difficult to test this as part of the e2e test, since otherwise I need to change test/e2e/manifests/mixins/workflow-controller-configmap.yaml, which could have consequences for other e2e tests.

ArtifactGCStrategies: map[wfv1.ArtifactGCStrategy]bool{strategy: true},
Deleted: pointer.BoolPtr(false),
NodeTypes: map[wfv1.NodeType]bool{wfv1.NodeTypePod: true},
})
artifactSearchResults := woc.findArtifactsToGC(strategy)
if len(artifactSearchResults) == 0 {
woc.log.Debugf("No Artifact Search Results returned from strategy %s", strategy)
return nil
Expand Down Expand Up @@ -151,16 +147,16 @@ func (woc *wfOperationCtx) processArtifactGCStrategy(ctx context.Context, strate
}
templateName := node.TemplateName
if templateName == "" && node.GetTemplateRef() != nil {
templateName = node.GetTemplateRef().Name
templateName = node.GetTemplateRef().Template
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was accidentally referring to the WorkflowTemplate's Name here rather than the name of the template within the WorkflowTemplate

if templateName == "" {
return fmt.Errorf("can't process Artifact GC Strategy %s: node %+v has an unnamed template", strategy, node)
}
template, found := templatesByName[templateName]
if !found {
template = woc.wf.GetTemplateByName(templateName)
template = woc.execWf.GetTemplateByName(templateName)
if template == nil {
return fmt.Errorf("can't process Artifact GC Strategy %s: template name %q belonging to node %+v not found??", strategy, node.TemplateName, node)
return fmt.Errorf("can't process Artifact GC Strategy %s: template name %q belonging to node %+v not found??", strategy, templateName, node)
Copy link
Contributor Author

@juliev0 juliev0 Nov 20, 2022 •
edited

Choose a reason for hiding this comment

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

note: I confirmed that I no longer see any more cases of woc.wf used in this file that should really be woc.execWf

}
templatesByName[templateName] = template
}
Expand Down Expand Up @@ -538,6 +534,28 @@ func (woc *wfOperationCtx) allArtifactsDeleted() bool {
return true
}

func (woc *wfOperationCtx) findArtifactsToGC(strategy wfv1.ArtifactGCStrategy) wfv1.ArtifactSearchResults {

var results wfv1.ArtifactSearchResults

for _, n := range woc.wf.Status.Nodes {

if n.Type != wfv1.NodeTypePod {
continue
}
for _, a := range n.GetOutputs().GetArtifacts() {

// artifact strategy is either based on overall Workflow ArtifactGC Strategy, or
// if it's specified on the individual artifact level that takes priority
artifactStrategy := woc.execWf.GetArtifactGCStrategy(&a)
if artifactStrategy == strategy && !a.Deleted {
results = append(results, wfv1.ArtifactSearchResult{Artifact: a, NodeID: n.ID})
}
}
}
return results
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use woc.execWf to get the strategy defined in the WorkflowTemplate if there is one


func (woc *wfOperationCtx) processCompletedArtifactGCPod(ctx context.Context, pod *corev1.Pod) error {
woc.log.Infof("processing completed Artifact GC Pod %q", pod.Name)

Expand Down