Check if PV/PVC Access Modes are Supported by the Volume Provider #139

rrpolanco · 2022-11-21T17:53:08Z

Currently, when migrating data, we do not consider whether the source PV's access modes are supported by the destination's volume provider. Not having this check makes the migrator prone to errors during the migration which is not the optimal experience. E.g. if migration from a volume that supports access modes RWX(Minio) to a volume which only supports RWO (local volume). For cases such as this, we want to warn the users about such errors prior to the migration so they can fix them ahead of time and have a smoother migration experience.

Solution

pvmigrate client and library will support running volume access modes validation checks on the destination PV. The checks will verify if the access modes on any of the source PVCs are supported by the destination volume provider. This is accomplished by creating temporary headless pods which create and consume the destination storage class volume with the access modes of the source PVCs. Any failures will be shown to the user via the terminal.

How it Works

At a high level, the validation does the following:

Retrieve all PVs for the source storage class
For each of the PVs gathered, get their associated PVC reference
For each source PVC:
1. Create a temporary PVC using source PVC volume access modes
2. Create a temporary POD to consume the temp PVC
3. Wait for temp POD to be ready, if not ready then it's highly likely there was an error creating the volume
  1. Get the volume provisioning error by inspecting the events for the PVC
Collect all the PVC errors and print them in a table format

NAMESPACE               PVC                             SOURCE                        REASON                           MESSAGE
---------               ---                             -----------                   --------------                   ----------
default                 rook-rwx-block-claim            openebs.io/local_openebs-localpv-provisioner-77c9bcfd96-v4dnj_311601fb-2832-4f3d-b464-e78d3895b046        ProvisioningFailed               failed to provision volume with StorageClass "openebs-localpv": Only support ReadWriteOnce access mode

Ref: Internal ticket tracker [sc-61144]

rrpolanco · 2022-11-22T15:37:44Z

[sc-61144]

camilamacedo86 · 2022-11-22T16:00:40Z

pkg/migrate/migrate_test.go

