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.

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

Manage and use own storage pool #1956

Merged
merged 3 commits into from
Jul 16, 2019
Merged

Manage and use own storage pool #1956

merged 3 commits into from
Jul 16, 2019

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Jul 9, 2019

Don't assume default storage pool and create a new custom one.

Fixes #1457.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 9, 2019
@zeenix
Copy link
Contributor Author

zeenix commented Jul 9, 2019

/test e2e-libvirt
/label platform/libvirt

@zeenix
Copy link
Contributor Author

zeenix commented Jul 9, 2019

gah, that's not at all a useful log from tf-fmt: https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/1956/pull-ci-openshift-installer-tf-fmt/5020 :(

@zeenix zeenix force-pushed the own-storage-pool branch from b5433f4 to f8e859f Compare July 9, 2019 14:42
@zeenix
Copy link
Contributor Author

zeenix commented Jul 9, 2019

/test e2e-libvirt

1 similar comment
@zeenix
Copy link
Contributor Author

zeenix commented Jul 9, 2019

/test e2e-libvirt

zeenix added 2 commits July 9, 2019 18:09
@zeenix
Update terraform-libvirt-provider to 0.5.2
74546e6 
We'll be using the newly added API to create a custom storage pool in a
following patch.
@zeenix
libvirt: Use custom storage pool
53bdc3e 
Don't assume `default` storage pool and create a new custom one.

Fixes openshift#1457.
@zeenix zeenix force-pushed the own-storage-pool branch from f8e859f to 353905f Compare July 9, 2019 16:09
@praveenkumar
Copy link
Contributor

/test e2e-libvirt

@zeenix
Copy link
Contributor Author

zeenix commented Jul 10, 2019

/assign @skitt

Could you do a review @skitt please?

skitt
skitt suggested changes Jul 10, 2019
View reviewed changes
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

LGTM (I haven’t tested this yet), apart from the path used for the storage pool. I’ll check the SELinux behaviour.

@zeenix
docs,libvirt: Drop storage pool related info
cfcd2f4 
Installer now manages and uses its own storage pool so this info is now
redundant.
@zeenix zeenix force-pushed the own-storage-pool branch from 353905f to cfcd2f4 Compare July 10, 2019 14:23
@zeenix
Copy link
Contributor Author

zeenix commented Jul 11, 2019

/test e2e-aws

Also as per bot's orders :)

/assign @abhinavdahiya

@abhinavdahiya
Copy link
Contributor

74546e6 is
/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2019
@zeenix
Copy link
Contributor Author

zeenix commented Jul 12, 2019 •
edited
Loading

seems it still needs /lgtm. @skitt could you please give that?

@zeenix
Copy link
Contributor Author

zeenix commented Jul 12, 2019

74546e6 is
/approve

Thanks. Do you mean only that commit is approved?

@skitt
Copy link
Member

skitt commented Jul 12, 2019

seems it still needs /lgtm. @skitt could you please give that?

I’m not sure mine counts, but here goes:

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, skitt, zeenix

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

@zeenix
Copy link
Contributor Author

zeenix commented Jul 12, 2019

@skitt thanks, obviously it does. :)

/test e2e-aws

@skitt
Copy link
Member

skitt commented Jul 12, 2019

@skitt thanks, obviously it does. :)

Yup, I need to flex my muscles a bit more ;-).

@markmc
Copy link
Contributor

markmc commented Jul 16, 2019

/hold

Sorry I'm late to this, but since the baremetal platform also uses libvirt (for the bootstrap VM), we will probably want to follow whatever you do here

I completely agree with the premise of this - users should not have to manually create the "default" storage pool before running the installer

But consider this - it has always been intended that libvirt installation would result in the /var/lib/libvirt/images path being created and then the first user of that would create the 'default' storage pool pointing to that. Here's what virt-manager and virt-install do:

https://www.redhat.com/archives/libvir-list/2008-August/msg00179.html
https://github.com/virt-manager/virt-manager/blob/2b24a85884f8242ab2ade14e6403d447be07f0f1/virtinst/storage.py#L154-L163

