carvel-dev / kapp-controller

Continuous delivery and package management for Kubernetes.
https://carvel.dev/kapp-controller
Apache License 2.0
268 stars 103 forks source link

consider global kapp-controller flag to raise default sync period #373

Closed cppforlife closed 2 years ago

cppforlife commented 2 years ago

Describe the problem/challenge you have By default sync period is 30s on App CRs. It might be useful for an administrator to set higher default syncPeriod.

Describe the solution you'd like


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible" 👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

danielhelfand commented 2 years ago

Think this would be a good addition. For anyone picking up this issue, a good place to start might be looking at the flags for the controller here.

joe-kimmel-vmw commented 2 years ago

@danielhelfand @cppforlife the suggestion above to look at the flags is helpful but i think not the full story.

The other end of the change imo is that this hardcoded30 wants to be read from some global-state store.

Off the top of my head I can't think of prior art for something like this, though maybe I'm forgetting something. Where could we keep information that we can access from the ReconcileTimer (or maybe via its .app)? ... e.g. could we use os.SetEnv to write it at initialization time and then look it up when we're in the reconcile timer? or could we add it to the kcconfig, and if so how is that available from the ReconcileTimer?

joe-kimmel-vmw commented 2 years ago

TODO: reach out to users (proactively reach out to internal users, TKG, other users welcome to comment on this ticket). to see the personas of who is setting the config: https://github.com/vmware-tanzu/carvel-kapp-controller/blob/develop/pkg/config/config.go#L38 vs. the deployment flags, to see if there's a more or less reasonable place to put these additional knobs.

aaronshurley commented 2 years ago

I had a brief chat with @shyaamsn about this and he expressed a preference for the ability to set these properties via the config. The reasons being that it would align with how they currently configure things and it would be easier to track changes.

aaronshurley commented 2 years ago

To consolidate some updates on this issue:

cppforlife commented 2 years ago

closing for now. will reopen if we need to provide more configurability to admins.