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 k8s aws and dynamic pv support (fixes #3) #33

Merged
merged 2 commits into from Feb 18, 2018

Conversation

bkconrad
Copy link
Contributor

@bkconrad bkconrad commented Feb 8, 2018

Configure the kubernetes AWS cloud provider support through openshift. This by extension enables persistent volume support with dynamic provisioning through k8s (closing #3 ). Confirmed by testing with the persistent rails sample app.

To support this I had to use the internal AWS hostnames in the inventory, which in turn required outputting the inventory. See:
openshift/openshift-ansible#5692

IAM Policies are taken from the AWS reference architecture. They are configured using access key pairs so that containers don't have access to the host's permissions via instance profile.

I plan to further generalize this module to support configurable host counts and sizes, which will build on the inventory generation in this PR.

@bkconrad bkconrad changed the title add k8s aws and dynamic pv support add k8s aws and dynamic pv support (fixes #3) Feb 8, 2018
@dwmkerr
Copy link
Owner

dwmkerr commented Feb 9, 2018

@bkconrad this is really, really nice! It cleans up a few issues which have been nagging at me for a while, and the removal of the whole sed step to generate the inventory is a great improvement.

I'm going to create the cluster and run some tests now, I'll also update the docs describing a little more about PVs and how to use them with this set up. Will probably be the end of the weekend before I can merge as I'm pretty swamped, but I'm super happy with these changes!

@stevenhaddox
Copy link

We ran this PR last Friday and it seems to have fixed our persistent storage issues with Jenkins in OpenShift (which was our first failing thing). Looks promising as a new user to this repo :)

@dwmkerr
Copy link
Owner

dwmkerr commented Feb 14, 2018

Brilliant, thanks for sharing @stevenhaddox! Hoping to get this in the mainline very soon, just a bit swamped at work this week! Am also adding some extra testing facilities and docs explaining how to use the PVs 😄

@dwmkerr
Copy link
Owner

dwmkerr commented Feb 16, 2018

OK final merge notes!

Conflicts

With the recent changes for linting (#35) the PR has conflicts, I'm fixed these on the command line, but am now fixing them on this PR as well so that it shows correctly as merged and that you are properly included as a contributor.

Inventory

I love the removal of the grotty sed command with the much cleaner terraform templating for inventory.cfg. However, I've removed the inventory section from the terraform variables, as I think for end users it is easier to quickly open the inventory.template.cfg to edit the node labels and see what their inventory will be, rather than having to jump to the terraform code for this.

Hostnames

I used the private addresses for the hosts in the inventory (essentially copying what was in the terraform template back into the inventory) but noticed that we end up with the AWS hostnames:

image

I realised this was related to the issue you'd mentioned, so have opened this issue #40 to track the hostnames. For now it looks like we need to use the AWS generated hostnames if using the AWS cloud provider, I'll watch for updates on the k8s repo.

Docs

I've added some basic docs for the PVs.

Thanks again for this @bkconrad - it's a fantastic improvement on the codebase!

@dwmkerr dwmkerr merged commit 7e32ab8 into dwmkerr:master Feb 18, 2018
ronaldkonjer pushed a commit to ronaldkonjer/terraform-aws-openshift that referenced this pull request Mar 22, 2018
add k8s aws and dynamic pv support (fixes dwmkerr#3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants