WINC-1043: Test upgrading to CSI storage, from in-tree drivers #1705
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
approved
sebsoto
force-pushed
the
upgradeBlock
branch
2 times, most recently
from
9d623e6
to
e9b35fb
Compare
|
/approve cancel |
|
/test vsphere-e2e-upgrade |
openshift-ci
bot
removed
the
approved
|
/test vsphere-e2e-upgrade |
1 similar comment
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
3 similar comments
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
2 similar comments
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
3 similar comments
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
test/e2e/storage_test.go
Outdated
| defer func() { | ||
| err := tc.client.K8s.CoreV1().PersistentVolumeClaims(tc.workloadNamespace).Delete(context.TODO(), | ||
| pvc.GetName(), meta.DeleteOptions{}) | ||
| log.Printf("error deleting PVC: %s", err) |
There was a problem hiding this comment.
should only print this if err != nil
test/e2e/storage_test.go
Outdated
| if err == nil && !skipWorkloadDeletion { | ||
| defer func() { | ||
| err := tc.deleteDeployment(winServerDeployment.GetName()) | ||
| log.Printf("error deleting deployment: %s", err) |
| vsphereProvider, ok := tc.CloudProvider.(*vsphere.Provider) | ||
| require.True(t, ok, "in tree upgrade must be ran on vSphere") |
There was a problem hiding this comment.
Is the cast necessary? We know the cloud provider is vSphere due to the check added on line 24 so doing tc.CloudProvider.CreateInTreePVC() directly should be safe
There was a problem hiding this comment.
The cast is necessary in order to use the method, which is specific to the vSphere provider.
| vsphereProvider, ok := tc.CloudProvider.(*vsphere.Provider) | ||
| require.True(t, ok, "in tree upgrade must be ran on vSphere") |
There was a problem hiding this comment.
nit: I'd prefer checking if cloud provider type is vSphere instead of doing it through a cast
There was a problem hiding this comment.
same as above:
The cast is necessary in order to use the method, which is specific to the vSphere provider.
There was a problem hiding this comment.
Fair enough, these comments were only applicable assuming we were to add the methods to the CloudProvider interface
| @@ -151,14 +151,23 @@ func (p *Provider) StorageSupport() bool { | |||
| return true | |||
| } | |||
|
|
|||
| // CreateInTreePVC creates a PVC which has the potential to be migrated to a CSI PVC on upgrade | |||
| func (p *Provider) CreateInTreePVC(client client.Interface, namespace string) (*core.PersistentVolumeClaim, error) { | |||
There was a problem hiding this comment.
IMO any exported method on Provider should be added to the CloudProvider interface and implemented by other providers (even if the implementation is just returning a "sorry, function not supported" error)
There was a problem hiding this comment.
I would usually agree, but in this case I chose not to make this change as this needs to be reverted after this release, and changes to the interface would make that more complicated.
|
/test vsphere-e2e-upgrade Retesting to see whats going on here, failure might be a flake. |
sebsoto
force-pushed
the
upgradeBlock
branch
from
b9702e2
to
98e17fb
Compare
|
/test vsphere-e2e-upgrade |
|
#1733 has merged. |
openshift-ci
bot
removed
the
do-not-merge/hold
Increases timeout to allow extra time for storage workloads.
Runs a storage workload that is not deleted at the end of the storage test, in order to test that the workload persists across the upgrade. Also collects node logs for better debugging.
Deployed pods with a storage mount take a bit longer to delete. Remove all pods ahead of the deletion test to make timing more consistent and stop flaking due to this.
Adds test functionality to deploy in-tree PVCs when upgrading from a WMCO which does not support CSI. This is triggered when the environment variable `UPGRADE_FROM_IN_TREE` is set to true. This commit should be reverted in the OCP 4.15 timeline, when there is no longer the possibility of this scenario.
sebsoto
force-pushed
the
upgradeBlock
branch
from
98e17fb
to
378aa32
Compare
|
/test vsphere-e2e-upgrade |
4 similar comments
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
|
/test vsphere-e2e-upgrade |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@sebsoto: all tests passed! 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. |
Adds various changes required to test upgrading a Windows node from a version of WMCO which supports in-tree storage drivers, to a version that does not.
The changes within
Enable upgrading from in-tree to CSIshould be reverted when branching for OCP 4.15 occurs, as they will break the tests. Those changes have been labeled with a TODO, and have been purposely been made easily removable.