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

Change wrap app/db PV definitions into kube templates #224

Merged
merged 1 commit into from
Sep 25, 2017

Conversation

tbielawa
Copy link
Contributor

@tbielawa tbielawa commented Sep 21, 2017

This is a follow up to #222 RFE/Question - Turn PV examples into templates


Description

The current docs describe creating the required PVs in terms of simply editing the provided app/db examples with the required NFS paths. This change turns creating PVs into a no-editing-required process by wrapping the PV objects in kube templates.

Usage

Using a template in this PR is pretty simple! No more editing required

Template Parameters

  • DB_PV_SIZE - Defaults to the recommended PV size for the DB template
  • APP_PV_SIZE - ** Defaults to the recommended PV size for the app template
  • BASE_PATH - Defaults to /exports
  • NFS_HOST - No Default - Value must be provided when processing/creating

Method 1 - Create Template, Process and Create Later

# oc create -f /path/to/miq-pv-example.yaml
# ... do stuff ...
# oc process miq-pv-example -p NFS_HOST=nfs.example.com | oc create -f -

Method 2 - Process Template and Create in one pass

# oc process /path/to/miq-pv-example.yaml -p NFS_HOST=nfs.example.com | oc create -f -

The current descriptions don't look like the rest of the manageiq descriptions, probably want to re-word these

[root@m01 ~]# oc get template
NAME              DESCRIPTION                                  PARAMETERS     OBJECTS
manageiq          ManageIQ appliance with persistent storage   54 (1 blank)   23
manageiq-app-pv   PV Template for MIQ Server                   3 (1 blank)    1
manageiq-db-pv    PV Template for MIQ PostgreSQL DB            3 (1 blank)    1

Questions

  1. Do you want to change the template descriptions to match what you already are doing?
  2. The app/db templates are using different parameter names for their spec.capacity.storage parameters: APP_PV_SIZE vs. DB_PV_SIZE. Do you want those to stay different, or do you want them to be the same, i.e., unify on a single name like MIQ_PV_SIZE or something?

@tbielawa
Copy link
Contributor Author

Oh I made the spec tests upset.

@tbielawa
Copy link
Contributor Author

I ran the rspec locally, I think I fixed the lint. Silly double quoting.

@fbladilo
Copy link
Contributor

fbladilo commented Sep 21, 2017

@tbielawa @carbonin I believe those default sizes are reasonable for a standard deployment (probably oversized for dev environments). I guess eventually we could revisit them, should be easy to adjust if needed.

Question regarding the templated PV creation :

Will the NFS shares now be configured automagically by OpenShift when the template is created/processed? Or this is something that still has to be done manually by Admin?

@tbielawa
Copy link
Contributor Author

@fbladilo said

Will the NFS shares now be configured by OpenShift when the template is created/processed? Or this is something that still has to be done manually by Admin.

Using the openshift-ansible openshift_cfme role, with the default value for openshift_cfme_storage_class (nfs) then:

  1. the templates are created
  2. the app PV is processed and created
  3. the db PV is conditionally processed and created

Note: in the PR there are still some hard-coded hostnames. I'm going to fix those to be standardized today

Using the role or not, this project will need a README update to describe how to create the actual PVs using the proposed templates in this PR. Method 1 and Method 2 (described in the OP) should be a good starting point

@fbladilo
Copy link
Contributor

fbladilo commented Sep 21, 2017

@tbielawa 👍 Yep will certainly will need to adjust our instructions on README, looks like an improvement. I like option 2 with a single pass modifying default parameters.

@carbonin
Copy link
Member

  1. Do you want to change the template descriptions to match what you already are doing?

I'm not very concerned with the descriptions, I think what you have here is fine.

  1. The app/db templates are using different parameter names for their spec.capacity.storage parameters: APP_PV_SIZE vs. DB_PV_SIZE. Do you want those to stay different, or do you want them to be the same, i.e., unify on a single name like MIQ_PV_SIZE or something?

I think the same parameter name would be more user friendly. Maybe just PV_SIZE namespacing any more in a template that is already called manageiq-*-pv feels a bit redundant.

Also, @tbielawa would you mind updating the README as a part of this patch? The explanation that you have in your description seems great.

@tbielawa
Copy link
Contributor Author

@carbonin no problem to all comments

tbielawa added a commit to tbielawa/manageiq-pods that referenced this pull request Sep 23, 2017
@tbielawa tbielawa force-pushed the pv_templates branch 3 times, most recently from 8eb821e to 5ba9b86 Compare September 23, 2017 17:19
@tbielawa
Copy link
Contributor Author

tbielawa commented Sep 24, 2017 via email

README.md Outdated
miq-pv02 5Gi RWO Recycle Available 19s
NAME CAPACITY ACCESSMODES RECLAIMPOLICY STATUS CLAIM STORAGECLASS REASON AGE
miq-app 4Gi RWO Retain Bound manageiq/manageiq-server-manageiq-0 1d
miq-db 10Gi RWO Retain Bound manageiq/manageiq-postgresql 1d
Copy link
Member

Choose a reason for hiding this comment

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

After PV creation, but before deploying the app I wouldn't expect these to be Bound

@tbielawa
Copy link
Contributor Author

tbielawa commented Sep 25, 2017 via email

@tbielawa
Copy link
Contributor Author

@carbonin fixed that for ya

@miq-bot
Copy link
Member

miq-bot commented Sep 25, 2017

Checked commit tbielawa@e648adc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the patch @tbielawa

@carbonin carbonin added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 25, 2017
@carbonin carbonin merged commit 9d06b0b into ManageIQ:master Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants