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

Flipping the SinkBinding storage bit to v1beta1 #3613

Closed
wants to merge 2 commits into from

Conversation

nachocano
Copy link
Contributor

@nachocano nachocano commented Jul 16, 2020

Helps with #3544

Proposed Changes

  • 🎁 Storing SinkBindings in v1beta1

Missing the storage migration job

Release Note

SinkBinding is available in v1beta1

Docs

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 16, 2020
@nachocano
Copy link
Contributor Author

/assign @capri-xiyue

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 16, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2020
@capri-xiyue
Copy link
Contributor

/test pull-knative-eventing-upgrade-tests

Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also have an issue tracking the storage migration tasks?

config/core/resources/sinkbindings.yaml Outdated Show resolved Hide resolved
@nachocano
Copy link
Contributor Author

Do you also have an issue tracking the storage migration tasks?

I'll use the current issue to track it. I just edited the description here not to mark it as Fixed, and added an extra item there. Thanks for pointing this out

@capri-xiyue
Copy link
Contributor

capri-xiyue commented Jul 16, 2020

Not sure whether we can change storage version to v1beta1. I remember eventing uses v1alpha1 for storage version because of some upgrade issues.
In addition, the upgrade test keeps failing.

@lionelvillard
Copy link
Member

we usually wait one release before changing the storage version. I believe the upgrade test expects v1alpha1 to be first.

@knative-prow-robot
Copy link
Contributor

@nachocano: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-eventing-upgrade-tests f931eb9 link /test pull-knative-eventing-upgrade-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-knative-eventing-upgrade-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-upgrade-tests:

test/upgrade.TestContinuousEventsPropagationWithProber

@vaikas
Copy link
Contributor

vaikas commented Jul 16, 2020

LOL, I wrote this comment to a wrong PR:

Ok, so I don't think it's actually failing because of the reordering. It's failing because of this:

The CustomResourceDefinition "sinkbindings.sources.knative.dev" is invalid: status.storedVersions[1]: Invalid value: "v1beta1": must appear in spec.versions
ERROR: Knative latest release installation failed

So, what's happening is that at the release (.16) you're at v1alpha1 (stored version) and then tests upgrade to head and run the tests, all the new objects that get created during this test will now be at v1beta1 (stored version), so you can't then just downgrade to the .16 again (which doesn't have the v1beta1) and drop that version because you have objects that are being stored at that version. You'd have to do something like this:

  • switch the storage back to v1alpha1
  • run the storage migration tool to get all the resources back to v1alpha1 storage
  • drop the v1beta1

@nachocano
Copy link
Contributor Author

LOL, I wrote this comment to a wrong PR:

Ok, so I don't think it's actually failing because of the reordering. It's failing because of this:

The CustomResourceDefinition "sinkbindings.sources.knative.dev" is invalid: status.storedVersions[1]: Invalid value: "v1beta1": must appear in spec.versions
ERROR: Knative latest release installation failed

So, what's happening is that at the release (.16) you're at v1alpha1 (stored version) and then tests upgrade to head and run the tests, all the new objects that get created during this test will now be at v1beta1 (stored version), so you can't then just downgrade to the .16 again (which doesn't have the v1beta1) and drop that version because you have objects that are being stored at that version. You'd have to do something like this:

  • switch the storage back to v1alpha1
  • run the storage migration tool to get all the resources back to v1alpha1 storage
  • drop the v1beta1

LOL!
OK, got it, thanks for explaining @vaikas, makes much more sense.
As @lionelvillard mentioned, if we wait one release to change the storage version, then I'll close this PR for now. And I guess there is no need for the migration tool yet, right? That will come when be change the storage to v1beta1, post 0.17. Is that correct?

@nachocano
Copy link
Contributor Author

/close

Closing this as we will wait for one release before flipping the bit.
Will create a tracking issue for 0.18 to flip the storage and add the migration tool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants