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

feat: Support GetWorkflow regardless of its archival status #11055

Merged
merged 3 commits into from
May 22, 2023
Merged

feat: Support GetWorkflow regardless of its archival status #11055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a02e3e
feat: Support GetWorkflow regardless of its archival status
terrytangyuan May 8, 2023
5851441
fix: Address feedback
terrytangyuan May 16, 2023
67b163e
Merge branch 'master' into db-api
sarabala1979 May 21, 2023
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
5 changes: 5 additions & 0 deletions api/openapi-spec/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,11 @@
"type": "string",
"name": "namespace",
"in": "query"
},
{
"type": "string",
"name": "name",
"in": "query"
}
],
"responses": {
Expand Down
18 changes: 9 additions & 9 deletions persist/sqldb/mocks/WorkflowArchive.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion persist/sqldb/null_workflow_archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func (r *nullWorkflowArchive) CountWorkflows(string, string, string, time.Time,
return 0, nil
}

func (r *nullWorkflowArchive) GetWorkflow(string) (*wfv1.Workflow, error) {
terrytangyuan marked this conversation as resolved.
Show resolved Hide resolved
func (r *nullWorkflowArchive) GetWorkflow(string, string, string) (*wfv1.Workflow, error) {
return nil, fmt.Errorf("getting archived workflows not supported")
}

Expand Down
48 changes: 40 additions & 8 deletions persist/sqldb/workflow_archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import (
"time"

log "github.com/sirupsen/logrus"
"google.golang.org/grpc/codes"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
"upper.io/db.v3"
"upper.io/db.v3/lib/sqlbuilder"

wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
sutils "github.com/argoproj/argo-workflows/v3/server/utils"
"github.com/argoproj/argo-workflows/v3/util/instanceid"
)

Expand Down Expand Up @@ -57,7 +59,7 @@ type WorkflowArchive interface {
// list workflows, with the most recently started workflows at the beginning (i.e. index 0 is the most recent)
ListWorkflows(namespace string, name string, namePrefix string, minStartAt, maxStartAt time.Time, labelRequirements labels.Requirements, limit, offset int) (wfv1.Workflows, error)
CountWorkflows(namespace string, name string, namePrefix string, minStartAt, maxStartAt time.Time, labelRequirements labels.Requirements) (int64, error)
GetWorkflow(uid string) (*wfv1.Workflow, error)
GetWorkflow(uid string, namespace string, name string) (*wfv1.Workflow, error)
DeleteWorkflow(uid string) error
DeleteExpiredWorkflows(ttl time.Duration) error
IsEnabled() bool
Expand Down Expand Up @@ -257,14 +259,44 @@ func namePrefixClause(namePrefix string) db.Cond {
}
}

func (r *workflowArchive) GetWorkflow(uid string) (*wfv1.Workflow, error) {
func (r *workflowArchive) GetWorkflow(uid string, namespace string, name string) (*wfv1.Workflow, error) {
var err error
archivedWf := &archivedWorkflowRecord{}
err := r.session.
Select("workflow").
From(archiveTableName).
Where(r.clusterManagedNamespaceAndInstanceID()).
And(db.Cond{"uid": uid}).
One(archivedWf)
if uid != "" {
err = r.session.
Select("workflow").
From(archiveTableName).
Where(r.clusterManagedNamespaceAndInstanceID()).
And(db.Cond{"uid": uid}).
One(archivedWf)
} else {
if name != "" && namespace != "" {
total := &archivedWorkflowCount{}
err = r.session.
Select(db.Raw("count(*) as total")).
From(archiveTableName).
Where(r.clusterManagedNamespaceAndInstanceID()).
And(namespaceEqual(namespace)).
And(nameEqual(name)).
One(total)
if err != nil {
return nil, err
}
num := int64(total.Total)
if num > 1 {
return nil, fmt.Errorf("found %d archived workflows with namespace/name: %s/%s", num, namespace, name)
}
err = r.session.
Select("workflow").
From(archiveTableName).
Where(r.clusterManagedNamespaceAndInstanceID()).
And(namespaceEqual(namespace)).
And(nameEqual(name)).
One(archivedWf)
} else {
return nil, sutils.ToStatusError(fmt.Errorf("both name and namespace are required if uid is not specified"), codes.InvalidArgument)
}
}
if err != nil {
if err == db.ErrNoMoreRows {
return nil, nil
Expand Down
5 changes: 4 additions & 1 deletion pkg/apiclient/argo-kube-client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
cronworkflowserver "github.com/argoproj/argo-workflows/v3/server/cronworkflow"
"github.com/argoproj/argo-workflows/v3/server/types"
workflowserver "github.com/argoproj/argo-workflows/v3/server/workflow"
"github.com/argoproj/argo-workflows/v3/server/workflowarchive"
workflowtemplateserver "github.com/argoproj/argo-workflows/v3/server/workflowtemplate"
"github.com/argoproj/argo-workflows/v3/util/help"
"github.com/argoproj/argo-workflows/v3/util/instanceid"
Expand Down Expand Up @@ -83,7 +84,9 @@ func newArgoKubeClient(ctx context.Context, clientConfig clientcmd.ClientConfig,
}

func (a *argoKubeClient) NewWorkflowServiceClient() workflowpkg.WorkflowServiceClient {
return &errorTranslatingWorkflowServiceClient{&argoKubeWorkflowServiceClient{workflowserver.NewWorkflowServer(a.instanceIDService, argoKubeOffloadNodeStatusRepo)}}
wfArchive := sqldb.NullWorkflowArchive
wfaServer := workflowarchive.NewWorkflowArchiveServer(wfArchive)
return &errorTranslatingWorkflowServiceClient{&argoKubeWorkflowServiceClient{workflowserver.NewWorkflowServer(a.instanceIDService, argoKubeOffloadNodeStatusRepo, wfaServer)}}
}

func (a *argoKubeClient) NewCronWorkflowServiceClient() (cronworkflow.CronWorkflowServiceClient, error) {
Expand Down
153 changes: 102 additions & 51 deletions pkg/apiclient/workflowarchive/workflow-archive.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/apiclient/workflowarchive/workflow-archive.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ message ListArchivedWorkflowsRequest {
message GetArchivedWorkflowRequest {
string uid = 1;
string namespace = 2;
string name = 3;
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend creating a new API and deprecating the existing one for supporting backward capability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Introducing new API will bring more maintenance effort IMO. There are already multiple artifacts APIs that need to be refactored later. Perhaps we can add a parameter, e.g. lookUpArchived=true, to toggle this functionality to maintain compatibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moving discussions to #10781

Copy link
Member

Choose a reason for hiding this comment

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

Is it going to be a breaking change? Is there a way we can support this feature with backward compatibility?

Copy link
Member

Choose a reason for hiding this comment

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

The change would not be considered breaking since introducing fields would not break any existing behavior. We're introducing a new name field as part of the existing request to allow users to retrieve workflows by name/namespace, but existing users who are retrieving workflow by UID would not be affected.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @jessesuen Just had a discussion with @terrytangyuan. I request to test the using current SDK with the new Argo server(signature changes) whether it passes the "" for new arguments or user needs to update the new SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this with existing SDK against the new Argo Server and existing behavior preserves. Users can still use only UID to retrieve the archived workflow.

}
message DeleteArchivedWorkflowRequest {
string uid = 1;
Expand Down
6 changes: 4 additions & 2 deletions sdks/java/client/docs/ArchivedWorkflowServiceApi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ Name | Type | Description | Notes

<a name="archivedWorkflowServiceGetArchivedWorkflow"></a>
# **archivedWorkflowServiceGetArchivedWorkflow**
> IoArgoprojWorkflowV1alpha1Workflow archivedWorkflowServiceGetArchivedWorkflow(uid, namespace)
> IoArgoprojWorkflowV1alpha1Workflow archivedWorkflowServiceGetArchivedWorkflow(uid, namespace, name)



Expand Down Expand Up @@ -113,8 +113,9 @@ public class Example {
ArchivedWorkflowServiceApi apiInstance = new ArchivedWorkflowServiceApi(defaultClient);
String uid = "uid_example"; // String |
String namespace = "namespace_example"; // String |
String name = "name_example"; // String |
try {
IoArgoprojWorkflowV1alpha1Workflow result = apiInstance.archivedWorkflowServiceGetArchivedWorkflow(uid, namespace);
IoArgoprojWorkflowV1alpha1Workflow result = apiInstance.archivedWorkflowServiceGetArchivedWorkflow(uid, namespace, name);
System.out.println(result);
} catch (ApiException e) {
System.err.println("Exception when calling ArchivedWorkflowServiceApi#archivedWorkflowServiceGetArchivedWorkflow");
Expand All @@ -133,6 +134,7 @@ Name | Type | Description | Notes
------------- | ------------- | ------------- | -------------
**uid** | **String**| |
**namespace** | **String**| | [optional]
**name** | **String**| | [optional]

### Return type

Expand Down
Loading