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 possible implementations for CleanupController #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NikhilSharmaWe
Copy link
Contributor

Signed-off-by: Nikhil Sharma [email protected]

Description

This PR adds possible implementations for CleanupController.

Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

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

What about failure recovery?

Please check misspellings carefully, updation is not an English word.

@@ -233,6 +233,87 @@ Kyverno could alternatively leverage a CronJob resource to perform the deletions
| CronJob will do both: schedule the deletion and execute the deletion. | A CronJob created through the cleanup rule should be ignored by other rules(validate and mutate). For example, A validate policy should not block the creation of that CronJob. This can cause some complexities in the approach. We can label these CronJobs and ignore them while applying other policies. |
| The CronJobs can be easily removed from the cluster after the completion by setting their ownerReference to the resource it was created to delete. | |

## Finalized Implementation
Copy link
Member

Choose a reason for hiding this comment

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

Suggest renaming to "Proposed Implementations".

Comment on lines +270 to +276
1. A new policy is added to the cluster.
2. The cleanup policy object is added to the controller queue with the help of CleanupPolicy and ClusterCleanupPolicy informers.
3. The controller then gets the policy.
4. Get the matching resources for that rule/policy.
5. Create CronJob for scheduling the deletion for each resource.
6. Kyverno’s Service account is used in CronJob.
7. OwnerReference of the CronJob is set to the trigger resource, so when the matching resource is deleted, the CJ is also deleted.
Copy link
Member

Choose a reason for hiding this comment

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

I would expect the end-to-end reconciliation logic to be added to the Flow section but failed to envision that way.


**Life Cycle of the CronJob:**

In this approach lifecycle of the CronJob created will be same as the matching resource. Since the OwnerReference of the matching resource will be set to the mathcing and this way CronJob's garbase collection will be handled.
Copy link
Member

Choose a reason for hiding this comment

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

In this approach lifecycle of the CronJob created will be the same as the matching resource. The trigger resource will be set as the owner of the created CronJob and it will be garbage-collected by Kubernetes when the trigger is deleted.


| Pros | Cons |
| --- | --- |
| Deletion of CJs after resource cleanup can be easily handled through OwnerReference | Creating CJs for each resource could be inefficient in large clusters. |
Copy link
Member

Choose a reason for hiding this comment

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

Cons - one CJ is created for each matching resource, this adds an additional X number of resources into the cluster

** X is the number of matching resources in the Kyverno policy.



### 2. Creating CronJobs per rule/policy.
A cleanup controller will be creating CronJob per rule/policy. Creation and updation of a new cleanup policy will trigger the reconciliation. When reconciliation is triggered, a CronJob is created and delete commands for all the matching resources are informed to the CronJob and are executed at the same schedule (specified in the policy).
Copy link
Member

Choose a reason for hiding this comment

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

..will be creating creates a CronJob .. Creation and updation update..



### 2. Creating CronJobs per rule/policy.
A cleanup controller will be creating CronJob per rule/policy. Creation and updation of a new cleanup policy will trigger the reconciliation. When reconciliation is triggered, a CronJob is created and delete commands for all the matching resources are informed to the CronJob and are executed at the same schedule (specified in the policy).
Copy link
Member

Choose a reason for hiding this comment

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

CronJob is created and delete commands for all the matching resources are informed to the CronJob and are executed at the same schedule (specified in the policy).

How? Using kubectl or some other ways?



### 1. Creating CronJobs per resource.
A cleanup controller will be creating a CronJob for each of the matching resources. CronJob will run the kubectl command to delete these resources. CronJob will run according to the schedule specified in the cleanupPolicy. Creation and updation of a new cleanup policy will trigger the reconciliation and the reconciliation logic will get all the matching resources and will create a CronJob per resource which will have info and commands to delete the matching resource.
Copy link
Member

Choose a reason for hiding this comment

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

"A cleanup controller will be creating" ->..will create a CronJob..

"CronJob will run according to the schedule specified in the cleanupPolicy." -> The CronJob is executed at the given schedule specified in the cleanup policy.

If this is a brief summary of the solution, you can remove "and the reconciliation logic will get all the matching resources and will create a CronJob per resource which will have info and commands to delete the matching resource.", this is detailed implementation.

Comment on lines 294 to 303
1. A new policy is added to the cluster.
2. The policy object is added to the controller queue with the help of CleanupPolicy and ClusterCleanupPolicy informers.
3. Get the matching resources for that policy. The matching resources could be of different kinds.
4. Delete commands for all the matching resources are informed to the CronJob and are executed at the same schedule.
5. On getting the admission request, fetch the CronJob created for the policy with the help of labels consisting of the rule and policy name.
6. Kyverno’s Service account is used in CronJob.
Copy link
Member

Choose a reason for hiding this comment

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

Not clear to me.


**Life Cycle of the CronJob:**

In this approach the life-cycle of the CronJob will same as the CleanupPolicy or ClusterCleanupPolicy resource which triggers the creation of the CronJob
Copy link
Member

@realshuting realshuting Nov 18, 2022

Choose a reason for hiding this comment

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

In this approach the life-cycle of the CronJob will be the same as the CleanupPolicy or the ClusterCleanupPolicy resource which triggers the creation of the CronJob.


| Pros | Cons |
| --- | --- |
| Creating CJs per rule / per Kind & schedule generates less load in the cluster in comparison to creating CJs per resource. | Need to come up with an approach for CJ deletion. Since it is not as straightforward as in the previous approach. |
Copy link
Member

Choose a reason for hiding this comment

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

Creating CJs per rule / per Kind

??

@NikhilSharmaWe NikhilSharmaWe force-pushed the cleanupController branch 3 times, most recently from db1690b to af7e9d3 Compare November 18, 2022 18:51
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.

2 participants