results
results copied to clipboard
tektoncd
fix: add stored status explicitly for logs
Changes
- added IsStored to LogStatus
- update the API Version to v1alpha3
- v1alpha2 will be removed later
- this will serve as a clear indication of if the logs have been stored, partially stored or not stored at all.
- this can be used to mitigate the race condition between pruning of runs and log storage.
Signed-off-by: Avinal Kumar [email protected]
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
- [x] Has Docs included if any changes are user facing
- [x] Has Tests included if any functionality added or changed
- [x] Tested your changes locally (if this is a code change)
- [x] Follows the commit message standard
- [x] Meets the Tekton contributor standards (including functionality, content, code)
- [x] 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 - [x] Release notes block below has been updated with any user-facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
- [x] Release notes contain the string "action required" if the change requires additional action from users switching to the new release
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
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
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 |
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 file based storage if the logs have been partially stored, we need to have a mechanism to clear them because the server will try to store the logs again, and those partially stored ones would be orphaned. we can also develop another mechanism to resume from where it broke.
In case of S3 or GCS the partial logs will be discarded.
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.
It seems we need to add WIP tag to this PR if it's still being worked on. Thanks.
I think v1alpha2 should be removed in a different PR or commit, please let me know if I should do that.
The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report
The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report
The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report
Can you have two commits here? One for API and other for status. It's easier to review then.
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
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.
The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report
@khrm @gabemontero I have broken the commits in two and also updated the PR description.
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% |
bump on my last round of comments @avinal : https://github.com/tektoncd/results/pull/704/files/505ae4eb5ed61f87d578495f972a1729f72fba62
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
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.
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% |
@avinal: PR needs rebase.
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.