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 reconciling logic for creating cronjobs whenever a new cleanup policy is created #5385

Merged
merged 14 commits into from
Nov 25, 2022

Conversation

NikhilSharmaWe
Copy link
Contributor

Signed-off-by: Nikhil Sharma nikhilsharma230303@gmail.com

Explanation

This PR adds reconciling logic for creating CronJob whenever a new cleanup policy is created.

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #5385 (fbad540) into main (fa88f4a) will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #5385   +/-   ##
=======================================
  Coverage   36.42%   36.42%           
=======================================
  Files         172      172           
  Lines       19087    19087           
=======================================
  Hits         6952     6952           
  Misses      11340    11340           
  Partials      795      795           
Impacted Files Coverage Δ
api/kyverno/v1alpha1/cleanup_policy_types.go 48.50% <50.00%> (ø)

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

@eddycharly
Copy link
Member

@NikhilSharmaWe we need to reconcile observed state with desired state, it means we can't just create cronjobs, the schedule could change for example.

@eddycharly
Copy link
Member

@NikhilSharmaWe I think code can be factorised, I will push a commit if it's ok.

@eddycharly
Copy link
Member

@NikhilSharmaWe I pushed the changes, I took the opportunity to add generic support to enqueue funcs in controller utils.

@eddycharly eddycharly marked this pull request as ready for review November 22, 2022 21:28
Comment on lines +132 to +184
cronjobNs := namespace
if namespace == "" {
cronjobNs = config.KyvernoNamespace()
}
Copy link
Member

Choose a reason for hiding this comment

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

@NikhilSharmaWe - can you clarify how the namespace is selected to create the cronjob? For clustercleanuppolicy and cleanuppolicy respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For CleanupPolicy, the cronjob namespace is the same as the policy's namespace.
For ClusterCleanupPolicy, the cronjob namespace will be the Kyverno namespace.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Nikhil, that's what I understood.

Not sure if it makes sense to create cronjobs for all policies in the kyverno namespace. It could be confusing to use different namespaces for cleanup policies. This is easy to change, we can update it in the follow-up PRs if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Cross namespace owner references are not allowed.

Copy link
Member

Choose a reason for hiding this comment

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

In case of a namespaced policy the cronjob has to live in the same namespace.

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.

Looks good overall, have a question about the cronjob's namespace. See comments:

realshuting
realshuting previously approved these changes Nov 23, 2022
@realshuting realshuting enabled auto-merge (squash) November 23, 2022 06:28
@realshuting realshuting self-assigned this Nov 23, 2022
@eddycharly eddycharly added this to the Kyverno Release 1.9.0 milestone Nov 23, 2022
auto-merge was automatically disabled November 23, 2022 13:02

Head branch was pushed to by a user without write access

NikhilSharmaWe and others added 11 commits November 23, 2022 19:48
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
@eddycharly
Copy link
Member

Let's try to get this in 🤞

@eddycharly eddycharly enabled auto-merge (squash) November 25, 2022 08:09
@eddycharly eddycharly merged commit 8547c8f into kyverno:main Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants