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

Handling v1 storage conversion #6382

Closed
vdemeester opened this issue Mar 17, 2023 · 11 comments
Closed

Handling v1 storage conversion #6382

vdemeester opened this issue Mar 17, 2023 · 11 comments
Assignees
Labels
area/api Indicates an issue or PR that deals with the API. area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) kind/feature Categorizes issue or PR as related to a new feature.

Comments

@vdemeester
Copy link
Member

vdemeester commented Mar 17, 2023

When we will switch the storage version to v1, we need to handle the storage version migration.

For context, when we switch storage version from one version to the other, all the object stored with a previous version will be stored as this until they are updated.

To illustrate this, consider the following hypothetical series of events:

  1. The storage version is v1beta1. You create an object. It is stored at version v1beta1
  2. You add version v1 to your CustomResourceDefinition and designate it as the storage version. Here the schemas for v1 and v1beta1 are identical, which is typically the case when promoting an API to stable in the Kubernetes ecosystem.
  3. You read your object at version v1beta1, then you read the object again at version v1. Both returned objects are identical except for the apiVersion field.
  4. You create a new object. It is stored at version v1. You now have two objects, one of which is at v1beta1, and the other of which is at v1.
  5. You update the first object. It is now stored at version v1 since that is the current storage version.

From Versions In CustomResourceDefinition

This is not necessarily a problem until we remove that version from the served version (aka, in the case of v1, when v1beta1 is no more served). This is what happened with v1alpha1 and can cause some issues like CustomResourceDefinition.apiextensions.k8s.io "clustertasks.tekton.dev" is invalid: status.StoredVersions[0]: Invalid value: "v1alpha1": must appear in spec.versions.

This problem is highlighted by the fact that that command we give in our release pages is kubectl apply which will merge they payload with previous objects in the cluster. This means, if I had an "old" version that had storedVersion set to v1alpha1, when applying a version where storedVersion is now set to v1beta1 (in the release.yaml), it will have both v1alpha1 and v1beta1 as storedVersion — which is a case.. we don't handle pretty well. _This is gonna happen for v1 as well, when we switch the storedVersion.

There is a "few" things we need to cover here:

@vdemeester vdemeester added kind/feature Categorizes issue or PR as related to a new feature. area/api Indicates an issue or PR that deals with the API. area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) labels Mar 17, 2023
@vdemeester
Copy link
Member Author

cc @tektoncd/operator-maintainers as this is something the operator can/should take care of as well (the migration I mean)

@vdemeester vdemeester added this to the Pipelines v0.47 milestone Mar 23, 2023
@vdemeester
Copy link
Member Author

cc @tektoncd/core-maintainers

@lbernick lbernick moved this to Todo in Pipelines V1 Mar 23, 2023
@vdemeester vdemeester assigned vdemeester and unassigned vdemeester Mar 24, 2023
@lbernick
Copy link
Member

@vdemeester can we use this issue to track swapping to using v1 as the storage version, or is this meant to track work (e.g. storage version migration) we'll do after that's completed?

@vdemeester
Copy link
Member Author

@vdemeester can we use this issue to track swapping to using v1 as the storage version, or is this meant to track work (e.g. storage version migration) we'll do after that's completed?

I am not sure to be honest. It is something we need to figure out to be able to switch the storage version safely, so imho it is part of the work for changing the storage version.

@dibyom
Copy link
Member

dibyom commented Mar 28, 2023

We should probably use kubectl replace instead of kubectl apply in our release pages (or provide a better/cleaner way to handle upgrades)

+1

We should probably handle the storage conversion ourselves. In the documentation, there is an example of how to handle that (below) as well as a link to a k8s upstream tool that handles this : kubernetes-sigs/kube-storage-version-migrator. knative/pkg also have some "helper"/job around this : apiextensions/storageversion/cmd/migrate

I agree we should have a StorageVersionMigrator. Could be something like a post-install job like this: https://github.com/knative/serving/blob/main/config/post-install/storage-version-migration.yaml cc @lbernick @JeromeJu

@lbernick
Copy link
Member

Agreed on having a storage version migrator, but I think this can wait until after we swap the storage version, since it's not relevant until we want to remove the v1beta1 API as Vincent pointed out

@dibyom
Copy link
Member

dibyom commented Mar 29, 2023

it will have both v1alpha1 and v1beta1 as storedVersion — which is a case.. we don't handle pretty well. _This is gonna happen for v1 as well, when we switch the storedVersion.

@vdemeester looking at this again - how will this happen? there is only one stored version right? We'll flip [storage](https://github.com/tektoncd/pipeline/blob/main/config/300-pipelinerun.yaml#LL30C17-L30C17) to true for v1 and false for v1beta1. Both apply and replace should handle that right?

@vdemeester
Copy link
Member Author

Agreed on having a storage version migrator, but I think this can wait until after we swap the storage version, since it's not relevant until we want to remove the v1beta1 API as Vincent pointed out

I am not sure if I agree but I don't have too much "strong" argument to weigh my thought 😅.

@vdemeester looking at this again - how will this happen? there is only one stored version right? We'll flip [storage](https://github.com/tektoncd/pipeline/blob/main/config/300-pipelinerun.yaml#LL30C17-L30C17) to true for v1 and false for v1beta1. Both apply and replace should handle that right?

Nope, apply will most likely merge things, making storedVersion to list 2 versions, which is technically possible 😅, but that is shaky (aka I am not sure what the controller behavior is, as we are only watching one version, we probably are going to "make use" of the conversion webhook way more often than required). The documentation describes this:

Option 2: Manually upgrade the existing objects to a new stored version

The following is an example procedure to upgrade from v1beta1 to v1.

  1. Set v1 as the storage in the CustomResourceDefinition file and apply it
    using kubectl. The storedVersions is now v1beta1, v1.
  2. Write an upgrade procedure to list all existing objects and write them with
    the same content. This forces the backend to write objects in the current
    storage version, which is v1.
  3. Remove v1beta1 from the CustomResourceDefinition status.storedVersions field.

@JeromeJu
Copy link
Member

JeromeJu commented Mar 30, 2023

Thanks @vdemeester , agree with the options and the document provided and they are good instructions to include moving forward.



Whilst for the option2, I think the condition stated was when we are deprecating the old storage version (assuming for v1beta1 it would be a long time after? 🤔 ). 


For this issue, please feel free to correct since I might be mistaken, we are not blocked by it for the stored version swap but need to align on the timeline about when we deprecate v1beta1 and migrate the rest of the old storedversion after the v1 storage swap?

we probably are going to "make use" of the conversion webhook way more often than required)

As for the usage of conversion webhook, according to documentation, any updates on the object stored with the old storedversion will make it rewritten to the v1 stored version. It seems that using option2 or the migrator would utilize the conversion webhook in batch, while, on the other hand, leaving both storedVersions in etcd would not lead to the extra conversions instead the webhook gets called sporadically. This might suggest that the total # of times that the conversion webhook should be the same theoretically 💭 , but I do agree with the concerns here about the issues that might happen with 2 storedversions that requires manual testing.



In terms of user-facing instructions, I am happy to include this notification in the release note of the v1 storage swap and the migration-guide as well.

@pritidesai
Copy link
Member

Pipelines WG - can wait until we remove v1beta1, @lbernick will break this into multiple issue, clearing the milestone for now.

@pritidesai pritidesai removed this from the Pipelines v0.48 milestone May 16, 2023
@lbernick
Copy link
Member

@vdemeester I've split this issue into the following three issues:

@github-project-automation github-project-automation bot moved this from Todo to Done in Pipelines V1 May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue or PR that deals with the API. area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) kind/feature Categorizes issue or PR as related to a new feature.
Projects
Status: Done
Development

No branches or pull requests

5 participants