Add log stored status explicitly #760

khrm · 2024-06-06T09:03:50Z

adding log stored stored status explictly in the Log object improves the detection for partial or no storage of logs
it might help mitigate the race condition between pruning the runs and storing the logs.

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Tested your changes locally (if this is a code change)
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add 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 contain the string "action required" if the change requires additional action from users switching to the new release

Release Notes

khrm

/assign @gabemontero @enarha

tekton-robot · 2024-06-06T09:04:18Z

@khrm: GitHub didn't allow me to assign the following users: gabemontero.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @gabemontero @enarha

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.

khrm · 2024-06-06T09:05:19Z

pkg/apis/v1alpha3/types.go


 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 )

 // LogRecordType represents the API resource type for Tekton log records.
-const LogRecordType = "results.tekton.dev/v1alpha2.Log"
+const LogRecordType = "results.tekton.dev/v1alpha3.Log"
+const LogRecordTypeV2 = "results.tekton.dev/v1alpha2.Log"


We need this also because some of the Type stored in DB can be v2.

ack @khrm good catch

khrm · 2024-06-06T09:06:27Z

pkg/api/server/v1alpha2/log/log.go

-func ToStream(ctx context.Context, record *db.Record, config *config.Config) (Stream, *v1alpha2.Log, error) {
-	if record.Type != v1alpha2.LogRecordType {
+func ToStream(ctx context.Context, record *db.Record, config *config.Config) (Stream, *v1alpha3.Log, error) {
+	if record.Type != v1alpha3.LogRecordType || record.Type != v1alpha3.LogRecordTypeV2 {


We need V2 here because it can be v2 also. So I added that,

khrm

/kind feature

tekton-robot · 2024-06-06T09:08:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	50.7%	-12.7
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	25.0%
pkg/apis/v1alpha3/types.go	Do not exist	75.0%

tekton-robot · 2024-06-06T09:22:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	50.7%	-12.7
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	100.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%

tekton-robot · 2024-06-06T11:27:01Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	50.7%	-12.7
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	100.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%

khrm · 2024-06-06T11:56:08Z

/test pull-tekton-results-build-tests

tekton-robot · 2024-06-06T12:01:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	50.7%	-12.7
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	100.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%

khrm · 2024-06-06T12:09:00Z

/test pull-tekton-results-unit-tests

tekton-robot · 2024-06-06T12:12:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	66.2%	2.8
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%
pkg/apis/v1alpha3/types.go	Do not exist	50.0%

khrm · 2024-06-06T13:12:13Z

/test pull-tekton-results-integration-tests

khrm · 2024-06-06T13:13:44Z

/test pull-tekton-results-build-tests

khrm · 2024-06-06T13:17:21Z

/test pull-tekton-results-unit-tests

khrm

/assign @gabemontero

tekton-robot · 2024-06-06T13:18:42Z

@khrm: GitHub didn't allow me to assign the following users: gabemontero.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @gabemontero

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.

tekton-robot · 2024-06-06T13:19:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	62.6%	-0.8
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	100.0%
pkg/apis/v1alpha3/types.go	Do not exist	100.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%

khrm

/assign @gabemontero

tekton-robot · 2024-06-06T17:43:08Z

@khrm: GitHub didn't allow me to assign the following users: gabemontero.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @gabemontero

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.

khrm · 2024-06-06T17:45:03Z

/hold I am testing various scenarios atm. I will cancel the hold tomorrow after I finish testing.

gabemontero · 2024-06-06T18:59:17Z

pkg/apis/v1alpha3/types.go

+	Path            string `json:"path,omitempty"`
+	Size            int64  `json:"size"`
+	IsStored        bool   `json:"isStored"`
+	ErrorOnStoreMsg string `json:"errorOnStoreMsg"`


yeah per my comment https://github.com/tektoncd/results/pull/704/files#r1630064603 @khrm lets add an errorOnStoreCode int as well

@avinal @sayan-biswas fyi

Do we use errorcode for streaming? We aren't using grpc code during streaming.

@khrm consider the old prototype for using this change over in #713

I want that call in the watcher's dynamic.go to not just check if it is stored, and if it is not, requeue to consider pruning later.

I want it to also see that if it is not stored per the update made in pkg/api/server/v1alpha2/logs.go 's handleReturn (where it has the original "error"), but because of an error on the api server side, to see if the particular error is one of the grpc intermittent sort of hiccups that mean we can re-attempt the log storage.

In this way, we finally address the lack of retry on error that @sayan-biswas @avinal @enarha and myself have lamented for months.

Now, if there is some flaw in the way I've connected the dots here, then let's you @khrm, myself, @avinal @sayan-biswas @enarha discuss either here on in the next WG call.

As a backup, I'm also fine with setting a boolean "IsRetrablyErrorOccurred" field. in LogStatus, have the handleReturn code make the decision if the error that occured in an intermittent, retryable sort of thing, and then the dynamic.go code works off of that to try the UpdateLog call again.

thanks

I have added a new field for this.

LGTM @khrm thanks

@enarha @avinal @sayan-biswas as I am not in the OWNERS file anywhere, we need reviews from you all to get this finally merged.

PTAL

gabemontero

just the one additional field on the LogStatus obj @khrm

thanks

khrm · 2024-06-10T10:31:38Z

/hold cancel

khrm · 2024-06-10T10:56:30Z

I have also tested the following scenarios now:

Creating Prs with older results release.
We are upgrading to newer results and checking whether we can access v2alpha logs.
Creating PRs with this results release. I was able to access those logs.

tekton-robot · 2024-06-10T12:07:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	62.3%	-1.1
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%
pkg/apis/v1alpha3/types.go	Do not exist	75.0%

- adding log stored stored status explictly in the Log object improves the detection for partial or no storage of logs - it might help mitigate the race condition between pruning the runs and storing the logs. Signed-off-by: Avinal Kumar <avinal@redhat.com> Signed-off-by: Khurram Baig <kbaig@redhat.com>

tekton-robot · 2024-06-10T12:32:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/api/server/v1alpha2/logs.go	63.4%	62.3%	-1.1
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	0.0%
pkg/apis/v1alpha3/types.go	Do not exist	50.0%
pkg/apis/v1alpha3/types.go	Do not exist	100.0%
pkg/apis/v1alpha3/types.go	Do not exist	66.7%

tekton-robot · 2024-06-10T17:57:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gabemontero
To complete the pull request process, please ask for approval from enarha after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 6, 2024

tekton-robot requested review from avinal and dibyom June 6, 2024 09:03

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2024

khrm commented Jun 6, 2024

View reviewed changes

tekton-robot assigned enarha Jun 6, 2024

khrm commented Jun 6, 2024

View reviewed changes

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 6, 2024

khrm force-pushed the log-status branch from 811040b to 60fb4ee Compare June 6, 2024 09:18

khrm force-pushed the log-status branch from 60fb4ee to 53a503b Compare June 6, 2024 11:22

khrm force-pushed the log-status branch 2 times, most recently from 4b5450e to 28d50ea Compare June 6, 2024 11:55

khrm force-pushed the log-status branch from 28d50ea to c25b6ef Compare June 6, 2024 12:08

khrm force-pushed the log-status branch from c25b6ef to f824a58 Compare June 6, 2024 13:11

khrm force-pushed the log-status branch from f824a58 to c882ded Compare June 6, 2024 13:13

khrm force-pushed the log-status branch from c882ded to f2f1a44 Compare June 6, 2024 13:14

khrm commented Jun 6, 2024

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2024

gabemontero reviewed Jun 6, 2024

View reviewed changes

gabemontero suggested changes Jun 6, 2024

View reviewed changes

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2024

khrm force-pushed the log-status branch from f2f1a44 to fcb824a Compare June 10, 2024 12:03

khrm force-pushed the log-status branch from fcb824a to d59e8f6 Compare June 10, 2024 12:28

gabemontero approved these changes Jun 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add log stored status explicitly #760

Add log stored status explicitly #760

khrm commented Jun 6, 2024 •

edited

khrm left a comment

tekton-robot commented Jun 6, 2024

khrm Jun 6, 2024

gabemontero Jun 6, 2024

khrm Jun 6, 2024

khrm left a comment

tekton-robot commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

khrm commented Jun 6, 2024

khrm commented Jun 6, 2024

khrm left a comment

tekton-robot commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm left a comment

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

gabemontero Jun 6, 2024

khrm Jun 7, 2024

gabemontero Jun 7, 2024

khrm Jun 10, 2024

gabemontero Jun 10, 2024

gabemontero left a comment

khrm commented Jun 10, 2024

khrm commented Jun 10, 2024

tekton-robot commented Jun 10, 2024

tekton-robot commented Jun 10, 2024

tekton-robot commented Jun 10, 2024

Add log stored status explicitly #760

Are you sure you want to change the base?

Add log stored status explicitly #760

Conversation

khrm commented Jun 6, 2024 • edited

Changes

Submitter Checklist

Release Notes

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

khrm commented Jun 6, 2024

khrm commented Jun 6, 2024

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 6, 2024

tekton-robot commented Jun 6, 2024

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 6, 2024

khrm commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero left a comment

Choose a reason for hiding this comment

khrm commented Jun 10, 2024

khrm commented Jun 10, 2024

tekton-robot commented Jun 10, 2024

tekton-robot commented Jun 10, 2024

tekton-robot commented Jun 10, 2024

khrm commented Jun 6, 2024 •

edited