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

Add support for hostpath persistent volume definitions #6022

Merged
merged 1 commit into from
Jun 14, 2018
Merged

Add support for hostpath persistent volume definitions #6022

merged 1 commit into from
Jun 14, 2018

Conversation

dmsimard
Copy link
Contributor

@dmsimard dmsimard commented Nov 6, 2017

hostpath volumes 1 mount a file or directory from the host node’s
filesystem into a pod. This adds support for declaring a hostPath
volume as a persistent volume and do a persistent volume claim for
one for the hosted registry.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 6, 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

@dmsimard
Copy link
Contributor Author

dmsimard commented Nov 6, 2017

This is working for us right now but I'm not sure if there are other places where we might check against supported volume types.. like for registry there is openshift.hosted.registry.storage.kind | default(none) in ['nfs', 'openstack', 'glusterfs', 'hostpath']. I'm not familiar with the other roles so advice is appreciated.

@michaelgugino
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 6, 2017
@michaelgugino
Copy link
Contributor

@sdodson Is this a feature we wish to support?

@ewolinetz
Copy link
Contributor

I'd be in favor of adding support for this, since with logging and metrics we recommend host mounted volumes over NFS for performance reasons.

@dmsimard
Copy link
Contributor Author

dmsimard commented Nov 7, 2017

I'm realizing that this is a perhaps a bit more complicated after all, at least in the case of using for the registry.

The things to consider as far as I can tell:

  • The filesystem permissions must allow the UID/GID of the container to read/write on the hostpath location (similar to how it is done in gluster here and here)
  • There's also considerations as far as the security context constraints are concerned, see this documentation

I'll do some testing and update the PR accordingly.

@sdodson
Copy link
Member

sdodson commented Nov 7, 2017

@sdodson Is this a feature we wish to support?

yes, we definitely have use cases where hostmount is a good choice. I'd want to get some feedback from @openshift/storage on the best implementation.

@dmsimard
Copy link
Contributor Author

dmsimard commented Nov 7, 2017

@sdodson I'll defer to the experts here but I'm not sure that hostmount is the same thing as hostpath... Is it ? hostmount is specifically documented here.

@sdodson
Copy link
Member

sdodson commented Nov 7, 2017

@dmsimard sorry, hostpath is what i should've said

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 7, 2017
@dmsimard
Copy link
Contributor Author

dmsimard commented Nov 7, 2017

Here's a new commit that includes the hostpath permissions part I mentioned in a previous comment. I'll test this one out and validate the necessary changes for the scc, I need to reinstall my test instance.

michaelgugino
michaelgugino reviewed Nov 7, 2017
View reviewed changes
roles/openshift_hosted/tasks/storage/hostpath.yml Outdated
# There must be as many matching pods with 'Ready' status True as there are expected replicas
- "registry_pods.results.results[0]['items'] | oo_collect(attribute='status.conditions') | oo_collect(attribute='status', filters={'type': 'Ready'}) | map('bool') | select | list | count == openshift_hosted_registry_replicas | int"
delay: 10
retries: "{{ (600 / 10) | int }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelgugino I'd agree with you here, I thought it was very odd too. Frankly, this was copy/pasted in large part from the gluster parts here

I left it as-is because I thought it was so weird there must've been a good reason to do it that way ? Happy to change it :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Woops, my bad! That was supposed to be replaced by a timeout variable that defaults to 600. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

60 would be fine, I suppose. Let's just put 60 in there, having an equation with two hard coded values is super silly :)

roles/openshift_hosted/tasks/storage/hostpath.yml Outdated

- name: Determine registry fsGroup and runAsUser
set_fact:
openshift_hosted_registry_fsgroup: "{{ registry_pods.results.results[0]['items'][0].spec.securityContext.fsGroup }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're only consuming these variables here, no need to set fact, just use these in the file task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@dmsimard
Copy link
Contributor Author

@michaelgugino @abutcher rebased the pull request and addressed the comments.

@openshift-merge-robot
Copy link
Contributor

@dmsimard PR needs rebase

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2017
@dabelenda
Copy link
Contributor

maybe someone can also look at #6250 which allows to use PV/PVC for local storage ? (/me doing self-promotion since he doesn't know who to ping)

@dmsimard
Copy link
Contributor Author

@michaelgugino @abutcher Are you still interested in this ? We've been successfully using it for a long time but rebasing it all the time is tedious. I can rebase it once more if we can finally merge it.

@michaelgugino
Copy link
Contributor

@dmsimard Let's rebase and get this in. The change is pretty self-contained, I don't see a reason to not merge it.

michaelgugino
michaelgugino approved these changes May 29, 2018
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/ok-to-test

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 29, 2018
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2018
@dmsimard dmsimard force-pushed the hostpath_volumes branch 2 times, most recently from a94c265 to feb24fb Compare June 13, 2018 14:57
@dmsimard
Copy link
Contributor Author

@sdodson @michaelgugino this has been rebased against master and should be good to land unless you notice any issues.

vrutkovs
vrutkovs reviewed Jun 13, 2018
View reviewed changes
roles/lib_utils/action_plugins/generate_pv_pvcs_list.py Outdated
@@ -90,6 +90,26 @@ def build_pv_glusterfs(self, varname=None):
path=path,
readOnly=read_only)))

def build_pv_hostpath(self, varname=None):
"""Build pv dictionary for hostpath storage type"""
Copy link
Member

Choose a reason for hiding this comment

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

roles/lib_utils/action_plugins/generate_pv_pvcs_list.py:94: [W0311(bad-indentation), ] Bad indentation. Found 12 spaces, expected 8

Add support for hostpath persistent volume definitions
6937ac4 
hostpath volumes [1] mount a file or directory from the host node’s
filesystem into a pod. This adds support for declaring a hostPath
volume as a persistent volume and do a persistent volume claim for
one for the hosted registry.

[1]: https://kubernetes.io/docs/concepts/storage/volumes/
@dmsimard
Copy link
Contributor Author

@vrutkovs oops, not sure how that happened. Fixed.

@abutcher
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abutcher, dmsimard, michaelgugino

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

@sdodson
Copy link
Member

sdodson commented Jun 13, 2018

/retest

@dmsimard
Copy link
Contributor Author

/test install

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 14, 2018 •
edited
Loading

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

Test name Commit Details Rerun command
ci/openshift-jenkins/gcp-upgrade 6937ac4 link /test gcp-upgrade

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.

@sdodson
Copy link
Member

sdodson commented Jun 14, 2018

gcp infra flake

@sdodson sdodson merged commit 5be4cfd into openshift:master Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarrpa jarrpa jarrpa left review comments

@vrutkovs vrutkovs vrutkovs left review comments

@michaelgugino michaelgugino michaelgugino approved these changes

Assignees

@abutcher abutcher

@michaelgugino michaelgugino

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/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.