Avoid occasional failures when using remote resolution

[l-qing]

fix #6408

When the time interval between two reconciliations of the
owner (TaskRun, PipelineRun) of a ResolutionRequest is short,
it may cause the second reconciliation to fail when triggering
a Submit because the informer cache may not have been updated yet.

In this case, we can assume that it is in progress, and the next
reconciliation will handle it based on the actual situation.

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• Has Docs included if any changes are user facing
• Has Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)
• Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Avoid occasional failures of TaskRun/PipelineRun execution using remote resolution.

[tekton-robot]

Hi @l-qing. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[l-qing]

/kind bug

[chuangw6]

/assign

[QuanZhang-William]

/assign

[dibyom]

/ok-to-test

[l-qing]

/hold

I need to check my account settings before the final merge.

[QuanZhang-William]

Thanks @l-qing! Could you please also add an unit test for it?

[QuanZhang-William]

can we add the reason why it may already exist in the comment? I personally didn't get the idea until reading the commit message 😄

[l-qing]

Ok, I add a simple description.

[QuanZhang-William]

Thanks!

[l-qing]

/remove-hold

[l-qing]

Could you please also add an unit test for it?

Can I create a separate pull request to add this unit test?
I anticipate that writing this unit test may be a bit complex.

Previously, there were no unit tests in this package. This feature relies on some interfaces.
I usually use gomock to mock interfaces, but I'm uncertain if this method is acceptable in Tekton.
I need to see how your community typically write unit tests before I can create a unit test that is similar in style to yours.

My computer needs to be repaired this weekend, so I may not have time to work on it, but I should have time next weekend. 😁

[l-qing]

pinging @lbernick @imjasonh for approval

[lbernick]

Thank you for the fix @l-qing! I have a few questions:

• When you say "submitting resolution requests quickly", do you mean creating many PipelineRuns/TaskRuns using remote resolution in a short window of time?
• What cache are you referring to here?
• Do you have a sense of why duplicate ResolutionRequests are being created?
• I saw on the issue you mentioned "In my environment, fixing it this way can avoid that error." I'm curious if you were able to reproduce the error, and how you know that this fix resolved it?

I agree with @QuanZhang-William we should have unit tests for this but I see your point that this package is not really tested at all; I've created #6429 to track testing.

Can you please reword the release note to mention changes visible to users? Users don't typically interact with ResolutionRequests (or shouldn't have to); they mainly interact with TaskRuns/PipelineRuns using remote resolution

[l-qing]

Hi, @lbernick I have revised my comments and release note based on your suggestions.

Replying to your questions:

1. "submitting resolution requests quickly"

I mean the same request reconciles frequently, such as TaskRun/PipelineRun.
Because the same request will always have the same name.

Call chain:

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 328 to 339 in 40763d8

	func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1.TaskSpec, *resources.ResolvedTask, error) {
	ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "prepare")
	defer span.End()
	logger := logging.FromContext(ctx)
	tr.SetDefaults(ctx)
	// list VerificationPolicies for trusted resources
	vp, err := c.verificationPolicyLister.VerificationPolicies(tr.Namespace).List(labels.Everything())
	if err != nil {
	return nil, nil, fmt.Errorf("failed to list VerificationPolicies from namespace %s with error %v", tr.Namespace, err)
	}
	getTaskfunc := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, tr, vp)

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Lines 165 to 172 in 40763d8

	// resolveTask accepts an impl of remote.Resolver and attempts to
	// fetch a task with given name. An error is returned if the
	// remoteresource doesn't work or the returned data isn't a valid
	// v1beta1.TaskObject.
	func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1beta1.TaskKind, k8s kubernetes.Interface) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
	// Because the resolver will only return references with the same kind (eg ClusterTask), this will ensure we
	// don't accidentally return a Task with the same name but different kind.
	obj, configSource, err := resolver.Get(ctx, strings.TrimSuffix(strings.ToLower(string(kind)), "s"), name)

pipeline/pkg/remote/resolution/resolver.go

Lines 58 to 65 in 40763d8

	// Get implements remote.Resolver.
	func (resolver *Resolver) Get(ctx context.Context, _, _ string) (runtime.Object, *v1beta1.ConfigSource, error) {
	resolverName := remoteresource.ResolverName(resolver.resolverName)
	req, err := buildRequest(resolver.resolverName, resolver.owner, resolver.targetName, resolver.targetNamespace, resolver.params)
	if err != nil {
	return nil, nil, fmt.Errorf("error building request for remote resource: %w", err)
	}
	resolved, err := resolver.requester.Submit(ctx, resolverName, req)

pipeline/pkg/resolution/resource/crd_resource.go

Lines 51 to 57 in 40763d8

	// Submit constructs a ResolutionRequest object and submits it to the
	// kubernetes cluster, returning any errors experienced while doing so.
	// If ResolutionRequest is succeeded then it returns the resolved data.
	func (r *CRDRequester) Submit(ctx context.Context, resolver ResolverName, req Request) (ResolvedResource, error) {
	rr, _ := r.lister.ResolutionRequests(req.Namespace()).Get(req.Name())
	if rr == nil {
	if err := r.createResolutionRequest(ctx, resolver, req); err != nil {

2. "What cache are you referring to here?"

I mean the informer cache.

pipeline/pkg/client/resolution/informers/externalversions/resolution/v1alpha1/resolutionrequest.go

Lines 80 to 90 in 40763d8

	func (f *resolutionRequestInformer) defaultInformer(client versioned.Interface, resyncPeriod time.Duration) cache.SharedIndexInformer {
	return NewFilteredResolutionRequestInformer(client, f.namespace, resyncPeriod, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, f.tweakListOptions)
	}
	func (f *resolutionRequestInformer) Informer() cache.SharedIndexInformer {
	return f.factory.InformerFor(&resolutionv1alpha1.ResolutionRequest{}, f.defaultInformer)
	}
	func (f *resolutionRequestInformer) Lister() v1alpha1.ResolutionRequestLister {
	return v1alpha1.NewResolutionRequestLister(f.Informer().GetIndexer())
	}

3. "Do you have a sense of why duplicate ResolutionRequests are being created?"

This is bound to happen as long as a TaskRun that uses remote tasks reconciles quickly twice in its initial creation phase.

4. "how you know that this fix resolved it?"

Previously, this occurred very frequently in my environment. About 8 out of 10 times, it would fail due to this issue.
I added some logs and found that AlreadyExists error did occur.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, QuanZhang-William

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[lbernick]

Thank you for the detailed explanation, and for assigning yourself to the issue I created!

[l-qing]

@chitrangpatel Hi, could you please review again and give an LGTM label?

[chitrangpatel]

/lgtm

[l-qing]

@chitrangpatel Thank you!