Add custom per-repo interval in AppRepo #5599

antgamdia · 2022-11-02T13:51:03Z

Description of the change

This PR is adding a new field (Interval) in the AppRepository CRD. This field will override the global default crontab used for every repository, which is aligned with the recent Repositories API we defined.

The PR includes every required change across the components, namely:

Add Interval field in the CR definition.
Add this field when spinning up jobs in the apprepository-controller and use it; otherwise, use the default one.
Use this value in the AddRepo/UpdateRepo methods of the Repos API (Helm plugin)
Remove any workarounds preventing the UI from setting the interval in Helm repos.

Benefits

Helm repos can have customizable intervals now.

Possible drawbacks

~~IRL test is still pending. I don't know if adding a field in the CRD is breaking something, though we already added some fields in the past with no impact, but want to re-check just in case.~~

Applicable issues

fixes Kubeapps Helm AppRepository should support configurable sync interval #3661

Additional information

After the IRL test, some notes:

Helm does not upgrade the CRDs, so you have to delete and install them.
- If the CRD is outdated, then the interval is not being saved, falling back to the previous, classical behavior.
One caveat is that the upgrade process: since we are creating a CronJob and passing the crontab value on creation via the apprepository controller, from the Repos API we don't have a clear way to edit the cronjob once generated.
- In this PR we've addressed it by just disabling the field when upgrading

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

netlify · 2022-11-02T13:51:09Z

✅ Deploy Preview for kubeapps-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`90b9ab0`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/6376b3348c4c1900089a5916
😎 Deploy Preview	https://deploy-preview-5599--kubeapps-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity

Thanks Antonio, nice clean change :)

One caveat is that the upgrade process: since we are creating a CronJob and passing the crontab value on creation via the apprepository controller, from the Repos API we don't have a clear way to edit the cronjob once generated.
* In this PR we've addressed it by just disabling the field when upgrading

I think this should be explained in a bit more detail in the code, or in an issue, that can be referenced in the code. I'd currently have to dig a bit to understand why we can't delete and re-create the CronJob when updating the AppRepository CRD.

absoludity · 2022-11-07T02:18:30Z

cmd/apprepository-controller/server/controller.go

+		cronTime, err = intervalToCron(apprepo.Spec.Interval)
+	}
+	// If the interval is invalid, use the default global crontab
+	if err != nil {


Hmm - I know we don't yet have any way to report this back to the user (other than failing the operation, which isn't great either), so I reckon it'd be useful to at least log this error so that we can help debug if there's an issue?

absoludity · 2022-11-07T02:19:45Z

cmd/apprepository-controller/server/controller.go

 			// Set to replace as short-circuit in k8s <1.12
 			// TODO re-evaluate ConcurrentPolicy when 1.12+ is mainstream (i.e 1.14)
 			// https://github.com/kubernetes/kubernetes/issues/54870
-			ConcurrencyPolicy: "Replace",
+			ConcurrencyPolicy: batchv1.ReplaceConcurrent,


Dunno, but does this change to the concurrency policy mean we can remove the above comment? EDIT: hah, or it's not a change at all, just switch to constant :)

Exactly, I just replaced the Replace string with the constant coming from the k8s package. Below the different options:

