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
Add annotation updating for migration for PVs and PVCs #87098
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -309,6 +309,89 @@ func (ctrl *PersistentVolumeController) Run(stopCh <-chan struct{}) { | |
<-stopCh | ||
} | ||
|
||
func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) { | ||
// TODO: update[Claim|Volume]MigrationAnnotations can be optimized to not | ||
// copy the claim/volume if no modifications are required. Though this | ||
Comment on lines
+313
to
+314
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be a lightweight refactor. Have The function would copy the map if needed, and you only call DeepCopy if you need to modify annotations. I'm fine if you want to leave this as a TODO. |
||
// requires some refactoring as well as an interesting change in the | ||
// semantics of the function which may be undesirable. If no copy is made | ||
// when no modifications are required this function could sometimes return a | ||
// copy of the volume and sometimes return a ref to the original | ||
claimClone := claim.DeepCopy() | ||
modified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, pvutil.AnnStorageProvisioner) | ||
if !modified { | ||
return claimClone, nil | ||
} | ||
newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone) | ||
if err != nil { | ||
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err) | ||
} | ||
_, err = ctrl.storeClaimUpdate(newClaim) | ||
if err != nil { | ||
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err) | ||
} | ||
return newClaim, nil | ||
} | ||
|
||
func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotations(volume *v1.PersistentVolume) (*v1.PersistentVolume, error) { | ||
volumeClone := volume.DeepCopy() | ||
modified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, volumeClone.Annotations, pvutil.AnnDynamicallyProvisioned) | ||
if !modified { | ||
return volumeClone, nil | ||
} | ||
newVol, err := ctrl.kubeClient.CoreV1().PersistentVolumes().Update(volumeClone) | ||
if err != nil { | ||
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err) | ||
} | ||
_, err = ctrl.storeVolumeUpdate(newVol) | ||
if err != nil { | ||
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err) | ||
} | ||
return newVol, nil | ||
|
||
} | ||
|
||
// updateMigrationAnnotations takes an Annotations map and checks for a | ||
// provisioner name using the provisionerKey. It will then add a | ||
// "volume.beta.kubernetes.io/migrated-to" annotation if migration with the CSI | ||
// driver name for that provisioner is "on" based on feature flags, it will also | ||
// remove the annotation is migration is "off" for that provisioner in rollback | ||
// scenarios. Returns true if the annotations map was modified and false otherwise. | ||
func updateMigrationAnnotations(cmpm CSIMigratedPluginManager, translator CSINameTranslator, ann map[string]string, provisionerKey string) bool { | ||
var csiDriverName string | ||
var err error | ||
|
||
if ann == nil { | ||
// No annotations so we can't get the provisioner and don't know whether | ||
// this is migrated - no change | ||
return false | ||
} | ||
provisioner, ok := ann[provisionerKey] | ||
if !ok { | ||
// Volume not dynamically provisioned. Ignore | ||
return false | ||
} | ||
|
||
migratedToDriver := ann[pvutil.AnnMigratedTo] | ||
if cmpm.IsMigrationEnabledForPlugin(provisioner) { | ||
csiDriverName, err = translator.GetCSINameFromInTreeName(provisioner) | ||
if err != nil { | ||
klog.Errorf("Could not update volume migration annotations. Migration enabled for plugin %s but could not find corresponding driver name: %v", provisioner, err) | ||
return false | ||
} | ||
if migratedToDriver != csiDriverName { | ||
ann[pvutil.AnnMigratedTo] = csiDriverName | ||
return true | ||
} | ||
} else { | ||
if migratedToDriver != "" { | ||
// Migration annotation exists but the driver isn't migrated currently | ||
delete(ann, pvutil.AnnMigratedTo) | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
// volumeWorker processes items from volumeQueue. It must run only once, | ||
// syncVolume is not assured to be reentrant. | ||
func (ctrl *PersistentVolumeController) volumeWorker() { | ||
|
@@ -461,6 +544,7 @@ func (ctrl *PersistentVolumeController) setClaimProvisioner(claim *v1.Persistent | |
// modify these, therefore create a copy. | ||
claimClone := claim.DeepCopy() | ||
metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, pvutil.AnnStorageProvisioner, provisionerName) | ||
updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, pvutil.AnnStorageProvisioner) | ||
newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.Namespace).Update(claimClone) | ||
if err != nil { | ||
return newClaim, err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,23 +18,29 @@ package persistentvolume | |
|
||
import ( | ||
"errors" | ||
"reflect" | ||
"testing" | ||
"time" | ||
|
||
v1 "k8s.io/api/core/v1" | ||
storagev1 "k8s.io/api/storage/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/watch" | ||
utilfeature "k8s.io/apiserver/pkg/util/feature" | ||
"k8s.io/client-go/informers" | ||
"k8s.io/client-go/kubernetes/fake" | ||
storagelisters "k8s.io/client-go/listers/storage/v1" | ||
core "k8s.io/client-go/testing" | ||
"k8s.io/client-go/tools/cache" | ||
"k8s.io/component-base/featuregate" | ||
featuregatetesting "k8s.io/component-base/featuregate/testing" | ||
csitrans "k8s.io/csi-translation-lib" | ||
"k8s.io/klog" | ||
"k8s.io/kubernetes/pkg/controller" | ||
pvtesting "k8s.io/kubernetes/pkg/controller/volume/persistentvolume/testing" | ||
pvutil "k8s.io/kubernetes/pkg/controller/volume/persistentvolume/util" | ||
"k8s.io/kubernetes/pkg/features" | ||
"k8s.io/kubernetes/pkg/volume/csimigration" | ||
) | ||
|
||
var ( | ||
|
@@ -461,3 +467,110 @@ func TestDelayBindingMode(t *testing.T) { | |
} | ||
} | ||
} | ||
|
||
func TestAnnealMigrationAnnotations(t *testing.T) { | ||
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigration, true)() | ||
|
||
const testPlugin = "non-migrated-plugin" | ||
const gcePlugin = "kubernetes.io/gce-pd" | ||
const gceDriver = "pd.csi.storage.gke.io" | ||
tests := []struct { | ||
name string | ||
volumeAnnotations map[string]string | ||
expVolumeAnnotations map[string]string | ||
claimAnnotations map[string]string | ||
expClaimAnnotations map[string]string | ||
migratedDriverGates []featuregate.Feature | ||
}{ | ||
{ | ||
name: "migration on for GCE", | ||
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin}, | ||
expVolumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, | ||
claimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: gcePlugin}, | ||
expClaimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, | ||
migratedDriverGates: []featuregate.Feature{features.CSIMigrationGCE}, | ||
}, | ||
{ | ||
name: "migration off for GCE", | ||
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin}, | ||
expVolumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin}, | ||
claimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: gcePlugin}, | ||
expClaimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: gcePlugin}, | ||
migratedDriverGates: []featuregate.Feature{}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this test fail when GCE migration is on by default, and should we dump that problem on future us? I vote yes. |
||
}, | ||
{ | ||
name: "migration off for GCE removes migrated to (rollback)", | ||
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, | ||
expVolumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin}, | ||
claimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: gcePlugin, pvutil.AnnMigratedTo: gceDriver}, | ||
expClaimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: gcePlugin}, | ||
migratedDriverGates: []featuregate.Feature{}, | ||
}, | ||
{ | ||
name: "migration on for GCE other plugin not affected", | ||
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: testPlugin}, | ||
expVolumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: testPlugin}, | ||
claimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: testPlugin}, | ||
expClaimAnnotations: map[string]string{pvutil.AnnStorageProvisioner: testPlugin}, | ||
migratedDriverGates: []featuregate.Feature{features.CSIMigrationGCE}, | ||
}, | ||
{ | ||
name: "not dynamically provisioned migration off for GCE", | ||
volumeAnnotations: map[string]string{}, | ||
expVolumeAnnotations: map[string]string{}, | ||
claimAnnotations: map[string]string{}, | ||
expClaimAnnotations: map[string]string{}, | ||
migratedDriverGates: []featuregate.Feature{}, | ||
}, | ||
{ | ||
name: "not dynamically provisioned migration on for GCE", | ||
volumeAnnotations: map[string]string{}, | ||
expVolumeAnnotations: map[string]string{}, | ||
claimAnnotations: map[string]string{}, | ||
expClaimAnnotations: map[string]string{}, | ||
migratedDriverGates: []featuregate.Feature{features.CSIMigrationGCE}, | ||
}, | ||
{ | ||
name: "nil annotations migration off for GCE", | ||
volumeAnnotations: nil, | ||
expVolumeAnnotations: nil, | ||
claimAnnotations: nil, | ||
expClaimAnnotations: nil, | ||
migratedDriverGates: []featuregate.Feature{}, | ||
}, | ||
{ | ||
name: "nil annotations migration on for GCE", | ||
volumeAnnotations: nil, | ||
expVolumeAnnotations: nil, | ||
claimAnnotations: nil, | ||
expClaimAnnotations: nil, | ||
migratedDriverGates: []featuregate.Feature{features.CSIMigrationGCE}, | ||
}, | ||
} | ||
|
||
translator := csitrans.New() | ||
cmpm := csimigration.NewPluginManager(translator) | ||
|
||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
for _, f := range tc.migratedDriverGates { | ||
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, f, true)() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hooray for global variables! |
||
} | ||
if tc.volumeAnnotations != nil { | ||
ann := tc.volumeAnnotations | ||
updateMigrationAnnotations(cmpm, translator, ann, pvutil.AnnDynamicallyProvisioned) | ||
if !reflect.DeepEqual(tc.expVolumeAnnotations, ann) { | ||
t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations) | ||
} | ||
} | ||
if tc.claimAnnotations != nil { | ||
ann := tc.claimAnnotations | ||
updateMigrationAnnotations(cmpm, translator, ann, pvutil.AnnStorageProvisioner) | ||
if !reflect.DeepEqual(tc.expClaimAnnotations, ann) { | ||
t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations) | ||
} | ||
} | ||
|
||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does mean that worst case scenario puts us in a tight loop hitting this condition and never updates PV or PVC objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same behavior as any other PV/PVC update failure - see call sites of things like
updateClaimStatus
,bind
,updateVolumePhase
, etc. This update is of similar importance going forward - without an annotation there is no migration - if migration is the default there is no storage without the annotation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