-
Notifications
You must be signed in to change notification settings - Fork 47
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
Kubestr browse PVC functionality #83
Conversation
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.
Nice work @bathina2 👏🏼 . Looks great overall. Some minor comments to be addressed before merging.
cmd/rootCmd.go
Outdated
} | ||
dyncli, err := kubestr.LoadDynCli() | ||
if err != nil { | ||
fmt.Printf("Failed to load kubeCLi (%s)", err.Error()) |
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.
fmt.Printf("Failed to load kubeCLi (%s)", err.Error()) | |
fmt.Printf("Failed to load dynCli (%s)", err.Error()) |
cmd/rootCmd.go
Outdated
) { | ||
kubecli, err := kubestr.LoadKubeCli() | ||
if err != nil { | ||
fmt.Printf("Failed to load kubeCLi (%s)", err.Error()) |
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.
fmt.Printf("Failed to load kubeCLi (%s)", err.Error()) | |
fmt.Printf("Failed to load kubeCli (%s)", err.Error()) |
StorageClassName: &args.StorageClass, | ||
Resources: v1.ResourceRequirements{ | ||
Requests: v1.ResourceList{ | ||
v1.ResourceStorage: resource.MustParse("1Gi"), |
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.
What does the 1Gi here refer to ?
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.
Looks like this is the default size that will be set if args.RestoreSize is not provided by the user ?
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.
Yes it is a default size. It's also the value that will get set during the createPVC step of the CSI snapshot restore check. This is being tracked by another issue #75
pkg/csi/csi_ops.go
Outdated
targetSnapClassName := clonePrefix + args.VolumeSnapshotClass | ||
err := snapshotter.CloneVolumeSnapshotClass(ctx, args.VolumeSnapshotClass, targetSnapClassName, kansnapshot.DeletionPolicyRetain, nil) | ||
if err != nil { | ||
return errors.Wrapf(err, "Failed to create a VolumeSnapshotClass to use to restore the snapshot") |
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.
return errors.Wrapf(err, "Failed to create a VolumeSnapshotClass to use to restore the snapshot") | |
return errors.Wrapf(err, "Failed to clone a VolumeSnapshotClass to use to restore the snapshot") |
} | ||
|
||
dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, http.MethodPost, &url.URL{Scheme: "https", Path: path, Host: hostIP}) | ||
fw, err := pf.New(dialer, []string{fmt.Sprintf("%d:%d", req.LocalPort, req.PodPort)}, req.StopCh, req.ReadyCh, &req.OutStream, &req.ErrOutStream) |
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.
Probably helpful to validate early on that the local port is available . Otherwise you might go through the whole process of creating a clone of the pvc and creating the file browser app , only to find that the local port is already used. We could file a separate github issue as enhancement to track this .
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.
Issue created #84
pkg/csi/types/csi_types.go
Outdated
LocalPort int | ||
// PodPort is the target port for the pod | ||
PodPort int | ||
// Steams configures where to write or read input from |
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.
// Steams configures where to write or read input from | |
// Streams configures where to write or read input from |
pkg/csi/types/csi_types.go
Outdated
|
||
func (p *PVCBrowseArgs) Validate() error { | ||
if p.PVCName == "" || p.Namespace == "" || p.VolumeSnapshotClass == "" { | ||
return fmt.Errorf("Invalid PVCInspectorArgs (%v)", p) |
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.
return fmt.Errorf("Invalid PVCInspectorArgs (%v)", p) | |
return fmt.Errorf("Invalid PVCBrowseArgs (%v)", p) |
pkg/csi/snapshot_restore.go
Outdated
GenerateName: originalPodGenerateName, | ||
PVCName: pvc.Name, | ||
Namespace: args.Namespace, | ||
//Cmd: fmt.Sprintf("echo '%s' >> /data/out.txt; sync; tail -f /dev/null", genString), |
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.
Remove this ?
pkg/csi/snapshot_restore.go
Outdated
GenerateName: clonedPodGenerateName, | ||
PVCName: pvc.Name, | ||
Namespace: args.Namespace, | ||
//Cmd: "tail -f /dev/null", |
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.
Remove ?
No description provided.