Skip to content

vshpere: download ova into cache #2922

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.

Sign up for GitHub

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

Jump to bottom
Closed
jstuever wants to merge 1 commit into from
Closed

vshpere: download ova into cache #2922

jstuever wants to merge 1 commit into from

Conversation

@jstuever
Copy link
Contributor

@jstuever jstuever commented Jan 14, 2020 •
edited
Loading

This change caches a copy of the vmware ova specified in rhcos.json in
preperation to upload the OVA into a Template.

  • Determine ova uri from rhcos.json
  • Download and cache ova
  • Enable override of ova uri in install-config.yaml

Depends on: #2889 #2893

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 14, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign crawford
You can assign the PR to them by writing /assign @crawford in a comment when ready.

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

Details 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

@jstuever
Copy link
Contributor Author

jstuever commented Jan 14, 2020

/uncc @jhixson74
/cc @jcpowermac

@openshift-ci-robot openshift-ci-robot requested review from jcpowermac and removed request for jhixson74 January 14, 2020 22:39
jcpowermac
jcpowermac reviewed Jan 15, 2020
View reviewed changes
@jstuever jstuever force-pushed the cors1337 branch 2 times, most recently from 9201d19 to 7a14ed3 Compare January 18, 2020 22:43
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2020
@jstuever jstuever force-pushed the cors1337 branch 2 times, most recently from 7120267 to d85bfa0 Compare January 18, 2020 23:41
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2020
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2020
@jstuever jstuever force-pushed the cors1337 branch 3 times, most recently from 30edba5 to 533209b Compare January 22, 2020 19:01
@jstuever jstuever changed the title [WIP] vshpere: download ova and create template [WIP] vshpere: download ova into cache Jan 22, 2020
@jstuever
vshpere: download ova into cache
882ac28 
This change caches a copy of the vmware ova specified in rhcos.json in
preperation to upload the OVA into a Template.
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 23, 2020
@jcpowermac jcpowermac mentioned this pull request Jan 24, 2020
Closed
5 tasks
@jstuever jstuever changed the title [WIP] vshpere: download ova into cache vshpere: download ova into cache Jan 24, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2020
@jstuever jstuever requested a review from jcpowermac January 24, 2020 18:38
@jstuever
Copy link
Contributor Author

jstuever commented Jan 24, 2020

/retest

1 similar comment
@jstuever
Copy link
Contributor Author

jstuever commented Jan 24, 2020

/retest

patrickdillon
patrickdillon reviewed Jan 29, 2020
View reviewed changes
pkg/asset/cluster/tfvars.go
if err != nil {
return errors.Wrapf(err, "failed to get %s Terraform variables", platform)
}
installConfig.Config.VSphere.ClusterOSImage = cachedImage
Copy link
Contributor

@patrickdillon patrickdillon Jan 29, 2020

Choose a reason for hiding this comment

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

What is the effect of doing this? Is this value saved or used later?

Copy link
Contributor Author

@jstuever jstuever Feb 3, 2020

Choose a reason for hiding this comment

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

The goal here is to stuff the cached location back into the installConfig so we can pass it to the preterraform func. https://github.com/openshift/installer/pull/2922/files#diff-661643e1b9745123649391076bfa5a01R22. I don't believe it's ever written out to disk, though.

Copy link
Contributor

@abhinavdahiya abhinavdahiya Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think you can assume writeback to your dependency assets, the asset store could easily be providing you the ready only copies.

dependency loops are not great.

@patrickdillon
Copy link
Contributor

patrickdillon commented Jan 29, 2020

One question https://github.com/openshift/installer/pull/2922/files#r372164571 but
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2020
@jstuever
Copy link
Contributor Author

jstuever commented Feb 3, 2020

/retest

1 similar comment
@jstuever
Copy link
Contributor Author

jstuever commented Feb 4, 2020

/retest

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 4, 2020

@jstuever: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovirt 882ac28 link /test e2e-ovirt
ci/prow/e2e-libvirt 882ac28 link /test e2e-libvirt
ci/prow/e2e-aws-scaleup-rhel7 882ac28 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jstuever
Copy link
Contributor Author

jstuever commented Feb 20, 2020

/unassign @patrickdillon
/assign @abhinavdahiya
for your approval.

abhinavdahiya
abhinavdahiya reviewed Feb 20, 2020
View reviewed changes
pkg/asset/cluster/vsphere/vsphere.go
Comment on lines +20 to +25
// PreTerraform performs any infrastructure initialization which must
// happen before Terraform creates the remaining infrastructure.
func PreTerraform(ctx context.Context, infraID string, installConfig *installconfig.InstallConfig) error {
// TODO: create VM Template using cachedImage
return nil
}
Copy link
Contributor

@abhinavdahiya abhinavdahiya Feb 20, 2020

Choose a reason for hiding this comment

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

the goal for pre terraform was to put in code that was not easily done by terraform itself, like tagging subnets for AWS.

my preference would be keep any cloud creation into terraform, we can easily keep local terraform providers for the custom objects/operations like the uploading ova.

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Feb 20, 2020

I think there is a serious issue with
a) updating an dependency from tfvars asset.
b) the setup of pre-terraform for ova. one i don't think this PR needs that changeset, another that we need to follow that path.

@jstuever
Copy link
Contributor Author

jstuever commented Feb 26, 2020

Going to be solved a different way.

@jstuever jstuever closed this Feb 26, 2020
@jstuever jstuever deleted the cors1337 branch June 17, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrickdillon patrickdillon patrickdillon left review comments

@jcpowermac jcpowermac Awaiting requested review from jcpowermac

+1 more reviewer

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@abhinavdahiya abhinavdahiya

Labels

lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jstuever @openshift-ci-robot @patrickdillon @abhinavdahiya @jcpowermac