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 support for Kubernetes Pod Disruption Budgets #102

Open
wants to merge 6 commits into
base: custom
Choose a base branch
from
Open

Add support for Kubernetes Pod Disruption Budgets #102

wants to merge 6 commits into from

Conversation

jcarr-sailthru
Copy link

hi @sl1pm4t ,

Greetings all the way from Wellington! Big fans of this fork, thanks for your work here.

@clairefinnie has added support for Pod Disruption Budgets to our fork of your fork (fork2?) and it would be awesome to get this merged up into your repo.

We have validated the creation and deletion of the Pod Disruption Budgets, but we struggled to get a working environment to validate the tests that we wrote, so the tests themselves are untested.

regards,
Jethro

clairefinnie and others added 4 commits February 22, 2019 13:39
This resource includes validation to ensure PDB is correctly set to either
min_avaliable or max_unavailable and supports both integer and percentages.

As this resource cannot be updated in Kubernetes, we only ever create and
delete upon changes.
Note: Currently untested due to difficulties executing the test framework.
LAB-358 Add support for Pod Disruption Budgets
@sl1pm4t
Copy link
Owner

sl1pm4t commented Feb 22, 2019

Hi @jcarr-sailthru + @clairefinnie,
Thanks for making the big trip from Wellington to visit this repo, and thanks for the PR!
I'll give it a spin on some test infrastructure.

@jcarr-sailthru
Copy link
Author

Thanks @sl1pm4t - we just saw your new readme comment about the official branch as well, good to see that Hashicorp is giving this provider some love. We'll work to get this PR up to them as well.

This is primarily to allow us to define selectors with only keys & no values set. This all makes defining PDB more flexible.
Main change is that 'selector' is now a TypeList that can have match_labels (TypeMap, as selector was before) OR match_expressions (a TypeList with properties: key, operator & value)
Updated relevant flatten & expand functions that map the PDB Spec config & updated relevant tests.
@mbarrien
Copy link

As a warning, I opened a PR on the main repo for adding PodDisruptionBudget based on my own implementation (didn't see this one when I was writing mine). I doubt they'll accept it since it is a beta resource, but doing a code diff between our branches, there are important differences in implementation. In particular, your implementation makes selector required although strictly it seems to be optional. Another difference is missing import support.

Mine needs to add the check for min_available and max_unavailable not both being set simultaneously that your implementation has, although I'm not sure if placing that check in the expand is the right place.

I have tested it using the acceptance tests. I may open my own PR here once I rebase it against sl1pm4t's fork.

@clairefinnie
Copy link

Hi @mbarrien,

Didn't see your PDB work either before submitting this PR.

We have made some more changes to this PR with respect to setting selectors. We were unable to target resources with a specific key set, not caring about the value of that key, just looking for the presence of a key in labels. So we added the ability to use match expressions or match labels for selectors.

Main change is
'selector' is now a TypeList that can have

  • match_labels (TypeMap, as selector was before)
  • match_expressions (a TypeList with properties: key, operator & value)
    properties set

We think this new structure matches better with how they are implementing this in the latest upstream code, for some other resources .

Totally agree with you that the validation does not sit nicely inside the expand functions. I could add a separate validation method as you did.

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.

4 participants