(Whether this was a good idea or not is another matter - I'm just explaining the intent!)

My conclusion - openshift-install should follow this approach and just create the default storage pool if it doesn't exist.

Why? I think /var/lib/libvirt/images is a well known location (and e.g. people are used to it containing large VM images) so I think it would be better for us to continue using that rather than switch to /var/lib//libvirt/openshift-images

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2019
@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2019

@markmc I created #1457 months ago and spent considerable amount of time implementing storage pools in terraform-provider-libvirt. In my discussion with Openshift devs, nobody objected to this before either so can you imagine my frustration on you blocking this at the last mile of a rather long journey?

People expecting images to be under a very specific location is not a great argument IMO and certainly not a good one to present this late. I already updated the code to not assume a specific path for images:

openshift/cluster-api-provider-libvirt#144
#1628

If your code assumes a path, please update it as that is not the correct thing to do. As mentioned before default storage pool is not guaranteed to exist to assuming this pool's existance is not correct from libvirt's POV.

As for the need for this, this is part of my effort to hopefully move us to the session libvirt connection by default or at least be able to use session libvirt if user asks for it. Currently Installer happily let's you provide a libvirt connection URL but it can't actually handle anything other than the system libvirt connection. One reason that is the case is assumption of default storage pool in a specific location.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2019

Here's what virt-manager and virt-install

Yes, that's where the assumption of default storage pool at a specific location comes from since most libvirt users use either or both of these tools but there is no reason to believe that is all the users. There is a reason why libvirt:

  1. doesn't create the default pool for you.
  2. has the storage pool creation/management API and not just volumes.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2019

openshift-install should follow this approach and just create the default storage pool if it doesn't exist.

I actually thought about that but how do you implement this? Can you tell TF to not fail if the resource already exists and not manage it? I couldn't think of any easy solution there and since every libvirt client should use it's own storage pool, I didn't see investing time on solving this non-problem.

@markmc
Copy link
Contributor

markmc commented Jul 16, 2019

The hold was to ask you to give the point some consideration before merging, that's all

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2019

@markmc oh ok. I'm terribly sorry that I ended up sounding so rude. It wasn't intentional.

@openshift-ci-robot
Copy link
Contributor

@zeenix: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 cfcd2f4 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.

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.

@openshift-merge-robot openshift-merge-robot merged commit b6cf548 into openshift:master Jul 16, 2019
@zeenix zeenix deleted the own-storage-pool branch July 17, 2019 09:48
@zeenix zeenix mentioned this pull request Jul 23, 2019
Closed
praveenkumar added a commit to crc-org/snc that referenced this pull request Sep 9, 2019
@praveenkumar
Issue #75 Add logic to get qcow2 location for 4.2 cluster
ae43b08 
Due to openshift/installer#1956 , now
by default the location of the qcow2 images are part of
`/var/lib/libvirt/openshift/<cluster_name>_<random_hash>` so
this change will make sure we are copy correct file as part of
our CI.
@praveenkumar praveenkumar mentioned this pull request Sep 9, 2019
Merged
praveenkumar added a commit to crc-org/snc that referenced this pull request Sep 10, 2019
@praveenkumar
Issue #75 Add logic to get qcow2 location for 4.2 cluster
c66c4ca 
Due to openshift/installer#1956 , now
by default the location of the qcow2 images are part of
`/var/lib/libvirt/openshift/<cluster_name>_<random_hash>` so
this change will make sure we are copying the correct file as part of
our CI.
praveenkumar added a commit to crc-org/snc that referenced this pull request Sep 10, 2019
@praveenkumar
Issue #75 Add logic to get qcow2 location for 4.2 cluster
71ba318 
Due to openshift/installer#1956 , now
by default the location of the qcow2 images are part of
`/var/lib/libvirt/openshift/<cluster_name>_<random_hash>` so
this change will make sure we are copying the correct file as part of
our CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skitt skitt skitt approved these changes

@jstuever jstuever Awaiting requested review from jstuever

@vrutkovs vrutkovs Awaiting requested review from vrutkovs

Assignees

@skitt skitt

@abhinavdahiya abhinavdahiya

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. platform/libvirt size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use own storage pool
8 participants
@zeenix @praveenkumar @abhinavdahiya @skitt @openshift-ci-robot @markmc @openshift-bot @openshift-merge-robot