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

3.5 Pagination may not work correctly for archived workflows #11715

Closed
2 of 3 tasks
terrytangyuan opened this issue Aug 30, 2023 · 7 comments · Fixed by #11761
Closed
2 of 3 tasks

3.5 Pagination may not work correctly for archived workflows #11715

terrytangyuan opened this issue Aug 30, 2023 · 7 comments · Fixed by #11761
Labels
area/api Argo Server API area/server area/workflow-archive P2 Important. All bugs with >=3 thumbs up that aren’t P0 or P1, plus: Any other bugs deemed important type/bug type/regression Regression from previous behavior (a specific type of bug)

Comments

@terrytangyuan
Copy link
Member

terrytangyuan commented Aug 30, 2023

Pre-requisites

  • I have double-checked my configuration
  • I can confirm the issues exists when I tested with :latest
  • I'd like to contribute the fix myself (see contributing guide)

What happened/what you expected to happen?

When we perform the merge of live workflows and archived workflows (in order to only show one of them in the UI), the listMeta in WorkflowList is obtained from live workflows. This may cause issues for pagination for archived workflows.

mergedWfsList := v1alpha1.WorkflowList{Items: mergedWfs, ListMeta: liveWfs.ListMeta}

We need to test the pagination more thoroughly and revisit this part of the code. https://github.com/argoproj/argo-workflows/blob/master/server/workflow/workflow_server.go#L155C2-L155C86

Version

latest

Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

TBA

Logs from the workflow controller

kubectl logs -n argo deploy/workflow-controller | grep ${workflow}

Logs from in your workflow's wait container

kubectl logs -n argo -c wait -l workflows.argoproj.io/workflow=${workflow},workflow.argoproj.io/phase!=Succeeded
@terrytangyuan
Copy link
Member Author

@sunyeongchoi Would you like to help take a look at this? Your recent PRs on pagination (#11703, #11684) brought my attention to this part of the code.

@sunyeongchoi
Copy link
Member

@terrytangyuan
Of course!! I will analyze the issue at hand :)

@agilgur5 agilgur5 added the type/regression Regression from previous behavior (a specific type of bug) label Aug 30, 2023
@sunyeongchoi
Copy link
Member

@terrytangyuan

Hello. I would like to share the results of my analysis on the issue so far.

func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.WorkflowListRequest) (*wfv1.WorkflowList, error) {
	wfClient := auth.GetWfClient(ctx)

	listOption := &metav1.ListOptions{}
	if req.ListOptions != nil {
		listOption = req.ListOptions
	}
	s.instanceIDService.With(listOption)
	wfList, err := wfClient.ArgoprojV1alpha1().Workflows(req.Namespace).List(ctx, *listOption)
	if err != nil {
		return nil, sutils.ToStatusError(err, codes.Internal)
	}

// In this part, always return err. Because archived workflows use int type continue not string type(workflows use).
	archivedWfList, err := s.wfArchiveServer.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{
		ListOptions: listOption,
		NamePrefix:  "",
		Namespace:   req.Namespace,
	})

	if err != nil {
		log.Warnf("unable to list archived workflows:%v", err)
	} else {
		if archivedWfList != nil {
			wfList = mergeWithArchivedWorkflows(*wfList, *archivedWfList, int(listOption.Limit))
		}
	}
...

In the given code, we cannot fetch data unconditionally in ListArchivedWorkflows. This is because the continue value used in ListArchivedWorkflows is of type int, whereas in Workflows, it's a string. As a result, it always leads to an error.

This error is being logged as follows:
server: time="2023-09-03T01:02:29.487Z" level=warning msg="unable to list archived workflows:rpc error: code = InvalidArgument desc = listOptions.continue must be int"

In essence, this problem arises because, as mentioned earlier, Workflows and ArchivedWorkflows use different pagination methods, and the issue occurs when we try to use the list options from Workflows in ArchivedWorkflows.

It would be a good idea for us to discuss how to address this issue together.
After considering various solutions, I will share them with you shortly. :)

@sunyeongchoi
Copy link
Member

@terrytangyuan

I have one question.

After version v3.4.10, there used to be a separate "archived workflows" page, but I'm curious why the current master branch does not have the "archived workflows" page.

@terrytangyuan
Copy link
Member Author

terrytangyuan commented Sep 2, 2023

Workflows and ArchivedWorkflows use different pagination methods, and the issue occurs when we try to use the list options from Workflows in ArchivedWorkflows.

Yes that's exactly what the issue is! Let me know if you can come up with solution to merge the two lists or pagination methods.

After version v3.4.10, there used to be a separate "archived workflows" page, but I'm curious why the current master branch does not have the "archived workflows" page.

Yes, we removed the archived workflows page and both types of workflows are now displayed in one list view.

@agilgur5
Copy link
Contributor

agilgur5 commented Feb 18, 2024

Re-opening this since #12068 reverted #11761

Also had a duplicate in #12647, which describes a specific bug that can occur due to this issue.

@agilgur5 agilgur5 reopened this Feb 18, 2024
@agilgur5 agilgur5 added P2 Important. All bugs with >=3 thumbs up that aren’t P0 or P1, plus: Any other bugs deemed important and removed P3 Low priority labels Feb 18, 2024
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 2, 2024
@agilgur5
Copy link
Contributor

This should be re-resolved by #12736 now

@agilgur5 agilgur5 changed the title Pagination may not work correctly for archived workflows 3.5 Pagination may not work correctly for archived workflows Apr 29, 2024
@argoproj argoproj locked as resolved and limited conversation to collaborators Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api Argo Server API area/server area/workflow-archive P2 Important. All bugs with >=3 thumbs up that aren’t P0 or P1, plus: Any other bugs deemed important type/bug type/regression Regression from previous behavior (a specific type of bug)
Projects
None yet
4 participants