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

Add port-forward functionalities to virtctl vmexport #10148

Merged
merged 4 commits into from Aug 17, 2023

Conversation

alromeros
Copy link
Contributor

@alromeros alromeros commented Jul 20, 2023

What this PR does / why we need it:

VirtualMachineExport volumes are only downloadable outside the cluster when proper ingress or routes are configured.

In some cases, it's helpful to force a way to download the volumes without this configuration, so this pull request introduces an option to set port-forwarding using the vmexport service so that we can get the vmexport contents outside the cluster without additional configuration.

Usage examples:

# To download the contents of pvc-test into disk.img.gz through a random available local port
$ virtctl vmexport download vm1-export --pvc=pvc-test --output=disk.img.gz --port-forward

# To download the contents of pvc-test into disk.img.gz through the local port 4342
$ virtctl vmexport download vm1-export --pvc=pvc-test --output=disk.img.gz --port-forward --local-port=4342

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Add port-forward functionalities to vmexport

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Jul 20, 2023
@alromeros alromeros force-pushed the port-forward-vmexport branch 2 times, most recently from 78c7282 to 022607a Compare July 20, 2023 14:43
pkg/virtctl/vmexport/vmexport.go Outdated Show resolved Hide resolved
pkg/virtctl/vmexport/vmexport.go Show resolved Hide resolved
pkg/virtctl/vmexport/vmexport.go Outdated Show resolved Hide resolved
pkg/virtctl/vmexport/vmexport.go Show resolved Hide resolved
pkg/virtctl/vmexport/vmexport.go Outdated Show resolved Hide resolved
VirtualMachineExport volumes are only downloadable outside the cluster when proper ingress or routes are configured.

In some cases it's useful to force a way to download the volumes without this configuration, so this commit introduces an option to set port-forwarding using the vmexport service, so we can get the vmexport contents outside the cluster without additional configuration.

Signed-off-by: Alvaro Romero <alromero@redhat.com>
@alromeros alromeros force-pushed the port-forward-vmexport branch 2 times, most recently from 61dfa61 to 566a1a8 Compare July 25, 2023 09:25
…-dump

Signed-off-by: Alvaro Romero <alromero@redhat.com>
@alromeros alromeros force-pushed the port-forward-vmexport branch 4 times, most recently from 015a3e9 to 1fb27ec Compare July 27, 2023 11:17
@alromeros alromeros changed the title [WIP] Add port-forward functionalities to virtctl vmexport Add port-forward functionalities to virtctl vmexport Jul 27, 2023
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2023
@alromeros
Copy link
Contributor Author

/retest-required

@alromeros alromeros requested a review from awels July 27, 2023 16:44
@alromeros
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.25-sig-compute-migrations

Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

looks good, just a few nitpicks

pkg/virtctl/utils/test_utils.go Outdated Show resolved Hide resolved
pkg/virtctl/vmexport/vmexport_test.go Show resolved Hide resolved
Signed-off-by: Alvaro Romero <alromero@redhat.com>
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023
@alromeros
Copy link
Contributor Author

/retest-required

@alromeros
Copy link
Contributor Author

/retest

@mhenriks
Copy link
Member

@alromeros looks good but wondering why user must supply a port number? Can't one be chosen randomly or just listen on "127.0.0.1:0" and check the returned listener address?

@alromeros
Copy link
Contributor Author

@alromeros looks good but wondering why user must supply a port number? Can't one be chosen randomly or just listen on "127.0.0.1:0" and check the returned listener address?

I assumed it was simply better to allow users to specify the port number. I can change it, but other than being less straightforward the current implementation seems more robust to me. wdyt? If you want I can make the port random.

@mhenriks
Copy link
Member

@alromeros looks good but wondering why user must supply a port number? Can't one be chosen randomly or just listen on "127.0.0.1:0" and check the returned listener address?

I assumed it was simply better to allow users to specify the port number. I can change it, but other than being less straightforward the current implementation seems more robust to me. wdyt? If you want I can make the port random.

I think that the user shouldn't have to care about the port since the tunnel is transient and only exists during command execution. I am not against giving the user an option to explicitly specify the port though

@alromeros
Copy link
Contributor Author

@alromeros looks good but wondering why user must supply a port number? Can't one be chosen randomly or just listen on "127.0.0.1:0" and check the returned listener address?

I assumed it was simply better to allow users to specify the port number. I can change it, but other than being less straightforward the current implementation seems more robust to me. wdyt? If you want I can make the port random.

I think that the user shouldn't have to care about the port since the tunnel is transient and only exists during command execution. I am not against giving the user an option to explicitly specify the port though

Okay, in that case, I will change the port-forward flag to be a bool and add a new one called target-port so users can specify an optional port.

@kubevirt-bot kubevirt-bot added size/XXL and removed lgtm Indicates that a PR is ready to be merged. size/XL labels Aug 16, 2023
This commit allows using port-forward without specifying any specific port number.

Port forward will listen on a random available port.

Signed-off-by: Alvaro Romero <alromero@redhat.com>
@alromeros
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.27-sig-compute-migrations

@alromeros alromeros requested a review from awels August 16, 2023 15:48
@alromeros
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.27-sig-compute-migrations

@mhenriks
Copy link
Member

/lgtm
/approve

Great job! Should update the PR description to reflect new args/usage

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2023
@kubevirt-bot kubevirt-bot merged commit c709e13 into kubevirt:main Aug 17, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants