search

tektoncd / results

Long term storage of execution results.
Apache License 2.0
77 stars 73 forks source link

fix: add stored status explicitly for logs #704

Closed avinal closed 2 months ago

avinal commented 7 months ago

Changes

Signed-off-by: Avinal Kumar avinal@redhat.com

Submitter Checklist

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

Release Notes

action required: - the log status contains an extra field called `is_stored` to denote if the logs have been correctly stored or not
- Breaking Change: API Version is updated to v1alpha3 from v1alpha2
tekton-robot commented 7 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please ask for approval from avinal 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](https://github.com/tektoncd/results/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
avinal commented 7 months ago

/kind flake

tekton-robot commented 7 months ago

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 68.3% 66.7% -1.6
avinal commented 7 months ago

As discussed in the WG meeting, I have tested it for partial log store, and it seems there is only one issue (not ATM) we need to take care of.

In case of S3 or GCS the partial logs will be discarded.

avinal commented 7 months ago

cc @khrm @sayan-biswas @ramessesii2 @enarha

sayan-biswas commented 7 months ago

Please also keep in mind this is an API change, previously stored data will not have the field isStore and with the current design GetLog will fail even it the log is there.

ramessesii2 commented 7 months ago

It seems we need to add WIP tag to this PR if it's still being worked on. Thanks.

avinal commented 6 months ago

I think v1alpha2 should be removed in a different PR or commit, please let me know if I should do that.

tekton-robot commented 6 months ago

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/v1alpha3/auth/impersonation/impersonation.go Do not exist 75.6%
pkg/api/server/v1alpha3/auth/nop.go Do not exist 0.0%
pkg/api/server/v1alpha3/auth/rbac.go Do not exist 75.0%
pkg/api/server/v1alpha3/lister/aggregator.go Do not exist 28.0%
pkg/api/server/v1alpha3/lister/filter.go Do not exist 92.3%
pkg/api/server/v1alpha3/lister/limit.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/lister.go Do not exist 15.8%
pkg/api/server/v1alpha3/lister/offset.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/order.go Do not exist 86.5%
pkg/api/server/v1alpha3/lister/page_token.go Do not exist 69.2%
pkg/api/server/v1alpha3/log/file.go Do not exist 78.6%
pkg/api/server/v1alpha3/log/gcs.go Do not exist 42.9%
pkg/api/server/v1alpha3/log/log.go Do not exist 72.7%
pkg/api/server/v1alpha3/log/s3.go Do not exist 38.2%
pkg/api/server/v1alpha3/logs.go Do not exist 67.4%
pkg/api/server/v1alpha3/ordering.go Do not exist 100.0%
pkg/api/server/v1alpha3/pagination.go Do not exist 100.0%
pkg/api/server/v1alpha3/record/record.go Do not exist 63.3%
pkg/api/server/v1alpha3/records.go Do not exist 87.1%
pkg/api/server/v1alpha3/result/result.go Do not exist 76.0%
pkg/api/server/v1alpha3/results.go Do not exist 88.9%
pkg/api/server/v1alpha3/server.go Do not exist 68.2%
pkg/api/server/v1alpha3/summary.go Do not exist 0.0%
tekton-robot commented 6 months ago

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/v1alpha3/auth/impersonation/impersonation.go Do not exist 75.6%
pkg/api/server/v1alpha3/auth/nop.go Do not exist 0.0%
pkg/api/server/v1alpha3/auth/rbac.go Do not exist 75.0%
pkg/api/server/v1alpha3/lister/aggregator.go Do not exist 28.0%
pkg/api/server/v1alpha3/lister/filter.go Do not exist 92.3%
pkg/api/server/v1alpha3/lister/limit.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/lister.go Do not exist 15.8%
pkg/api/server/v1alpha3/lister/offset.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/order.go Do not exist 86.5%
pkg/api/server/v1alpha3/lister/page_token.go Do not exist 69.2%
pkg/api/server/v1alpha3/log/file.go Do not exist 78.6%
pkg/api/server/v1alpha3/log/gcs.go Do not exist 42.9%
pkg/api/server/v1alpha3/log/log.go Do not exist 72.7%
pkg/api/server/v1alpha3/log/s3.go Do not exist 38.2%
pkg/api/server/v1alpha3/logs.go Do not exist 67.4%
pkg/api/server/v1alpha3/ordering.go Do not exist 100.0%
pkg/api/server/v1alpha3/pagination.go Do not exist 100.0%
pkg/api/server/v1alpha3/record/record.go Do not exist 63.3%
pkg/api/server/v1alpha3/records.go Do not exist 87.1%
pkg/api/server/v1alpha3/result/result.go Do not exist 76.0%
pkg/api/server/v1alpha3/results.go Do not exist 88.9%
pkg/api/server/v1alpha3/server.go Do not exist 68.2%
pkg/api/server/v1alpha3/summary.go Do not exist 0.0%
tekton-robot commented 6 months ago

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/v1alpha3/auth/impersonation/impersonation.go Do not exist 75.6%
pkg/api/server/v1alpha3/auth/nop.go Do not exist 0.0%
pkg/api/server/v1alpha3/auth/rbac.go Do not exist 75.0%
pkg/api/server/v1alpha3/lister/aggregator.go Do not exist 28.0%
pkg/api/server/v1alpha3/lister/filter.go Do not exist 92.3%
pkg/api/server/v1alpha3/lister/limit.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/lister.go Do not exist 15.8%
pkg/api/server/v1alpha3/lister/offset.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/order.go Do not exist 86.5%
pkg/api/server/v1alpha3/lister/page_token.go Do not exist 69.2%
pkg/api/server/v1alpha3/log/file.go Do not exist 78.6%
pkg/api/server/v1alpha3/log/gcs.go Do not exist 42.9%
pkg/api/server/v1alpha3/log/log.go Do not exist 72.7%
pkg/api/server/v1alpha3/log/s3.go Do not exist 38.2%
pkg/api/server/v1alpha3/logs.go Do not exist 67.4%
pkg/api/server/v1alpha3/ordering.go Do not exist 100.0%
pkg/api/server/v1alpha3/pagination.go Do not exist 100.0%
pkg/api/server/v1alpha3/record/record.go Do not exist 63.3%
pkg/api/server/v1alpha3/records.go Do not exist 87.1%
pkg/api/server/v1alpha3/result/result.go Do not exist 76.0%
pkg/api/server/v1alpha3/results.go Do not exist 88.9%
pkg/api/server/v1alpha3/server.go Do not exist 68.2%
pkg/api/server/v1alpha3/summary.go Do not exist 0.0%
khrm commented 6 months ago

Can you have two commits here? One for API and other for status. It's easier to review then.

gabemontero commented 6 months ago

Why are we adding stored status for logs?

It is in relation to fixing https://github.com/tektoncd/results/issues/514 @khrm as the use of labels or annotations had undesirable side effects, but that should be explicitly stated in the PR's description, which as I look now, I don't see it.

Good catch @khrm

gabemontero commented 6 months ago

Can you have two commits here? One for API and other for status. It's easier to review then.

I agree ... fwiw @avinal what @khrm is calling for has been a best practice employed in various k8s / golang based projects.

tekton-robot commented 6 months ago

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/v1alpha3/auth/impersonation/impersonation.go Do not exist 75.6%
pkg/api/server/v1alpha3/auth/nop.go Do not exist 0.0%
pkg/api/server/v1alpha3/auth/rbac.go Do not exist 75.0%
pkg/api/server/v1alpha3/lister/aggregator.go Do not exist 28.0%
pkg/api/server/v1alpha3/lister/filter.go Do not exist 92.3%
pkg/api/server/v1alpha3/lister/limit.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/lister.go Do not exist 15.8%
pkg/api/server/v1alpha3/lister/offset.go Do not exist 100.0%
pkg/api/server/v1alpha3/lister/order.go Do not exist 86.5%
pkg/api/server/v1alpha3/lister/page_token.go Do not exist 69.2%
pkg/api/server/v1alpha3/log/file.go Do not exist 78.6%
pkg/api/server/v1alpha3/log/gcs.go Do not exist 42.9%
pkg/api/server/v1alpha3/log/log.go Do not exist 72.7%
pkg/api/server/v1alpha3/log/s3.go Do not exist 38.2%
pkg/api/server/v1alpha3/logs.go Do not exist 67.4%
pkg/api/server/v1alpha3/ordering.go Do not exist 100.0%
pkg/api/server/v1alpha3/pagination.go Do not exist 100.0%
pkg/api/server/v1alpha3/record/record.go Do not exist 63.3%
pkg/api/server/v1alpha3/records.go Do not exist 87.1%
pkg/api/server/v1alpha3/result/result.go Do not exist 76.0%
pkg/api/server/v1alpha3/results.go Do not exist 88.9%
pkg/api/server/v1alpha3/server.go Do not exist 68.2%
pkg/api/server/v1alpha3/summary.go Do not exist 0.0%
pkg/watcher/reconciler/dynamic/dynamic.go 64.2% 62.3% -1.9
avinal commented 6 months ago

@khrm @gabemontero I have broken the commits in two and also updated the PR description.

tekton-robot commented 5 months ago

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 68.4% 67.5% -0.9
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%
gabemontero commented 4 months ago

bump on my last round of comments @avinal : https://github.com/tektoncd/results/pull/704/files/505ae4eb5ed61f87d578495f972a1729f72fba62

gabemontero commented 4 months ago

also @avinal - just noticed something when looking at @khrm 's event PR .... should we be bumping the annotation for logs to v1alpha3 ?

See https://github.com/tektoncd/results/pull/748/files#diff-83680fa39b0bf1807ef48e2ec46f89b5ef89abf69a628ba8fac9a2e01554e473R12

avinal commented 4 months ago

also @avinal - just noticed something when looking at @khrm 's event PR .... should we be bumping the annotation for logs to v1alpha3 ?

Yes, that was the specific change we needed to differentiate v1alpha3 logs from v1alpha2.

tekton-robot commented 4 months ago

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 68.4% 67.5% -0.9
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 commented 4 months ago

@avinal: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
khrm commented 2 months ago

/close