const ( // AllowConcurrent allows CronJobs to run concurrently. AllowConcurrent ConcurrencyPolicy = "Allow" // ForbidConcurrent forbids concurrent runs, skipping next run if previous // hasn't finished yet. ForbidConcurrent ConcurrencyPolicy = "Forbid" // ReplaceConcurrent cancels currently running job and replaces it with a new one. ReplaceConcurrent ConcurrencyPolicy = "Replace" )

absoludity · 2022-11-07T02:31:52Z

cmd/apprepository-controller/server/controller_test.go

+		{
+			name:         "good interval, every two hours",
+			interval:     "2h",
+			expectedCron: "*/120 * * * *",


Is that actually valid? I thought Cron was stateless (and so effectively reset at the turn of each hour for a minutes range here). I'm not 100%, but worth checking (see some notes on the wikipedia article - not a source of truth obviously).

Could be worth using https://pkg.go.dev/github.com/robfig/cron#Every , not sure.

➕ I don't know either if that's even valid, but besides that, I think that if we allow values higher than 60 min, the intervalToCron function should turn this kind of value to * */2 * * *, which is a more readable cron expression.

Thanks for your comments and pointers, I've had to revisit my knowledge about how cron works internally.

The expression I was building is syntactically valid, that's why I thought it was valid (=meaningful). However, it is semantically pointless given how the cron works.
Kubernetes is using the vixie-cron implementation, the POSIX-compliant implementation developed for BSD (back in 1988).

NOTE: There are some other implementations, like dcron, fcron, bcron and cronie. The latter, is a vixie-cron fork maintained by Fedora where, for instance, does validate the step field and therefore makes the discussed expression invalid syntactically.

Well, so coming back to the vixie implementation, the step can be any number:

https://github.com/vixie/cron/blob/2e298fc87bf6f9220d09e60162ebf909a607c4fb/entry.c#L518-L521

But, it is going to, silently, drop the invalid values:

https://github.com/vixie/cron/blob/2e298fc87bf6f9220d09e60162ebf909a607c4fb/entry.c#L580-L584

In short, as you pointed out, in an interval 0-59, the only possible values for step are [1-59].

Unfortunately, this is something that k8s is NOT validating:

k get cronjob NAME SCHEDULE SUSPEND ACTIVE LAST SCHEDULE AGE kubernetes-cron-job */61 * * * * False 0 <none> 20m

Note the "last schedule" equals to "none"

The library you pointed out seems to be a scheduler itself, as it does not generate the crontab thing, but actually runs a timer to calculate the next executions.

So, it seems we might need to make the function a bit smarter :S and limit the values + perform the proper conversions between time units.

The library you pointed out seems to be a scheduler itself, as it does not generate the crontab thing, but actually runs a timer to calculate the next executions.

Yes, sorry - I realised the library was for a scheduler, but had thought that function I linked to was for converting a duration into a crontab format, but seems it is not.

absoludity · 2022-11-07T02:37:37Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/globals_vars_test.go

+		Type:            "helm",
+		Url:             "http://example.com",
+		NamespaceScoped: true,
+		Interval:        "99m",


Again, not sure that's valid. We could even simplify things by just using a drop-down selection in the UX (and validating valid values of 1, 2, ..., 15, 20, 30 etc.

absoludity · 2022-11-07T02:38:34Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go

 			requestCustomizer: func(request *corev1.UpdatePackageRepositoryRequest) *corev1.UpdatePackageRepositoryRequest {
-				request.Interval = "1s"
+				request.Interval = "99m"


Again, maybe use 15m as a valid interval.

beni0888 · 2022-11-07T07:34:56Z

cmd/apprepository-controller/server/controller.go

+func intervalToCron(duration string) (string, error) {
+	if duration == "" {
+		return "", fmt.Errorf("duration cannot be empty")
+	} else {


The else branch here is redundant. IMHO the code would be easier to follow if you get rid of it.

beni0888 · 2022-11-07T07:35:17Z

cmd/apprepository-controller/server/controller.go

+	} else {
+		if d, err := time.ParseDuration(duration); err != nil {
+			return "", err
+		} else {


The same here.

beni0888 · 2022-11-07T07:58:50Z

cmd/apprepository-controller/server/controller_test.go

+		{
+			name:         "good interval, every two hours",
+			interval:     "2h",
+			expectedCron: "*/120 * * * *",


➕ I don't know either if that's even valid, but besides that, I think that if we allow values higher than 60 min, the intervalToCron function should turn this kind of value to * */2 * * *, which is a more readable cron expression.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-11-08T09:56:16Z

I think this should be explained in a bit more detail in the code, or in an issue, that can be referenced in the code. I'd currently have to dig a bit to understand why we can't delete and re-create the CronJob when updating the AppRepository CRD.

After re-reading it, you're right, there's something wrong. The CR controller should be smart enough to detect a change in the definition to recreate the cronjob... the problem is in how we consider two AppRepos are equals. Below the excerpt of the code:

if oldApp.Spec.URL != newApp.Spec.URL || oldApp.Spec.ResyncRequests != newApp.Spec.ResyncRequests {
  controller.enqueueAppRepo(newApp)
}

That's why the "update" branch of the controller wasn't being triggered. Fixing this in the next commit. Thansk for the heads-up!

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-11-08T11:13:41Z

cmd/apprepository-controller/server/controller.go

-			if oldApp.Spec.URL != newApp.Spec.URL || oldApp.Spec.ResyncRequests != newApp.Spec.ResyncRequests {
+			if !reflect.DeepEqual(oldApp.Spec, newApp.Spec) {


@absoludity, do you know if there's any reason for checking the equality manually instead of just using reflect.DeepEqual as I'm proposing herein? I guess that a change in the AppRepo spec should trigger an update in the controller, doesn't it?

Nope. I can't see anything on that AppRepo Spec struct that, if changed, should not trigger an update. Agree 100%.

antgamdia · 2022-11-08T11:14:32Z

cmd/apprepository-controller/server/controller.go

+	if cronMins < 60 {
+		// https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/#cron-schedule-syntax
+		// minute(0-59) hour(0-23) dayOfMonth(1-31) month(1-12) dayOfWeek(0-6)
+		return fmt.Sprintf("*/%v * * * *", cronMins), nil // every cronMins minutes
+	}
+
+	cronHours := math.Ceil(d.Hours()) // round up to nearest hour
+	if cronHours < 24 {
+		return fmt.Sprintf("0 */%v * * *", cronHours), nil // every cronHours hours
+	}
+
+	cronDays := math.Ceil(cronHours / 24) // get the days
+	if cronDays < 32 {
+		return fmt.Sprintf("0 0 */%v * *", cronDays), nil // every cronHoursDays days
+	}
+
+	cronMonths := math.Ceil(cronDays / 31) // get the months
+	if cronMonths < 13 {
+		return fmt.Sprintf("0 0 1 */%v *", cronMonths), nil // every cronHoursMonths months
+	}


New logic for roughly estimating the cron line from a duration

Nice - thanks for the update. Just note, small typo in the inline comments on lines 451 and 456 (s/Hours//)

antgamdia · 2022-11-08T11:16:27Z

cmd/apprepository-controller/server/controller.go

+		if gron.IsValid(apprepo.Spec.Interval) {
+			cronTime = apprepo.Spec.Interval
+		} else {
+			// otherwise, convert it
+			cronTime, err = intervalToCron(apprepo.Spec.Interval)
+		}


Since people might want to specify their own cron lines, this Helm plugin is accepting a valid cron expression and, if invalid, it will try to pasre it as a go duration. This brings more flexibility, but it differs from the API spec a little bit. WDYT?

Hmmm, handy I think. I'd be ok with that (and updating the API spec to match, if we want - still backwards compatible so a nice addition, imo). Nice one!

absoludity · 2022-11-09T06:06:54Z

cmd/apprepository-controller/server/controller.go

-			if oldApp.Spec.URL != newApp.Spec.URL || oldApp.Spec.ResyncRequests != newApp.Spec.ResyncRequests {
+			if !reflect.DeepEqual(oldApp.Spec, newApp.Spec) {


Nope. I can't see anything on that AppRepo Spec struct that, if changed, should not trigger an update. Agree 100%.

absoludity · 2022-11-09T06:10:17Z

cmd/apprepository-controller/server/controller.go

+	if cronMins < 60 {
+		// https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/#cron-schedule-syntax
+		// minute(0-59) hour(0-23) dayOfMonth(1-31) month(1-12) dayOfWeek(0-6)
+		return fmt.Sprintf("*/%v * * * *", cronMins), nil // every cronMins minutes
+	}
+
+	cronHours := math.Ceil(d.Hours()) // round up to nearest hour
+	if cronHours < 24 {
+		return fmt.Sprintf("0 */%v * * *", cronHours), nil // every cronHours hours
+	}
+
+	cronDays := math.Ceil(cronHours / 24) // get the days
+	if cronDays < 32 {
+		return fmt.Sprintf("0 0 */%v * *", cronDays), nil // every cronHoursDays days
+	}
+
+	cronMonths := math.Ceil(cronDays / 31) // get the months
+	if cronMonths < 13 {
+		return fmt.Sprintf("0 0 1 */%v *", cronMonths), nil // every cronHoursMonths months
+	}


Nice - thanks for the update. Just note, small typo in the inline comments on lines 451 and 456 (s/Hours//)

absoludity · 2022-11-09T06:11:43Z

cmd/apprepository-controller/server/controller.go

+		if gron.IsValid(apprepo.Spec.Interval) {
+			cronTime = apprepo.Spec.Interval
+		} else {
+			// otherwise, convert it
+			cronTime, err = intervalToCron(apprepo.Spec.Interval)
+		}


Hmmm, handy I think. I'd be ok with that (and updating the API spec to match, if we want - still backwards compatible so a nice addition, imo). Nice one!

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-11-09T12:27:34Z

cmd/kubeapps-apis/proto/kubeappsapis/core/packages/v1alpha1/repositories.proto

+  // Some plugins may, addionally, support other formats, for instance,
+  // a cron expression.
+  // e.g., "*/10 * * * *" will be equivalent to "10m"
  // Optional. Defaults to 10m if not specified
  string interval = 7;


I've added this note to document the fact that other plugins might support other formats while also supporting the "time+unit" one.

antgamdia · 2022-11-09T16:36:16Z

Something is wrong with the CI, out of the blue, I'm getting a problem with the test in Flux :S
I'd say it is unrelated to this PR, so will re-try in a while.

conflicts

antgamdia added 10 commits October 28, 2022 17:38

Change "Replace" str with the exported str from batchv1

99e1552

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add Interval to the AppRepo spec

0f082cc

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main' into 3661-appRepoCustomInterval

8ae0e0b

Add test case

37cf53d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix typo

f84221b

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add explicit error handling in intervalToCron

9c0b85f

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add interval to the crd openapi model

a1147e3

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix interval generation syntax

b23e055

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Use the apprepo.Interval in the plugin

72a3259

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Use the interval in the repo form even if using helm plugin

030be87

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

vmwclabot added the cla-not-required label Nov 2, 2022

antgamdia added 5 commits November 2, 2022 17:03

Fix linter issue

b9a5265

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Change apprepository.crontab msg

ac5a047

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main' into 3661-appRepoCustomInterval

010f1d8

Use the field in the get repo details. Disable field for helm

2960a2b

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix disable condition

a6eae43

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia marked this pull request as ready for review November 3, 2022 16:44

Merge branch 'main' into 3661-appRepoCustomInterval

95c1775

absoludity approved these changes Nov 7, 2022

View reviewed changes

beni0888 approved these changes Nov 7, 2022

View reviewed changes

antgamdia added 3 commits November 7, 2022 14:13

prevent invalid cron lines

6042c2d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

fix wrong interval examples

04b02f5

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add more replacements with their respective constants

f3344a4

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Trigger AppRepo controller sync on every change

1b65469

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia commented Nov 8, 2022

View reviewed changes

antgamdia mentioned this pull request Nov 8, 2022

Add more helm custom fields #5622

Merged

absoludity approved these changes Nov 9, 2022

View reviewed changes

antgamdia added 2 commits November 9, 2022 13:08

Fix typos

43efb9d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add more interval formats in the api doc

00f9dc0

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia commented Nov 9, 2022

View reviewed changes

absoludity added 2 commits November 18, 2022 09:00

Merge branch 'main' into 3661-appRepoCustomInterval and resolve

0a32eb8

conflicts

Merge branch 'main' into 3661-appRepoCustomInterval and fix more

90b9ab0

conflicts

absoludity merged commit 2671868 into vmware-tanzu:main Nov 17, 2022

antgamdia deleted the 3661-appRepoCustomInterval branch December 18, 2023 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom per-repo interval in AppRepo #5599

Add custom per-repo interval in AppRepo #5599

antgamdia commented Nov 2, 2022 •

edited

Loading

netlify bot commented Nov 2, 2022 •

edited

Loading

absoludity left a comment

absoludity Nov 7, 2022

absoludity Nov 7, 2022

antgamdia Nov 7, 2022

absoludity Nov 7, 2022

beni0888 Nov 7, 2022

antgamdia Nov 7, 2022

absoludity Nov 7, 2022

absoludity Nov 7, 2022

absoludity Nov 7, 2022

beni0888 Nov 7, 2022

beni0888 Nov 7, 2022

beni0888 Nov 7, 2022

antgamdia commented Nov 8, 2022 •

edited

Loading

antgamdia Nov 8, 2022

absoludity Nov 9, 2022

antgamdia Nov 8, 2022

absoludity Nov 9, 2022

antgamdia Nov 8, 2022

absoludity Nov 9, 2022

absoludity Nov 9, 2022

absoludity Nov 9, 2022

absoludity Nov 9, 2022

antgamdia Nov 9, 2022

antgamdia commented Nov 9, 2022

		if oldApp.Spec.URL != newApp.Spec.URL \|\| oldApp.Spec.ResyncRequests != newApp.Spec.ResyncRequests {
		if !reflect.DeepEqual(oldApp.Spec, newApp.Spec) {

Add custom per-repo interval in AppRepo #5599

Add custom per-repo interval in AppRepo #5599

Conversation

antgamdia commented Nov 2, 2022 • edited Loading

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

netlify bot commented Nov 2, 2022 • edited Loading

✅ Deploy Preview for kubeapps-dev ready!

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia commented Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia commented Nov 9, 2022

antgamdia commented Nov 2, 2022 •

edited

Loading

netlify bot commented Nov 2, 2022 •

edited

Loading

antgamdia commented Nov 8, 2022 •

edited

Loading