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

Drop volumes full paths #144

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Drop volumes full paths #144

merged 1 commit into from
Apr 25, 2019

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Apr 16, 2019

The path of the storage volume is and should be determined by the containing storage pool and hence we should not associate and assume the path of the volume.

Without this change, it is not possible for user to specify alternative location for storage volumes to Openshift Installer.

WARNING:

  • This will also require a change in the Openshift Installer as that still passess the volume path instead of name to the actuator.

  • I've not testing this yet, only built successfully.

This is a revision/rebase of #45

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 16, 2019
@zeenix zeenix mentioned this pull request Apr 16, 2019
Closed
@zeenix
Copy link
Contributor Author

zeenix commented Apr 16, 2019

Hmm.. i see a bunch of issues. I'll fix them!

@zeenix zeenix force-pushed the drop-volumes-paths-rebased branch from b784ca5 to 4c15371 Compare April 16, 2019 14:00
zeenix added a commit to zeenix/installer that referenced this pull request Apr 16, 2019
@zeenix
libvirt: Drop volume paths
cfbf99c 
Volume should be identified by their names and their paths should be
left to their container pools. This change is needed to allow for custom
storage locations.

This is the needed update in Installer for the actual change in the
libvirt actuator: openshift/cluster-api-provider-libvirt#144
@zeenix zeenix mentioned this pull request Apr 16, 2019
Merged
@zeenix
Copy link
Contributor Author

zeenix commented Apr 16, 2019

The e2e tests won't work I think cause this requires a small change in Installer as well: openshift/installer#1628

@zeenix zeenix force-pushed the drop-volumes-paths-rebased branch from 4c15371 to 4bad91d Compare April 16, 2019 17:52
@zeenix
Copy link
Contributor Author

zeenix commented Apr 16, 2019

The e2e tests won't work I think cause this requires a small change in Installer as well: openshift/installer#1628

Actually I made the changes backwards compat so existing API should work and hence e2e tests should succeed now. I'll check tomorrow why that's not the case.

@zeenix zeenix force-pushed the drop-volumes-paths-rebased branch 3 times, most recently from 050ec1d to 412ef21 Compare April 23, 2019 17:54
@ingvagabund
Copy link
Member

/retest

@ingvagabund
Copy link
Member

ingvagabund commented Apr 24, 2019 •
edited
Loading

Repeating error:

Error: packet_device.libvirt: "facilities": required field is not set

Error: packet_device.libvirt: "facility": [REMOVED] Use the "facilities" array instead, i.e. change 
  facility = "ewr1"
to 
  facilities = ["ewr1"]

Fixing with #147

@ingvagabund
Copy link
Member

/approve

no objections here, just waiting until CI goes green

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2019
@ingvagabund
Copy link
Member

/test e2e

@zeenix
Drop volumes full paths
920d098 
The path of the storage volume is and should be determined by the
containing storage pool and hence we should not associate and assume the
path of the volume.

Without this change, it is not possible for user to specify alternative
location for storage volumes to Openshift Installer.

This will also require a change in the Openshift Installer as that still
passess the volume path instead of name to the actuator.

Based on a patch from Enxebre <[email protected]>.
@zeenix zeenix force-pushed the drop-volumes-paths-rebased branch from 412ef21 to 920d098 Compare April 24, 2019 15:32
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 24, 2019 •
edited
Loading

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

Test name Commit Details Rerun command
ci/prow/actuator-pkg-staleness 920d098 link /test actuator-pkg-staleness

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.

@ingvagabund
Copy link
Member

/test e2e

@ingvagabund
Copy link
Member

@zeenix is the change compatible with the installer? Anything else left to do before the PR merges?

@frobware
Copy link
Contributor

@zeenix is the change compatible with the installer? Anything else left to do before the PR merges?

I'm assuming this needs to merge first per the comments in openshift/installer#1628 (comment)

@frobware
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2019
@openshift-merge-robot openshift-merge-robot merged commit ba0dc75 into openshift:master Apr 25, 2019
@zeenix
Copy link
Contributor Author

zeenix commented Apr 25, 2019

@zeenix is the change compatible with the installer? Anything else left to do before the PR merges?

I did my best to ensure that it is but I failed to test it since it turns out it's not easy to make Installer launch the cluster with custom-built actuator.

@zeenix
Copy link
Contributor Author

zeenix commented Apr 25, 2019

I'm assuming this needs to merge first per the comments in openshift/installer#1628 (comment)

Indeed. Thanks for the review and merge.

@steven-ellis steven-ellis mentioned this pull request May 21, 2019
Closed
@zeenix zeenix mentioned this pull request Jul 16, 2019
Merged
@cfergeau cfergeau mentioned this pull request May 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frobware frobware Awaiting requested review from frobware

@paulfantom paulfantom Awaiting requested review from paulfantom

Assignees

@frobware frobware

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. 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.
5 participants
@zeenix @ingvagabund @openshift-ci-robot @frobware @openshift-merge-robot