+			resources: []runtime.Object{
+				&storagev1.StorageClass{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "srcSc",


Could we try to add names here that clarifies WHEN.. THEN .. EXPECTED?
more info: https://dev.to/smyrman/test-with-expect-a-bdd-style-go-naming-pattern-5eh5

Example: when has x should not fail

Good suggestion. I'll give the TWE naming convention a try.

camilamacedo86 · 2022-11-22T16:01:58Z

pkg/migrate/migrate.go

+
+// checkVolumeAccessModeValid checks if the access modes of a pv are supported by the
+// destination storage class.
+func (p *PVMigrator) checkVolumeAccessModes(pvc corev1.PersistentVolumeClaim) (PVCError, error) {


Could we not here return error only?
That is little strange return PVCError and error. is not PVCError an error as well?

I could return (error, error) however, I would have to deal with the type conversation later on. I rather use the concrete type directly, PVCErrror, which represents the error from the PVC event.

Why not just return error, and have the cause of the PVCError be the parent error if one exists?

camilamacedo86 · 2022-11-22T16:10:08Z

pkg/migrate/migrate.go

+		case <-waitInterval.C:
+			continue
+		case <-migrationTimeout.C:
+			return fmt.Errorf("migration pod %s failed to go into Running phase: timedout", createdPod.Name)


have we a CR that represents the service? if so, could we use use conditional status?
So that, we can properly set the status of this one?

We don't have a CR. This project is both a CLI and library for doing PVC migrations.
What do you mean by

if so, could we use use conditional status?
So that, we can properly set the status of this one?

camilamacedo86 · 2022-11-22T17:05:50Z

pkg/migrate/migrate.go

@@ -61,6 +85,9 @@ func Cli() {
 	flag.BoolVar(&options.SetDefaults, "set-defaults", false, "change default storage class from source to dest")
 	flag.BoolVar(&options.VerboseCopy, "verbose-copy", false, "show output from the rsync command used to copy data between PVCs")
 	flag.BoolVar(&options.SkipSourceValidation, "skip-source-validation", false, "migrate from PVCs using a particular StorageClass name, even if that StorageClass does not exist")
+	flag.BoolVar(&options.SkipPVAccessModeValidation, "skip-pv-access-mode-validation", false, "skip the volume access modes validation on the destination storage provider")
+	flag.IntVar(&options.PvcCopyTimeout, "timeout", 300, "length of time to wait (in seconds) when transferring data from the source to the destination storage volume")
+	flag.IntVar(&options.PodReadyTimeout, "pod-ready-timeout", 60, "length of time to wait (in seconds) for volume validation pod(s) to go into Ready phase")


Question: how we have been using this project?
Do we distribute it like a bin and call in the kURL?
Where and how it is used?

It's used as a library in kURL's version of pvmigrate CLI: https://github.com/replicatedhq/kURL/blob/main/kurl_util/cmd/pvmigrate/main.go#L14

It's also distributed as a binary publicly, and is installable with krew

emosbaugh · 2022-11-22T17:48:19Z

go.mod

@@ -3,16 +3,20 @@ module github.com/replicatedhq/pvmigrate
 go 1.19

 require (
+	github.com/google/uuid v1.3.0
+	github.com/replicatedhq/kurl v0.0.0-20221116172021-8ac37b7b9867


this seems like it begs to be a circular dep. can we move the method into this project?

I'm specifically importing github.com/replicatedhq/kurl/pkg/k8sutil. However, nothing in k8sutil package imports this (migrate) package, so no import cycle there. I'd rather no duplicate code here if not absolutely required.

then lets move the code to pvmigrate. this project should absolutely not depend on kurl

@emosbaugh To sever the kURL dependency I can copy those functions into this project or refactor so as to not depend on those functions per your original suggestion. Agree?

Strongly agree
We should NOT be importing kurl here

emosbaugh · 2022-11-22T17:48:32Z

go.mod

+	k8s.io/api v0.25.4
+	k8s.io/apimachinery v0.25.4
+	k8s.io/client-go v0.25.4
+	k8s.io/kubernetes v1.25.3


emosbaugh · 2022-11-22T17:51:11Z

pkg/migrate/migrate.go

+	scaleAnnotation          = baseAnnotation + "-scale"
+	kindAnnotation           = baseAnnotation + "-kind"
+	sourceNsAnnotation       = baseAnnotation + "-sourcens"
+	sourcePvcAnnotation      = baseAnnotation + "-sourcepvc"


for consistency

Suggested change

sourcePvcAnnotation = baseAnnotation + "-sourcepvc"

sourcePVCAnnotation = baseAnnotation + "-sourcepvc"

emosbaugh · 2022-11-22T17:51:31Z

pkg/migrate/migrate.go

+	VerboseCopy                bool
+	SkipSourceValidation       bool
+	SkipPVAccessModeValidation bool
+	PvcCopyTimeout             int


Suggested change

PvcCopyTimeout int

PVCCopyTimeout int

emosbaugh · 2022-11-22T17:53:29Z

pkg/migrate/migrate.go

+		}
+	}
+
+	// start the migration


do you see any value in adding a --dry-run flag?

yes! Namely for standalone CLI users.

That's been requested before, actually: #104

Dry run of course could not create pods

emosbaugh · 2022-11-22T18:22:12Z

pkg/migrate/migrate.go

+	var waitFor []string
+	propagation := metav1.DeletePropagationForeground
+	delopts := metav1.DeleteOptions{PropagationPolicy: &propagation}
+	if err := p.k8scli.CoreV1().PersistentVolumeClaims("default").Delete(


should this be in the same namespace as the src pvc?

Temp pod is created in default ns. See #139 (comment)

emosbaugh · 2022-11-22T18:22:56Z

pkg/migrate/migrate.go

+			Containers: []corev1.Container{
+				{
+					Name:  "busybox",
+					Image: "busybox",


Will this work for airgap customers?

Good point! This image would need to be loaded into Docker or containerd unless it's been loaded already by another image which builds atop of busybox.

Hmm, @ricardomaraschini node disk space validator would also run into the same issue if it was integrated in the kURL installer. The disk space validator relies on the rsync image to be available. It's probably a good idea for both to use the same container image. I need to think about how to best handle this. I'm currently thinking we should pick an image to use, archive it and then load it during install.

We already require an image to be passed via the CLI to do the migration with
Use that image

pvmigrate/pkg/migrate/migrate.go

Line 83 in 075f71c

flag.StringVar(&options.RsyncImage, "rsync-image", "eeacms/rsync:2.3", "the image to use to copy PVCs - must have 'rsync' on the path")

Resolved: We can use the kurl util image and specify it as part of the CLI as show here: https://github.com/replicatedhq/kURL/blob/aea79861716d66787f0b31670f1fc74a7ee16d1f/scripts/common/rook.sh#L153

emosbaugh · 2022-11-22T18:27:35Z

pkg/migrate/migrate.go

+	propagation := metav1.DeletePropagationForeground
+	delopts := metav1.DeleteOptions{PropagationPolicy: &propagation}
+	if pod != nil {
+		if err := p.k8scli.CoreV1().Pods(pod.Namespace).Delete(context.Background(), pod.Name, delopts); err != nil {


Suggested change

if err := p.k8scli.CoreV1().Pods(pod.Namespace).Delete(context.Background(), pod.Name, delopts); err != nil {

// Cleanup should use background context so as not to fail if context has already been canceled

if err := p.k8scli.CoreV1().Pods(pod.Namespace).Delete(context.Background(), pod.Name, delopts); err != nil {

emosbaugh · 2022-11-22T18:28:26Z

pkg/migrate/migrate.go

+}
+
+// getPvcError returns the error event for why a PVC is in Pending status
+func (p *PVMigrator) getPvcError(pvc *corev1.PersistentVolumeClaim) (PVCError, error) {


Suggested change

func (p *PVMigrator) getPvcError(pvc *corev1.PersistentVolumeClaim) (PVCError, error) {

func (p *PVMigrator) getPVCError(pvc *corev1.PersistentVolumeClaim) (PVCError, error) {

emosbaugh · 2022-11-22T18:29:06Z

pkg/migrate/migrate.go

+	}
+	waitFor = append(waitFor, pvc.Name)
+
+	timeout := time.NewTicker(5 * time.Minute)


Suggested change

timeout := time.NewTicker(5 * time.Minute)

timeout := time.After(5 * time.Minute)

laverya · 2022-11-29T15:04:00Z

pkg/migrate/migrate.go

@@ -79,6 +106,37 @@ func Cli() {

 	output := log.New(os.Stdout, "", 0) // this has no time prefix etc

+	// escape hatch - for DEV/TEST ONLY


if this is for dev/test only, that should be mentioned in the flag description - or the flag should be hidden.

I'll update the comment. I added this in the case where there is a rare scenario where someone may a justifiable reason to ignore a PVC access volume validation.

laverya · 2022-11-29T15:05:34Z

pkg/migrate/migrate.go

+	if !options.SkipPVAccessModeValidation {
+		srcPVs, err := kurlutils.PVSByStorageClass(context.TODO(), clientset, options.SourceSCName)
+		if err != nil {
+			fmt.Printf("failed to get volumes using storage class %s: %s", options.SourceSCName, err)


Suggested change

fmt.Printf("failed to get volumes using storage class %s: %s", options.SourceSCName, err)

output.Printf("failed to get volumes using storage class %s: %s", options.SourceSCName, err)

ethan commented the same elsewhere, but let's avoid fmt.printf when we have output available

laverya · 2022-11-29T15:10:11Z

pkg/migrate/migrate.go

@@ -134,6 +192,82 @@ func Migrate(ctx context.Context, w *log.Logger, clientset k8sclient.Interface,
 	return nil
 }

+// NewPVMigrator returns a PV migration context
+func NewPVMigrator(cfg *rest.Config, log *log.Logger, srcSC, dstSC string, podReadyTimeout int) (*PVMigrator, error) {


What is this used by?
What is the use case?

Aren't you initializing the struct directly elsewhere?

IMO this whole function is unused and should not exist

I think there should be a single function to run the whole 'can I migrate from sc A to B without having broken access modes' check, but it should take all the args it needs directly.

NewPVMigrator() is used by kURL's pvmigrator CLI: https://github.com/replicatedhq/kURL/pull/3767/files#diff-fc4b9380946b46363bac9e7ecee925d5142b5fa0f607de99b9f5d3383d952e01R97

Truthfully what I'm getting from that is 'oh dear lord why did we start reimplementing this logic in kURL, that was the entire reason that the CLI function exists'

I still think the solution is to move the entire 'can I make proper dest PVCs' logic inside of 'Migrate'

laverya · 2022-11-29T15:19:20Z

pkg/migrate/migrate.go

@@ -79,6 +106,37 @@ func Cli() {

 	output := log.New(os.Stdout, "", 0) // this has no time prefix etc

+	// escape hatch - for DEV/TEST ONLY
+	if !options.SkipPVAccessModeValidation {
+		srcPVs, err := kurlutils.PVSByStorageClass(context.TODO(), clientset, options.SourceSCName)


I'm relatively certain we have this logic in pvmigrate already - refactoring it into a function would be better than using kurlutils (even besides the 'let's not do circular deps' issue, having the same logic use two different implementations will cause issues)

Yeah, we do. I can make use of that instead of relying the kurl k8s package.

laverya · 2022-11-29T15:23:59Z

pkg/migrate/migrate.go

+	PvcCopyTimeout             int
+	PodReadyTimeout            int


Suggested change

PvcCopyTimeout int

PodReadyTimeout int

PvcCopyTimeout time.Duration

PodReadyTimeout time.Duration

Why are we passing around unitless integers when there is literally a type for that

My thought on this was to make easy on clients...just give me a time in seconds (int) and the implementation will convert it to the type(time.Duration) it needs.

laverya · 2022-11-29T15:24:28Z

pkg/migrate/migrate.go

@@ -103,7 +161,7 @@ func Migrate(ctx context.Context, w *log.Logger, clientset k8sclient.Interface,
 		return fmt.Errorf("failed to scale down pods: %w", err)
 	}

-	err = copyAllPVCs(ctx, w, clientset, options.SourceSCName, options.DestSCName, options.RsyncImage, updatedMatchingPVCs, options.VerboseCopy, time.Second)
+	err = copyAllPVCs(ctx, w, clientset, options.SourceSCName, options.DestSCName, options.RsyncImage, updatedMatchingPVCs, options.VerboseCopy, time.Duration(options.PvcCopyTimeout)*time.Second)


just.... no
I've requested a change to make the option actually a duration

Will accept that change.

laverya · 2022-11-29T15:27:49Z

pkg/migrate/migrate.go

+// ValidateVolumeAccessModes checks whether the provided persistent volumes support the access modes
+// of the destination storage class.
+// returns a map of pvc errors indexed by namespace
+func (p *PVMigrator) ValidateVolumeAccessModes(pvs map[string]corev1.PersistentVolume) (map[string]map[string]PVCError, error) {


Why is this function public?

If we're going to keep it public let's move it (and all supporting infa) to a new package.

The one benefit I see for keeping this public and separating the validation from the migration behaviour is that in the near future we could rollup all the different validation checks into one package or method.

Sure, but even if we do that we probably don't want to encourage calling one individual validator.

Better to keep this private until we 100% have a need for it externally

laverya · 2022-11-29T15:31:20Z

pkg/migrate/migrate.go

+			timeoutCtx, cancelCtx := context.WithTimeout(ctx, timeout)
+			defer cancelCtx()


IIRC this doesn't work great in a for loop.

Also, just more generally, this isn't an OK change IMO - the current logic is 'keep copying as long as it's not stuck', and since some copies may legitimately take hours I would rather not have to wait hours to find out that something was really stuck

Also, you're doubling up on timeouts here - you've added timeout logic to copyOnePVC but also to the context it's being passed. Choose one (but preferably choose none in this PR), if we need to do that it needs more discussion

This needs to be fleshed out bit a more. Namely, figure out what constitutes as "stuck" and how to detect that.

Created internal tracking ticket for handling timeouts in this context.

laverya · 2022-11-29T15:36:11Z

pkg/migrate/migrate.go

@@ -229,7 +376,7 @@ func copyAllPVCs(ctx context.Context, w *log.Logger, clientset k8sclient.Interfa
 	return nil
 }

-func copyOnePVC(ctx context.Context, w *log.Logger, clientset k8sclient.Interface, ns string, sourcePvcName string, destPvcName string, rsyncImage string, verboseCopy bool, waitTime time.Duration, nodeName string) error {
+func copyOnePVC(ctx context.Context, w *log.Logger, clientset k8sclient.Interface, ns string, sourcePvcName string, destPvcName string, rsyncImage string, verboseCopy bool, nodeName string, timeout time.Duration) error {


Please back out all of the changes for this function. They do not appear related to the main thrust of the PR and require more discussion (as they prevent legitimately slow/large transfers from working, or prevent stalled transfers from being detected early, depending on configuration)

I'll back this out.

laverya · 2022-11-29T15:39:22Z

pkg/migrate/migrate.go

@@ -1146,3 +1296,249 @@ func resetReclaimPolicy(ctx context.Context, w *log.Logger, clientset k8sclient.

 	return nil
 }
+
+// buildPVConsumerPod creates a pod spec for consuming a pvc
+func (p *PVMigrator) buildPVConsumerPod(pvcName string) *corev1.Pod {


can we pass in options instead of using an object?
It's not the patten the project has used thus far.

@laverya you want buildPVConsumerPod() to accept the name override as an argument instead of pulling it from the receiver?

laverya · 2022-11-29T15:40:01Z

pkg/migrate/migrate.go

+	if len(podName) > 63 {
+		podName = podName[0:31] + podName[len(podName)-32:]
+	}


I swear this logic is present elsewhere in the code
could you factor them into a common function?

laverya · 2022-11-29T15:46:24Z

pkg/migrate/migrate.go

+	if len(pvcName) > 63 {
+		pvcName = pvcName[0:31] + pvcName[len(pvcName)-32:]
+	}


another instance of this logic to factor out

Will address this.

laverya · 2022-11-29T15:52:57Z

pkg/migrate/migrate.go

+	defer func() {
+		if err = p.deleteTmpPVC(tmpPVC); err != nil {
+			p.log.Printf("failed to delete tmp claim: %s", err)
+		}
+	}()
+	defer func() {
+		if err = p.deletePVConsumerPod(pvConsumerPod); err != nil {
+			p.log.Printf("failed to delete pv consumer pod %s: %s", pvConsumerPod.Name, err)
+		}
+	}()


IMO these should be the same defer func, not two

I can consolidate these.

laverya · 2022-11-29T16:04:24Z

pkg/migrate/migrate.go

+	if !options.SkipPVAccessModeValidation {
+		srcPVs, err := kurlutils.PVSByStorageClass(context.TODO(), clientset, options.SourceSCName)
+		if err != nil {
+			fmt.Printf("failed to get volumes using storage class %s: %s", options.SourceSCName, err)
+			os.Exit(1)
+		}
+
+		pvMigrator := PVMigrator{
+			ctx:             context.TODO(),
+			log:             output,
+			k8scli:          clientset,
+			srcSc:           options.SourceSCName,
+			dstSc:           options.DestSCName,
+			deletePVTimeout: 5 * time.Minute,
+			podReadyTimeout: time.Duration(options.PodReadyTimeout) * time.Second,
+		}
+		unsupportedPVCs, err := pvMigrator.ValidateVolumeAccessModes(srcPVs)
+		if err != nil {
+			fmt.Printf("failed to validate volume access modes for destination storage class %s", options.DestSCName)
+			os.Exit(1)
+		}


I would greatly prefer for this logic to be contained within 'Migrate' (after the 'do these storageclasses exist' check, for instance), not independent

If it must be independent, it should be callable as a single function that returns an error and should handle getting the list of PVCs itself.

laverya · 2022-11-29T16:13:41Z

pkg/migrate/migrate.go

+
+// buildPVC creates a temporary PVC requesting for 1Mi of storage for a provided storage class name.
+func (p *PVMigrator) buildTmpPVC(pvc corev1.PersistentVolumeClaim, sc string) *corev1.PersistentVolumeClaim {
+	pvcName := p.pvcNameOverride


instead of having pvcNameOverride, I'd generally recommend using the source PVC's name with a suffix and then trimmed

(And name the pod based on the PVC being tested too)

this makes things much clearer to any cluster operator that encounters this in progress

The name override is meant for testing since we need a deterministic name.

The name should ALWAYS be deterministic, never random

The way to do that is to take something like my-app-pvc-0 and turn it into my-app-pvc-0-pvcmigrate-accessmodes

This is what is done elsewhere in the code (and in fact that's why we need the 'trim from middle' code)

Will make this change.

laverya · 2022-11-29T16:15:17Z

pkg/migrate/migrate.go

+		// for testing/mocking
+		// This fields gets set and updated by the kube api server
+		Status: corev1.PersistentVolumeClaimStatus{Phase: pvc.Status.Phase},


I'm confused - why are you setting status here?

If you're doing this so that you can set the claim status for something else to read in a test function, I'd recommend looking at the backgroundfunc stuff in some of the other test functions - that way you can wait for 1/10th of a second or whatever and ensure that the logic does not require status to be immediately perfect

emosbaugh · 2022-12-02T16:40:30Z

cmd/main.go

+
+		if len(failures) != 0 {
+			preflight.PrintValidationFailures(output.Writer(), failures)
+			os.Exit(2)


is there any possibility for the purposes of kurl we run validate and then migrate separately? then we dont have to have this alternate exit code. if we cant, can we try to map this exit code to standard linux exit codes? also should we use a constant here?

The intention was to only run validate() as part of the openebs migration prompt here and abort the install if the validate failed. Also, AFAIK there are no standard/conventional exit codes. We could model this after a linux command or come up with our own scheme. I lean towards the latter.

emosbaugh · 2022-12-02T16:41:50Z

cmd/main.go

+		err = migrate.Migrate(context.TODO(), output, clientset, options)
+		if err != nil {
+			output.Printf("migration failed: %s", err)
+			os.Exit(3)


this should be 1

emosbaugh · 2022-12-02T16:42:45Z

cmd/main.go

+	flag.BoolVar(&options.SetDefaults, "set-defaults", false, "change default storage class from source to dest")
+	flag.BoolVar(&options.VerboseCopy, "verbose-copy", false, "show output from the rsync command used to copy data between PVCs")
+	flag.BoolVar(&options.SkipSourceValidation, "skip-source-validation", false, "migrate from PVCs using a particular StorageClass name, even if that StorageClass does not exist")
+	flag.IntVar(&options.PvcCopyTimeout, "pvc-copy-timeout", 300, "length of time to wait (in seconds) when transferring data from the source to the destination storage volume")


Suggested change

flag.IntVar(&options.PvcCopyTimeout, "pvc-copy-timeout", 300, "length of time to wait (in seconds) when transferring data from the source to the destination storage volume")

flag.IntVar(&options.PVCCopyTimeout, "pvc-copy-timeout", 300, "length of time to wait (in seconds) when transferring data from the source to the destination storage volume")

emosbaugh · 2022-12-02T16:43:24Z

pkg/k8sutil/storage.go

+
+	allpvs, err := cli.CoreV1().PersistentVolumes().List(ctx, metav1.ListOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("failed to get persistent volumes: %w", err)


Suggested change

return nil, fmt.Errorf("failed to get persistent volumes: %w", err)

return nil, fmt.Errorf("failed to list persistent volumes: %w", err)

pkg/k8sutil/storage.go

emosbaugh · 2022-12-02T19:25:55Z

pkg/preflight/validate.go

+	// validate access modes for all PVCs using the d source storage class
+	pvcs, err := pvcsForStorageClass(ctx, w, clientset, options.SourceSCName, options.Namespace)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get PVCs for storage %s: %s", options.SourceSCName, err)


Suggested change

return nil, fmt.Errorf("failed to get PVCs for storage %s: %s", options.SourceSCName, err)

return nil, fmt.Errorf("failed to get PVCs for storage %s: %w", options.SourceSCName, err)

emosbaugh · 2022-12-02T19:26:02Z

pkg/preflight/validate.go

+	}
+	pvcAccesModeFailures, err := validateVolumeAccessModes(ctx, w, clientset, options.DestSCName, time.Duration(options.PodReadyTimeout), pvcs)
+	if err != nil {
+		return nil, fmt.Errorf("failed validate PVC access modes: %s", err)


Suggested change

return nil, fmt.Errorf("failed validate PVC access modes: %s", err)

return nil, fmt.Errorf("failed validate PVC access modes: %w", err)

emosbaugh · 2022-12-02T19:27:16Z

pkg/preflight/validate.go

+
+	pvcEvents, err := client.CoreV1().Events(pvc.Namespace).Search(scheme.Scheme, pvc)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list events for PVC %s", pvc.Name)


Suggested change

return nil, fmt.Errorf("failed to list events for PVC %s", pvc.Name)

return nil, fmt.Errorf("failed to list events for PVC %s: %w", pvc.Name, err)

good catch. I've addressed this.

emosbaugh · 2022-12-02T19:28:38Z

pkg/preflight/validate.go

+	return srcPVCs, nil
+}
+
+func newK8sName(prefix, original string) string {


move this to k8sutil package?

Done. thanks.

pkg/preflight/validate.go

rrpolanco · 2022-12-05T17:45:39Z

Modified the output of the preflight validation to be generic.

When there is a validation failure when migrating from Rook to OpenEBS 👇

⚙  Waiting for OpenEBS CustomResourceDefinitions to be ready
✔ OpenEBS CustomResourceDefinitions are ready
Existing default storage class that is not OpenEBS LocalPV detected.
OpenEBS LocalPV will be installed as the non-default storage class.
storageclass.storage.k8s.io/openebs-localpv unchanged
awaiting openebs-localpv-provisioner deployment
Running Rook to OpenEBS Migration Checks ...

The following resources cannot be migrated:

NAMESPACE		RESOURCE			MESSAGE
---------		--------			-------
default			pvc/rook-rwx-claim		failed to provision volume with StorageClass "openebs-localpv": Only support ReadWriteOnce access mode
Cannot upgrade from Rook to OpenEBS due to previous errors.

emosbaugh · 2022-12-05T22:19:10Z

cmd/main.go

+		}
+
+		if len(failures) != 0 {
+			preflight.PrintValidationFailures(os.Stdout, failures)


should this be stderr?

I decided to separate the output of the validation failures to go to stdout and program errors to stderr.

laverya · 2022-12-06T17:16:26Z

cmd/main.go

+	// Cli uses CLI options to run Migrate
+	var options migrate.Options
+	var skipPreflightValidation bool
+	var preflightValidationOnly bool
+	flag.StringVar(&options.SourceSCName, "source-sc", "", "storage provider name to migrate from")
+	flag.StringVar(&options.DestSCName, "dest-sc", "", "storage provider name to migrate to")
+	flag.StringVar(&options.RsyncImage, "rsync-image", "eeacms/rsync:2.3", "the image to use to copy PVCs - must have 'rsync' on the path")
+	flag.StringVar(&options.Namespace, "namespace", "", "only migrate PVCs within this namespace")
+	flag.BoolVar(&options.SetDefaults, "set-defaults", false, "change default storage class from source to dest")
+	flag.BoolVar(&options.VerboseCopy, "verbose-copy", false, "show output from the rsync command used to copy data between PVCs")
+	flag.BoolVar(&options.SkipSourceValidation, "skip-source-validation", false, "migrate from PVCs using a particular StorageClass name, even if that StorageClass does not exist")
+	flag.IntVar(&options.PodReadyTimeout, "pod-ready-timeout", 60, "length of time to wait (in seconds) for volume validation pod(s) to go into Ready phase")
+	flag.BoolVar(&skipPreflightValidation, "skip-preflight-validation", false, "skip the volume access modes validation on the destination storage provider")
+	flag.BoolVar(&preflightValidationOnly, "preflight-validation-only", false, "skip the migration and run preflight validation only")
+
+	flag.Parse()
+


Why have we removed the CLI function?

I suppose that kurl isn't using it anymore, so it might not be useful...

I refactored this to avoid import cycles from pvmigrate package. Moreover, handling CLI flags in the pvmigrate package is not the right place to do this.

laverya · 2022-12-06T17:18:32Z

pkg/preflight/validate.go

+// Validate runs preflight check on storage volumes returning a list of failures
+func Validate(ctx context.Context, w *log.Logger, clientset k8sclient.Interface, options migrate.Options) ([]ValidationFailure, error) {


Should validateStorageClasses - which validates that storageclasses are present in the cluster - be part of Validate instead of Migrate?

The validation of storage classes will happen in both the validation and migration paths.

This the output when the validation can't find the storage class(es):

$ ./kurl/bin/pvmigrate --source-sc "non-default" --dest-sc "longhorn" --rsync-image "replicated/ku rl-util:alpha" --skip-free-space-check --preflight-validation-only The following resources cannot be migrated: NAMESPACE RESOURCE MESSAGE --------- -------- ------- sc/non-default Resource not found sc/longhorn Resource not found

laverya · 2022-12-06T17:20:44Z

pkg/migrate/migrate.go

-		fmt.Printf("%s\n", err.Error())
-		os.Exit(1)
-	}
+	PodReadyTimeout      int


This needs to be a time.Duration, not an int

laverya · 2022-12-06T17:24:35Z

pkg/k8sutil/label.go

+
+// NewPrefixedName returns a name prefixed by prefix and with length that is no longer than 63
+// chars
+func NewPrefixedName(prefix, original string) string {


Would you mind using this in Migrate? (specifically in newPvcName)

(This might require changing this to be NewSuffixedName for consistency)

will take this suggestion as a future TODO.

laverya

This looks great, but there's one other thing I'd like to see -

PLEASE update the readme with the additional CLI flags and process steps.

laverya · 2022-12-08T16:44:52Z

pkg/preflight/validate.go

@@ -98,6 +109,34 @@ func validateVolumeAccessModes(ctx context.Context, l *log.Logger, client k8scli
 	return volAccessModeFailures, nil
 }

+// validateStorageClasses returns any failures encountered when discovering the source and destination
+// storage classes
+func validateStorageClasses(ctx context.Context, l *log.Logger, clientset k8sclient.Interface, sourceSCName, destSCName string) ([]ValidationFailure, error) {


Could you copy the logic from the migrate version more precisely? This drops support for the 'skip-source-validation' flag

pkg/preflight/validate.go

emosbaugh · 2022-12-14T23:00:21Z

pkg/preflight/validate.go

+		if _, err := client.CoreV1().PersistentVolumes().Get(
+			ctx, pv.Name, metav1.GetOptions{},
+		); err != nil && !k8serrors.IsNotFound(err) {
+			l.Printf("failed to get pv for temp pvc %s: %s", pvc, err)
+		} else if err != nil && k8serrors.IsNotFound(err) {
+			break
 		}


can you return nil here?

Suggested change

if _, err := client.CoreV1().PersistentVolumes().Get(

ctx, pv.Name, metav1.GetOptions{},

); err != nil && !k8serrors.IsNotFound(err) {

l.Printf("failed to get pv for temp pvc %s: %s", pvc, err)

} else if err != nil && k8serrors.IsNotFound(err) {

break

}

if _, err := client.CoreV1().PersistentVolumes().Get(

ctx, pv.Name, metav1.GetOptions{},

); err != nil {

if k8serrors.IsNotFound(err) {

return nil

}

l.Printf("failed to get pv for temp pvc %s: %s", pvc, err)

}

emosbaugh · 2022-12-14T23:00:57Z

pkg/preflight/validate.go

+		case <-interval.C:
+			continue
+		case <-timeout:
+			return fmt.Errorf("failed to delete pvs: timeout")
 		}
 	}
 	return nil


Suggested change

return nil

I can't remove this return statement, Go requires a return.

this felt like unreachable code in my head based on the other change requested in my review. but ill take your word for it

emosbaugh

as i dont feel that strongly about my suggestion ill approve

rrpolanco mentioned this pull request Nov 22, 2022

Run Node Disk and Volume Validation Checks when Upgrading from Rook to OpenEBS replicatedhq/kURL#3767

Merged

camilamacedo86 reviewed Nov 22, 2022

View reviewed changes

emosbaugh requested changes Nov 22, 2022

View reviewed changes

laverya reviewed Nov 29, 2022

View reviewed changes

laverya requested changes Nov 29, 2022

View reviewed changes

laverya reviewed Nov 29, 2022

View reviewed changes

emosbaugh requested changes Dec 2, 2022

View reviewed changes

rrpolanco added 5 commits December 5, 2022 14:55

WIP: Check PVC access mode

6eead06

Still WIP

3ac07a8

More WIP

3ef7e7d

Adding tests

aa01da4

Add more unit test

61bc196

rrpolanco force-pushed the rrpolanco/sc-61144/check-pvc-accessmode branch from 7ae9c3d to 7c8d9e5 Compare December 5, 2022 21:04

emosbaugh requested changes Dec 5, 2022

View reviewed changes

laverya reviewed Dec 6, 2022

View reviewed changes

laverya previously approved these changes Dec 6, 2022

View reviewed changes

validate storage class resource is present in the cluster

9e2f86c

rrpolanco dismissed laverya’s stale review via 9e2f86c December 7, 2022 19:00

rrpolanco added 2 commits December 7, 2022 13:30

Change output message

263427f

Add todo

0659ab5

rrpolanco marked this pull request as ready for review December 7, 2022 19:37

rrpolanco added 3 commits December 7, 2022 14:08

Update flags and README

573e996

Update text for pod-ready-timeout flag

9d3323d

Add Ethan's suggestion

25f7358

rrpolanco changed the title ~~Check if PV Access Modes are Supported by the Volume Provider~~ Check if PV/PVC Access Modes are Supported by the Volume Provider Dec 7, 2022

laverya reviewed Dec 8, 2022

View reviewed changes

emosbaugh previously approved these changes Dec 8, 2022

View reviewed changes

--skip-source-validation works with preflight validation

b8382ca

rrpolanco dismissed emosbaugh’s stale review via b8382ca December 9, 2022 06:15

ricardomaraschini reviewed Dec 9, 2022

View reviewed changes

pkg/preflight/validate.go Outdated Show resolved Hide resolved

pkg/preflight/validate.go Show resolved Hide resolved

rrpolanco added 2 commits December 13, 2022 22:20

Simplify deleteTmpPVC()

92cb2b7

Update README with --delete-pv-timeout flag

726a160

rrpolanco mentioned this pull request Dec 14, 2022

bug: take only "default" namespace into account (OpenEBS check) replicatedhq/kURL#3837

Merged

rrpolanco added 2 commits December 14, 2022 14:57

fix timeout flags

54ba092

log name of the tempt pvc and not the struct

df71c22

emosbaugh requested changes Dec 14, 2022

View reviewed changes

emosbaugh approved these changes Dec 14, 2022

View reviewed changes

laverya approved these changes Dec 14, 2022

View reviewed changes

rrpolanco merged commit 19f941d into main Dec 14, 2022

	sourcePvcAnnotation = baseAnnotation + "-sourcepvc"
	sourcePVCAnnotation = baseAnnotation + "-sourcepvc"

	if err := p.k8scli.CoreV1().Pods(pod.Namespace).Delete(context.Background(), pod.Name, delopts); err != nil {
	// Cleanup should use background context so as not to fail if context has already been canceled
	if err := p.k8scli.CoreV1().Pods(pod.Namespace).Delete(context.Background(), pod.Name, delopts); err != nil {

	func (p PVMigrator) getPvcError(pvc corev1.PersistentVolumeClaim) (PVCError, error) {
	func (p PVMigrator) getPVCError(pvc corev1.PersistentVolumeClaim) (PVCError, error) {

	timeout := time.NewTicker(5 * time.Minute)
	timeout := time.After(5 * time.Minute)

		@@ -79,6 +106,37 @@ func Cli() {

		output := log.New(os.Stdout, "", 0) // this has no time prefix etc

		// escape hatch - for DEV/TEST ONLY

	fmt.Printf("failed to get volumes using storage class %s: %s", options.SourceSCName, err)
	output.Printf("failed to get volumes using storage class %s: %s", options.SourceSCName, err)

		timeoutCtx, cancelCtx := context.WithTimeout(ctx, timeout)
		defer cancelCtx()

	flag.IntVar(&options.PvcCopyTimeout, "pvc-copy-timeout", 300, "length of time to wait (in seconds) when transferring data from the source to the destination storage volume")
	flag.IntVar(&options.PVCCopyTimeout, "pvc-copy-timeout", 300, "length of time to wait (in seconds) when transferring data from the source to the destination storage volume")

	return nil, fmt.Errorf("failed to get persistent volumes: %w", err)
	return nil, fmt.Errorf("failed to list persistent volumes: %w", err)

	return nil, fmt.Errorf("failed to get PVCs for storage %s: %s", options.SourceSCName, err)
	return nil, fmt.Errorf("failed to get PVCs for storage %s: %w", options.SourceSCName, err)

	return nil, fmt.Errorf("failed validate PVC access modes: %s", err)
	return nil, fmt.Errorf("failed validate PVC access modes: %w", err)

	return nil, fmt.Errorf("failed to list events for PVC %s", pvc.Name)
	return nil, fmt.Errorf("failed to list events for PVC %s: %w", pvc.Name, err)

		// Validate runs preflight check on storage volumes returning a list of failures
		func Validate(ctx context.Context, w *log.Logger, clientset k8sclient.Interface, options migrate.Options) ([]ValidationFailure, error) {

Check if PV/PVC Access Modes are Supported by the Volume Provider #139

Check if PV/PVC Access Modes are Supported by the Volume Provider #139

Conversation

rrpolanco commented Nov 21, 2022 • edited

Solution

How it Works

rrpolanco commented Nov 22, 2022

camilamacedo86 Nov 22, 2022 • edited

Choose a reason for hiding this comment

rrpolanco Nov 29, 2022 • edited

Choose a reason for hiding this comment

camilamacedo86 Nov 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrpolanco Nov 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrpolanco commented Nov 21, 2022 •

edited

camilamacedo86 Nov 22, 2022 •

edited

rrpolanco Nov 29, 2022 •

edited

camilamacedo86 Nov 22, 2022 •

edited

rrpolanco Nov 29, 2022 •

edited