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

feat: remove the creation of cronjobs in cleanup controller #8526

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

MariamFahmy98
Copy link
Collaborator

@MariamFahmy98 MariamFahmy98 commented Sep 25, 2023

Explanation

This PR removes the usage of cronjob in the cleanup controller. Instead the policy is requeued with a delay.
The reconciliation function do the following:

  1. If it's the first time to execute the cleanup policy:
    • calculate the next execution time based on the policy creation time.
    • calculate the delay between now and the next execution time.
    • re-queue the policy with the delay.
  2. If it isn't the first time to execute the cleanup policy:
    • check if this is the time to do the cleanup process, if so:
      • do the cleanup process.
      • update the cleanup policy status with the execution time.
      • calculate the next execution time.
      • calculate the delay between now and the next execution time.
      • re-queue the policy with the delay.
    • if it isn't the time to do the cleanup, then:
      • calculate the delay between now and the expected execution time.
      • re-queue the policy with the delay.

In case spec.schedule is modified, then the next execution time is re-calculated with the new value of spec.schedule.

Fixes #8306
Fixes #8073

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
@chipzoller
Copy link
Member

Does this take into consideration if the policy is updated to increase or decrease the time between the next reconciliation?

@eddycharly
Copy link
Member

Why not store lastExecutionTime ?

@MariamFahmy98
Copy link
Collaborator Author

Why not store lastExecutionTime ?

If a new cleanup policy is created, then the policy needs to be executed at least once to have status.lastExecutionTime set so how to know the time to do the cleanup process for the 1st time?

Does this take into consideration if the policy is updated to increase or decrease the time between the next reconciliation?

No. In case spec.schedule is modified, the next reconciliation happens depending on status.nextExecutionTime which is already calculated depending on the old value of spec.schedule before the policy itself modified. After the next reconciliation is executed, the status.nextExecutionTime will be calculated based on the new value of spec.schedule

@eddycharly
Copy link
Member

If a new cleanup policy is created, then the policy needs to be executed at least once to have status.lastExecutionTime set so how to know the time to do the cleanup process for the 1st time?

Take creationTime in this case ;)

@MariamFahmy98
Copy link
Collaborator Author

MariamFahmy98 commented Sep 25, 2023

@chipzoller - Now, if spec.schedule is modified then the policy is re-queued with a delay equals to the new value + the last execution time.

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #8526 (2bebbf8) into main (45a45b6) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #8526      +/-   ##
==========================================
- Coverage   35.78%   35.65%   -0.14%     
==========================================
  Files         313      313              
  Lines       25049    25143      +94     
==========================================
  Hits         8965     8965              
- Misses      15194    15288      +94     
  Partials      890      890              
Files Coverage Δ
api/kyverno/v2alpha1/cleanup_policy_types.go 45.20% <ø> (ø)
pkg/controllers/cleanup/condition.go 17.85% <ø> (ø)
api/kyverno/v2alpha1/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
pkg/controllers/cleanup/controller.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MariamFahmy98 MariamFahmy98 enabled auto-merge (squash) September 25, 2023 20:33
@eddycharly
Copy link
Member

The logic to get last execution time should be part of the interface imho.

@eddycharly
Copy link
Member

/cherry-pick release-1.11

@eddycharly eddycharly added the Documentation Update Documentation label Sep 26, 2023
@MariamFahmy98 MariamFahmy98 merged commit 7add300 into kyverno:main Sep 26, 2023
217 of 219 checks passed
gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Sep 26, 2023
* feat: remove the creation of cronjobs in cleanup controller

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>

* fix: use lastExecutionTime instead of nextExecutionTime

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>

---------

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
@MariamFahmy98 MariamFahmy98 deleted the remove-cleanup-cronjobs branch September 26, 2023 10:06
eddycharly pushed a commit that referenced this pull request Sep 26, 2023
…8528)

* feat: remove the creation of cronjobs in cleanup controller



* fix: use lastExecutionTime instead of nextExecutionTime



---------

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
Co-authored-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
@eddycharly eddycharly added the cherry-pick-completed The PR was cherry-picked (or merged) to required release branches label Sep 26, 2023
swastik959 pushed a commit to swastik959/kyverno that referenced this pull request Nov 1, 2023
…8526)

* feat: remove the creation of cronjobs in cleanup controller

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>

* fix: use lastExecutionTime instead of nextExecutionTime

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>

---------

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-completed The PR was cherry-picked (or merged) to required release branches cherry-pick-required Documentation Update Documentation milestone 1.11.0
Projects
None yet
3 participants