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 kubevirt_cdi_upload module #52990

Merged
merged 1 commit into from
Mar 1, 2019
Merged

Conversation

mmazur
Copy link
Contributor

@mmazur mmazur commented Feb 26, 2019

SUMMARY

Adds a module for uploading VM images from local system to kubernetes PVC's using the Containerized Data Importer

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

kubevirt_cdi_upload

@mmazur mmazur force-pushed the kubevirt_cdi_upload branch 3 times, most recently from e54f677 to 4d94ee0 Compare February 26, 2019 16:04
@ansibot
Copy link
Contributor

ansibot commented Feb 26, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Feb 26, 2019
@mmazur
Copy link
Contributor Author

mmazur commented Feb 26, 2019

FYI: can't use module_utils.urls here, since that lacks user CA support. We can drop the external import once that feature gets added.

@pkliczewski
Copy link

@mmazur I am not sure what do you want to drop and why. Please provide more details.

@ansibot
Copy link
Contributor

ansibot commented Feb 26, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/kubevirt/kubevirt_template.py:0:0: E325 Argument 'resource_definition' in argument_spec defines type as <function list_dict_str at 0x7fce1da047b8> but documentation defines type as 'str'

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. labels Feb 26, 2019
@jamescassell
Copy link
Contributor

@mmazur I'm also interested in being able to use a user CA with ansible tools like get_url and uri. Do you know if there's an existing feature request?

@mmazur
Copy link
Contributor Author

mmazur commented Feb 27, 2019

@jamescassell A long time ago someone tried this, but then gave up. Old PR: #32049

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 27, 2019
@mmazur
Copy link
Contributor Author

mmazur commented Feb 27, 2019

@pkliczewski that was a note to any ansible core dev that will need to merge this, which is required because this module uses the external python requests lib, because the internal one lacks the required functionality. So please disregard that comment in your review.

@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 27, 2019
@pkliczewski
Copy link

shipit

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 27, 2019
@ansibot ansibot added the shipit This PR is ready to be merged by Core label Feb 27, 2019
# Let's check the file's there before we do anything else
imgfile = open(path, 'rb')

resource = self.find_resource(KIND, api_version, fail=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using find_supported_resource here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sh… I forgot.

@@ -161,6 +161,7 @@ lib/ansible/modules/cloud/google/gcpubsub_facts.py E322
lib/ansible/modules/cloud/google/gcpubsub_facts.py E324
lib/ansible/modules/cloud/google/gcpubsub_facts.py E326
lib/ansible/modules/cloud/google/gcspanner.py E322
lib/ansible/modules/cloud/kubevirt/kubevirt_cdi_upload.py E203
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we can't fix this issue? AFAIK shouldn't be complex to fix,no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd need to take this and rewrite it: #32049, then get it merged, then modify this patch to use it. The first time this issue came came up (when merging k8s_auth) I figured that'd be my plan B, as that's quite a time investment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and openshift which we depend on uses requests (or was it urllib3?) anyways under the hood, so it's not like there's anything gained from the user's POV.

@gundalow gundalow merged commit fb4d0d8 into ansible:devel Mar 1, 2019
@gundalow
Copy link
Contributor

gundalow commented Mar 1, 2019

Merged into devel for release in Ansible 2.8

Would be great if #32049 could be moved forward as @mmazur suggested.

description:
- URL containing the host and port on which the CDI Upload Proxy is available.
- "More info: U(https://github.com/kubevirt/containerized-data-importer/blob/master/doc/upload.md#expose-cdi-uploadproxy-service)"
upload_host_verify_ssl:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this different than the equivalent option specified in k8s_auth_options? Does this module connect to two API endpoints, and you could have two different cert-validation settings? Can we re-use the one defined in k8s_auth_options here?

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, this is completely different. Stuff in k8s_auth_options controls connectivity to the kubernetes cluster's api server endpoint. This module does that, but then also connects to a service running on the cluster itself (called the Containerized Data Importer Upload Proxy), which is why it has a completely separate set of upload_host_* parameters that control that.

It's probable that before The Freeze I'll be attempting to switch the default on upload_host_verify_ssl to autodetect (aka "look for a Secret with the certs in a standard place") for ease of use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that clarification. Upon reviewing the code, it wasn't obvious to me that this module is actively using both connection paths (to the k8s API and also to the Upload Proxy.) I've taken your word for it and updated my related PR.

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants