Handle status.storedVersions migration during upgrades

[furkatgofurov7]

User Story

As a developer/user/operator, I would like to do an upgrade from old API versions to newer ones in sequence (v1a3 => v1a4 => v1beta1 ) without manually removing stored versions and applying them in the middle of the upgrade process (v1a4 => v1beta1 since v1a4 would have v1a3 storedVersion in the status).

Detailed Description

we have a scenario where we do an upgrade from CAPM3 v1a4 => v1a5 => v1beta1 (CAPI v1a3 => v1a4 => v1beta1). The problem is, clusterctl fails (see logs below) to upgrade from v1a5 => v1b1 because of the status.storedVersions in the CRD which includes v1a4 API version in the status.

Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"
Retrying with backoff Cause="failed to patch provider object: CustomResourceDefinition.apiextensions.k8s.io \"metal3clusters.infrastructure.cluster.x-k8s.io\" is invalid: status.storedVersions[0]: Invalid value: \"v1alpha4\": must appear in spec.versions"
Patching CustomResourceDefinition="metal3clusters.infrastructure.cluster.x-k8s.io"

The request would be, should the clusterctl take care of removing of old version from the status.storedVersions or how is it handled in general?

Anything else you would like to add:

There has been a long discussion in the slack thread, xref: https://kubernetes.slack.com/archives/C8TSNPY4T/p1655805341632159

/kind feature

[furkatgofurov7]

Based on the slack conversation we had, this is a small summary of all points we had:

• there is an instruction on upstream k8s docs how to handle this:
  https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version and whole project to ensure storage version for all CRDs:
  https://github.com/kubernetes-sigs/kube-storage-version-migrator

Seems, the way we could tackle this as of now is:

1. Run the storage Version migrator

2. Remove the old version from the CustomResourceDefinition status.storedVersions field.

based on the https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#previous-storage-versions

• Also, it was noted that this is a generic Kubernetes problem with migrations. API server does not migrate objects in bulk for reasons, but everything will be migrated over time. But if we upgrade again in a fast sequence before that happen we would have problems. In most use cases maybe just going manually over the CRDs, checking for old storage versions and then reapplying with the current version might be safer. However, to have a better user experience and let the clusterctl handle it, we could consider improving some parts of clusterctl (e.g. drop old unused versions from the CRDs before we update them)

• There were/are multiple upstream KEPs,
  KEP-3166: Migrating and deprecating storage versions kubernetes/enhancements#3185
  should be the latest one (prior ones, i.e https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2339-storageversion-api-for-ha-api-servers was taken up in PRs https://github.com/kubernetes/kubernetes/pull/96403 & https://github.com/kubernetes-sigs/kube-storage-version-migrator/pull/85 but those PRs stopped receiving updates and were marked as stale in mid 2021)

/cc @sbueringer @fabriziopandini @lentzi90 @chrischdi

[schrej]

but everything will be migrated over time

This is not necessarily true. The apiserver only upgrades the storage version on write. So if an object never gets written, it won't get upgraded. So ideally we would automatically write the object during reconcile if the storage version is outdated, even if nothing changed. If I'm not mistaken, controllers will reconcile all of their objects after start, so that would automatically convert everything.

I think we discussed this at KubeCon during the CAPI meeting. One way to get around this would be to have some annotation/label on objects whose storage version is outdated. The idea was to talk to sig-apimachinery and suggest adding such an annotation. But I think we don't even need their support: conversion happens through conversion webhooks, which are implemented by operators. It should be possible to add an annotation during migration, when the target version is not the storage version. This could potentially be added on controller-runtime level, solving this problem for lots of operators at once.

Operators probably always reconcile the latest apiversion of an object, right? So if the controller gets an object, no conversion should occur. If conversion webhooks add a label during conversion, the controller would notice that the object was piped through conversion, and therefore isn't using the latest apiversion for storage.

[sbueringer]

Hey folks,
we've implemented a CRD migration in clusterctl upgrade in #6749 as we have to do it for cert-manager anyway.

I think it's a clean and straightforward solution for our issue and won't require a multi-month/year effort to get something implemented upstream in Kubernetes.

I looked over the KEPs and it seems like this will take a while: (and then is probably not available in old versions of Kubernetes)

• KEPs 2330, 2339, 2341, 2342, 2343
• Replacement Draft
  • Migrating and deprecating storage versions kubernetes/enhancements#3166
  • KEP-3166: Migrating and deprecating storage versions kubernetes/enhancements#3185

I would highly prefer a straightforward solution in clusterctl over modifying webhook and controllers in all providers to write all CRs on upgrades (this would also not work for cert-manager).

If there will be an upstream solution in either controller-runtime or Kubernetes over time which works with all controller-runtime/Kubernetes version that we need it for we can of course deprecate our mechanism as it's then not necessary anymore.