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

Remove error when converting Finally field to alpha1 #3757

Closed
wants to merge 1 commit into from
Closed

Remove error when converting Finally field to alpha1 #3757

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 11, 2021

Changes

Fixes #3206

/hold

I'm posting this PR because I don't know what the right thing to do here is. Looking for feedback from anyone who knows the conversion webhook stuff better than me.

Prior to this commit pipelines returned an error when converting from
a v1beta1 pipeline to a v1alpha1 pipeline if that pipeline had a Finally
section in it. As a result of this error the kubeapi would repeatedly
request v1alpha1 pipelines every second, filling up our webhook logs
with these errors.

This is the place that kubernetes is repeatedly hitting us with conversion attempts.

This commit removes the error when downgrading a beta1 resource to alpha1.
I actually have no idea if this is the correct thing to do and I can't find
any docs explaining the dos and donts of conversion. This stops the spam
though.

/kind bug

Submitter Checklist

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

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Release Notes

Fix issue where webhook logs would rapidly fill up with conversion errors when a pipeline with Finally was applied to the cluster.

Prior to this commit pipelines returned an error when converting from
a v1beta1 pipeline to a v1alpha1 pipeline if that pipeline had a Finally
section in it. As a result of this error the kubeapi would repeatedly
request v1alpha1 pipelines every second, filling up our webhook logs
with these errors.

This commit removes the error when downgrading a beta1 resource to alpha1.
I actually have no idea if this is the correct thing to do and I can't find
any docs explaining the dos and donts of conversion. This stops the spam
though.
@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 11, 2021
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sbwsg
You can assign the PR to them by writing /assign @sbwsg in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 11, 2021
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/pipeline_conversion.go 96.6% 96.5% -0.1

@afrittoli
Copy link
Member

I'm happy to be convinced otherwise, but as far as I understand the problem, I think this approach is not an option.
If we returned an incomplete resource to an alpha client and not raise an error, the client would have no way of knowing that the resource is incomplete. It could alter it an push it back to the cluster, effectively deleting parts of it.
Even if the client was aware of the error, it would not have any use of an incomplete resource.

@ghost
Copy link
Author

ghost commented Feb 12, 2021

Capturing all the ideas that have been generated during a lengthy Slack discussion on this topic:

  1. Could we disable the kube api's caching of v1alpha1 resources so that we can continue to return errors to v1alpha1 users? This would be ideal because then those users are aware they've "lost" information in the downgrade.
  2. Could we backport Finally to v1alpha1?
    • A problem we might face here is that old clients who haven't been recompiled with the backported support will continue to ignore "finally" and potentially clobber the beta resource if they attempt to re-apply the received version
  3. Could we drop support for v1alpha1 entirely at this point?
    • This would stop the errors, prevent bad versions from being applyied to the cluster, and make life generally simpler going forward.
    • We still need to learn lessons from this going forward to v1 and beyond.
  4. Could we convert down from beta's Finally to alpha Tasks while preserving the "finally"-ness somehow?
    • Use a runAfter in the ex-Finally Tasks so they're scheduled at the end of the v1alpha1 Pipeline
    • Add an annotation on the Pipeline that records the mapping of v1lpha1 Task names to v1beta1 Finally Task names
    • Re-hydrate Finally if applying a v1alpha1 Pipeline w/ the annotations to the server
  5. Could we "poison" the v1alpha1 Pipeline we return so that it's cached without error by the api server but fails validation if the user attempts to re-apply the v1alpha1 version?
    • E.g. inject an additional "fake" PipelineTask with an invalid name that will trigger validation errors on re-submission

@ghost
Copy link
Author

ghost commented Feb 22, 2021

Closing this in favor of #3779

@ghost ghost closed this Feb 22, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
2 participants