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 possibility to enable Persistent Local Storage using Ansible #6250

Merged
merged 13 commits into from
May 9, 2018

Conversation

dabelenda
Copy link
Contributor

No description provided.

@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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 24, 2017
@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from e5657df to 09f0291 Compare November 24, 2017 09:57
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 24, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from 226f7cf to 1d3b498 Compare November 30, 2017 09:50
@dabelenda dabelenda changed the title [WIP] Add possibility to enable Persistent Local Storage using Ansible Add possibility to enable Persistent Local Storage using Ansible Nov 30, 2017
@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 Nov 30, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from 1d3b498 to 365470f Compare December 1, 2017 07:35
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch 2 times, most recently from d729f66 to 18a43cd Compare December 8, 2017 09:47
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 8, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from 18a43cd to d32950e Compare December 11, 2017 06:38
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch 3 times, most recently from 15d4a67 to 474a850 Compare December 14, 2017 12:14
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2017
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from 09fa03d to af825ba Compare January 16, 2018 12:33
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2018
tasks:
- name: Create Persistent Local Storage Classes Directories
file:
path: "/mnt/local-storage/{{ item }}"
Copy link
Member

Choose a reason for hiding this comment

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

path should be configurable

@dabelenda
Copy link
Contributor Author

/test crio

Copy link
Member

@vrutkovs vrutkovs 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, although default /mnt/local-storage is defined multiple times across the playbook, it would be nice to have it set in one place only

fieldPath: metadata.namespace
- name: VOLUME_CONFIG_NAME
value: local-volume-provisioner-config
image: quay.io/external_storage/local-volume-provisioner:v1.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Image location and version should be parametrized

@@ -585,6 +585,11 @@ openshift_master_identity_providers=[{'name': 'htpasswd_auth', 'login': 'true',
# openshift_storageclass_name=gp2
# openshift_storageclass_parameters={'type': 'gp2', 'encrypted': 'false'}
#
# PersistentLocalStorage
Copy link
Member

Choose a reason for hiding this comment

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

example hosts should include openshift_persistentlocalstorage_path (with default value)

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2018
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from aee199e to efce631 Compare February 19, 2018 07:33
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2018
@dabelenda
Copy link
Contributor Author

@vrutkovs I guess the duplication of the default value comes from my inexperience with ansible, but I do not see how to reduce it further than what I already did (I removed the default value from the role. I thought that roles should be independent from the playbook and thus put the defaults there, too)

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2018
@cmoulliard
Copy link

Can this PR been updated, rebased and reviewed please ?

@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from 5d81ef3 to b882647 Compare April 12, 2018 13:27
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2018
@dabelenda dabelenda force-pushed the add_persistentlocalvolumes branch from b882647 to 6500d24 Compare May 7, 2018 15:03
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dabelenda
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: michaelgugino

Assign the PR to them by writing /assign @michaelgugino in a comment when ready.

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

@vrutkovs
Copy link
Member

vrutkovs commented May 9, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2018
@vrutkovs vrutkovs merged commit d8a8f1d into openshift:master May 9, 2018
Conan-Kudo added a commit to Conan-Kudo/openshift-ansible that referenced this pull request Jul 20, 2018
Adapted from PR#6250 to openshift-ansible, originally authored by
Diego Abelenda <[email protected]> for OpenShift Ansible 3.10.

PR reference: openshift#6250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

7 participants