Skip to content
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

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Kubestr browse PVC functionality #83

merged 3 commits into from
Oct 8, 2021

Conversation

bathina2
Copy link
Contributor

@bathina2 bathina2 commented Oct 6, 2021

No description provided.

@bathina2 bathina2 requested a review from onkarbhat October 6, 2021 22:48
Copy link
Member

@onkarbhat onkarbhat left a 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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"),
Copy link
Member

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 ?

Copy link
Member

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 ?

Copy link
Contributor Author

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

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Member

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 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue created #84

LocalPort int
// PodPort is the target port for the pod
PodPort int
// Steams configures where to write or read input from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Steams configures where to write or read input from
// Streams configures where to write or read input from


func (p *PVCBrowseArgs) Validate() error {
if p.PVCName == "" || p.Namespace == "" || p.VolumeSnapshotClass == "" {
return fmt.Errorf("Invalid PVCInspectorArgs (%v)", p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return fmt.Errorf("Invalid PVCInspectorArgs (%v)", p)
return fmt.Errorf("Invalid PVCBrowseArgs (%v)", p)

GenerateName: originalPodGenerateName,
PVCName: pvc.Name,
Namespace: args.Namespace,
//Cmd: fmt.Sprintf("echo '%s' >> /data/out.txt; sync; tail -f /dev/null", genString),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this ?

GenerateName: clonedPodGenerateName,
PVCName: pvc.Name,
Namespace: args.Namespace,
//Cmd: "tail -f /dev/null",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove ?

@bathina2 bathina2 merged commit 032878d into master Oct 8, 2021
@bathina2 bathina2 deleted the issue78 branch October 8, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants