argo-workflows icon indicating copy to clipboard operation
argo-workflows copied to clipboard

feat: Workflows list page performance improvements

Open sunyeongchoi opened this issue 1 year ago • 5 comments

Fixes #12025

Motivation

A performance issue occurred when merging the workflows and archived workflows pages into one place, and the logic was changed to resolve this.

Modifications

  • When archived, an archived label is attached to the k8s resource, so the performance problem caused by the entire search was solved by changing the logic using the labelSelector of k8s.
  • To manage the pagination values ​​of workflows and archived workflows separately, a value called paginationOptions was added to both the front and back.

Verification

  1. created a workflows resource with archived enabled.
  2. checked that the data was not displayed in duplicate.
  3. checked if it operates normally when changing the number of pages and clicking the next page and first page buttons.
  4. checked whether all test codes pass without any problems

sunyeongchoi avatar Nov 21 '23 15:11 sunyeongchoi

@sunyeongchoi are u able to include fix for https://github.com/argoproj/argo-workflows/issues/12161 too?

tooptoop4 avatar Nov 22 '23 02:11 tooptoop4

@sunyeongchoi are u able to include fix for #12161 too?

Yes. But I will take a look after this PR is completely finished.

sunyeongchoi avatar Nov 22 '23 23:11 sunyeongchoi

I found a bug in the code I implemented. (Cronjob, event, and cli commands were also using a workflow list, but I realized that this had not been considered in this PR.)

As a result, the current PR is not complete and plan to add additional commit.

However, I would like to receive a review to see if the PR I implemented is in line with the direction of resolving the issue.

for example, when combining k8s workflows and archived workflows to show them on one page, the PaginationOptions parameter was added to the parameters, and the names were set to WfContinue for k8s workflows continue and ArchivedContinue for archived workflows continue.

I would like to receive reviews on whether you think this name is right or if there are other good names.

sunyeongchoi avatar Dec 09 '23 07:12 sunyeongchoi

When archived, an archived label is attached to the k8s resource, so the performance problem caused by the entire search was solved by changing the logic using the labelSelector of k8s.

Could you elaborate why this would solve the performance problem? If the number of archived workflows is large, it seems like we would still encounter the performance issue.

terrytangyuan avatar Dec 13 '23 02:12 terrytangyuan

Could you elaborate why this would solve the performance problem? If the number of archived workflows is large, it seems like we would still encounter the performance issue.

Okay. A performance problem occurred because the total number of archived workflows was searched.

So the logic I implemented is as follows:

  1. If the label "Archived" is attached to k8s workflows, it means that it is stored in archived workflows. Therefore, by adding the labelSelector syntax to k8s workflows, workflows stored in archived workflows are not searched in k8s workflows. (In other words, the possibility that k8s workflows and archived workflows may overlap was resolved through kubernetes labelSelector.)
wfLabelSelector += fmt.Sprintf("%s!=%s", common.LabelKeyWorkflowArchivingStatus, "Archived")
  1. If 10 pieces of data are needed on one page, if 6 k8s workflows were searched in step 1, 10-6 = 4, that is, there is a possibility of searching 4 more data, so at this time, only search up to 4 archived workflows.
if int64(len(mergedWfs)) < limit {
  listOption.Continue = archivedContinue
  listOption.Limit = limit - int64(len(mergedWfs))
  listOption.LabelSelector = labelSelector
  archivedWfList, err = s.wfArchiveServer.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{
    ListOptions: listOption,
    NamePrefix:  "",
    Namespace:   req.Namespace,
  })
}
  1. As described in number 1, deduplication was solved through Kubernetes' labelSelector, and the page limit was solved as described in number 2, so if k8s workflows and archived workflows manage only the continue value for each pagination separately, the performance problem will be solved because k8s workflows and archived workflows use their own pagination logic.
res := &wfv1.WorkflowList{ListMeta: metav1.ListMeta{ResourceVersion: wfList.ResourceVersion}, Items: mergedWfs, PaginationOptions: wfv1.WorkflowPaginationOptions{
  WfContinue:       common.CheckNilString(wfList.ListMeta.Continue),
  ArchivedContinue: common.CheckNilString(archivedWfList.ListMeta.Continue),
}}

sunyeongchoi avatar Dec 14 '23 00:12 sunyeongchoi

hi @sunyeongchoi , could you fix the DCO signature

tczhao avatar Jan 25 '24 23:01 tczhao

hi @sunyeongchoi , could you fix the DCO signature

This still needs more work in any case, per above review.

Also per my last comment on the issue, Sun Yeong will be busy a few more days at least, please allow for some more time here. Per the same comment, we will have a front-end workaround out sooner

agilgur5 avatar Jan 26 '24 00:01 agilgur5

hi @sunyeongchoi , could you fix the DCO signature

This still needs more work in any case, per above review.

Also per my last comment on the issue, Sun Yeong will be busy a few more days at least, please allow for some more time here. Per the same comment, we will have a front-end workaround out sooner

Thanks for the update. I look forward to the front-end workaround

tczhao avatar Jan 26 '24 21:01 tczhao

https://github.com/argoproj/argo-workflows/issues/12025#issuecomment-1949330952

I believe the concept of the issue has completely changed, so if everyone agrees, I will close this PR.

sunyeongchoi avatar Feb 26 '24 23:02 sunyeongchoi

Thank you so much for your efforts so far!

terrytangyuan avatar Feb 27 '24 00:02 terrytangyuan