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

Create PodDisruptionBudget for every integration #1787

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

mmelko
Copy link
Contributor

@mmelko mmelko commented Oct 26, 2020

Closes: #1760

This is what I have so far. PDB is created for every integration and deleted with integration. I decided to go with new trait for the PDB but I can refactor if necessary.

I still need to create test for the trait and also I'm not sure if IntegrationRunning phase is the best here. Any feedback appreciated.

Release Note

feat: Configure PodDisruptionBudget for integrations

@mmelko
Copy link
Contributor Author

mmelko commented Oct 26, 2020

pkg/trait/pdb.go Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
@mmelko mmelko force-pushed the PodDisruptionBudget-trait branch 2 times, most recently from 4be1e54 to c11ef47 Compare October 30, 2020 10:28
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
pkg/trait/pod.go Outdated Show resolved Hide resolved
@astefanutti
Copy link
Member

I think permissions to get, create, update the PDB resources must be added to the operator RBAC.

@mmelko
Copy link
Contributor Author

mmelko commented Nov 2, 2020

Should that be in operator-role-kubernetes/openshift,yaml file ?

@astefanutti
Copy link
Member

Should that be in operator-role-kubernetes/openshift,yaml file ?

Exactly, and the generated resources, like the OLM CSV updated accordingly.

pkg/trait/pod.go Outdated Show resolved Hide resolved
deploy/operator-role-olm.yaml Show resolved Hide resolved
deploy/operator-role-openshift.yaml Outdated Show resolved Hide resolved
pkg/trait/pod_test.go Outdated Show resolved Hide resolved
@mmelko mmelko force-pushed the PodDisruptionBudget-trait branch 3 times, most recently from f20766e to 10a0a98 Compare November 4, 2020 13:50
@mmelko mmelko changed the title WIP: Create PodDisruptionBudget for every integration Create PodDisruptionBudget for every integration Nov 5, 2020
@mmelko
Copy link
Contributor Author

mmelko commented Nov 5, 2020

I've tested this also with knative and cron-job deployment strategies and it works as expected.

@astefanutti
Copy link
Member

astefanutti commented Nov 5, 2020

@lburgazzoli @nicolaferraro 👋,

That PR introduces the pod trait, whose purpose is to be responsible for all the aspects that directly relate to the integration pods configuration. That includes upcoming work to support:

Technically, the affinity trait should be merged into that new trait, yet we may want to keep that concern separated, at least to maintain compatibility for the 1.x lifespan.

It turns out we already have traits responsible for the container and deployment aspects, but none for those of the pods. The deployment trait could have been a candidate to meet that need. However it's specific to the Deployment strategy, while the pod aspects can be shared across deployment strategies.

WDYT?

@lburgazzoli
Copy link
Contributor

I guess this also relates to: #1657

@astefanutti
Copy link
Member

I guess this also relates to: #1657

Damn auto-complete, that's the one I wanted to type. Corrected my previous comment.

@lburgazzoli
Copy link
Contributor

I'm thinking if after we'll have #1657, we should reduce the number of traits to the bare minimum and with a very opinionated list of what can be configured via traits and leave everything else to pod templates & co

@lburgazzoli
Copy link
Contributor

maybe also have a flag to use a pod tamplate from a local yaml

@astefanutti
Copy link
Member

After thinking a bit based on this discussion, I'd suggest we introduce a pdb trait for the sole scope of that PR, and start working on the new pod trait to address #1657, and see what other options it could be responsible for.

@mmelko
Copy link
Contributor Author

mmelko commented Nov 11, 2020

I refactored it back to pdb trait.

deploy/operator-role-kubernetes.yaml Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
@mmelko mmelko force-pushed the PodDisruptionBudget-trait branch 2 times, most recently from 3b2bd2a to eee1ade Compare November 12, 2020 10:31
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb.go Outdated Show resolved Hide resolved
pkg/trait/pdb_test.go Outdated Show resolved Hide resolved
@astefanutti astefanutti merged commit e4f52eb into apache:master Nov 12, 2020
@nicolaferraro nicolaferraro mentioned this pull request Dec 22, 2020
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.

Ability to configure PodDisruptionBudget for integrations
3 participants