Add config for packageserver wakeup interval

[tmshort]

No description provided.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (28c6773) 41.99% compared to head (9274a37) 41.99%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #298   +/-   ##
=======================================
  Coverage   41.99%   41.99%           
=======================================
  Files          39       39           
  Lines        3505     3512    +7     
=======================================
+ Hits         1472     1475    +3     
- Misses       1887     1891    +4     
  Partials      146      146           

Files	Coverage Δ
crds/zz_defs.go	35.84% <ø> (ø)
pkg/operators/v1/olmconfig_types.go	100.00% <100.00%> (ø)
pkg/operators/v1/zz_generated.deepcopy.go	0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tmshort]

The one thing missing here is validation; it gets parsed as a duration, but doesn't validate beforehand (i.e. no regex)

[ncdc]

Try // +kubebuilder:validation:Format=duration

[ncdc]

meh: kubernetes/apiextensions-apiserver#56

[awgreene]

Would packageServerPeriodicSyncInterval be more appropriate?

[tmshort]

It's called wakeupInterval in the code... but I really don't care.

[tmshort]

"Periodic" and "Interval" are redundant. So, perhaps just "packageServerSyncInterval"?

[dinhxuanvu]

While this config is on olmconfig api which users choose to do this, I still feel a bit concerned that if they set this duration too longer (either intentionally or accidentally), the catalog data in package server is gonna be stale for a long time. I wonder if there is a way to validate and warn against this. Having said that, this should only impact the consumer of packageserver which is console at the moment. This shouldn’t impact the runtime resolution which is more important.

[tmshort]

Try // +kubebuilder:validation:Format=duration

meh: kubernetes/apiextensions-apiserver#56

I found this, and reduced it (before @dinhxuanvu even said anything) to limit it to hours/minutes/seconds.
openshift/hive#1684

[dinhxuanvu]

Try // +kubebuilder:validation:Format=duration
meh: kubernetes/apiextensions-apiserver#56

I found this, and reduced it (before @dinhxuanvu even said anything) to limit it to hours/minutes/seconds. openshift/hive#1684

I can live with hours restrictions. At least that will allow daily update of catalog content.

[tmshort]

Try // +kubebuilder:validation:Format=duration
meh: kubernetes/apiextensions-apiserver#56

I found this, and reduced it (before @dinhxuanvu even said anything) to limit it to hours/minutes/seconds. openshift/hive#1684

I can live with hours restrictions. At least that will allow daily update of catalog content.

There's no limit to the number before the hours, so it could be e.g. 72h

[dinhxuanvu]

Try // +kubebuilder:validation:Format=duration
meh: kubernetes/apiextensions-apiserver#56

I found this, and reduced it (before @dinhxuanvu even said anything) to limit it to hours/minutes/seconds. openshift/hive#1684

I can live with hours restrictions. At least that will allow daily update of catalog content.

There's no limit to the number before the hours, so it could be e.g. 72h

I’m aware of that but it’s unlikely that they mistakenly add that big of a number though it could still be a typo. That at least reduces the risk of having days of waiting if we allow days or beyond. I would prefer to restrict it to mins only but then that may sound too restrictive. I think this is reasonable compromise.
We can do further by issuing warning if the number exceeds 24 hrs but that is up to you.

[tmshort]

I’m aware of that but it’s unlikely that they mistakenly add that big of a number though it could still be a typo. That at least reduces the risk of having days of waiting if we allow days or beyond. I would prefer to restrict it to mins only but then that may sound too restrictive. I think this is reasonable compromise. We can do further by issuing warning if the number exceeds 24 hrs but that is up to you.

It seemed telcos wanted to increase the period to days, and this is meant for IoT-type devices (which again, they may want to update reasonably quickly).

[dinhxuanvu]

I’m aware of that but it’s unlikely that they mistakenly add that big of a number though it could still be a typo. That at least reduces the risk of having days of waiting if we allow days or beyond. I would prefer to restrict it to mins only but then that may sound too restrictive. I think this is reasonable compromise. We can do further by issuing warning if the number exceeds 24 hrs but that is up to you.

It seemed telcos wanted to increase the period to days, and this is meant for IoT-type devices (which again, they may want to update reasonably quickly).

Given this is for single-node/telco/LP usage, I wonder if their intention is to suppress the packageserver as much as possible because they simply don't use console at all. Deploying via CLI is usually more common then point-and-click console for these types of deployments. If that is the case then frankly I don't care as much about safeguarding this feature given packageserver has nothing to do with runtime catalog update.

[tmshort]

ping for approval/lgtm?

[dinhxuanvu]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, tmshort

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dinhxuanvu]

ping for approval/lgtm?

I approved this but will need a secondary person to lgtm.

[ncdc]

/lgtm

[ncdc]

I approved this but will need a secondary person to lgtm.

@dinhxuanvu why?

[dinhxuanvu]

I approved this but will need a secondary person to lgtm.

@dinhxuanvu why?

Usually we require two separate people to review and thumb up on the PR especially involving an API change at least that's how we worked back in my time. I'm still following that rule. That's why I didn't approve and lgtm at the same time. Plus, I wanted other people to have a chance to block this PR if they didn't agree with me.