Package.Spec.SyncPeriod should be respected when PackageInstall.Spec.SyncPeriod is nil

[jessehu]

What steps did you take:
When applying a PackageInstall CR with Spec.SyncPeriod == nil.

What happened:
The generated App.Spec.SyncPeriod is 10min.

What did you expect:
Package.Spec.Template.Spec.SyncPeriod (if not nil) should be respected when PackageInstall.Spec.SyncPeriod is nil.
Because each Package knows its proper SyncPeriod and deploy.kapp.RawOptions.'--wait-timeout=30s', unless PackageInstall CR explicitly sets the SyncPeriod.

Anything else you would like to add:
Here is the code causing the issue: https://github.com/vmware-tanzu/carvel-kapp-controller/blob/v0.36.1/pkg/packageinstall/app.go#L46-L52

Environment:

• kapp Controller version: v0.36.1 and develop branch

---

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.

[cppforlife]

@jessehu this was done intentionally since this is a type of configuration that is controlled by the package consumer, not authors. would you mind elaborating why it would be useful for package author to "pitch in" with their value?

[jessehu]

Thanks @cppforlife. My point is Package.Spec.Template.Spec.SyncPeriod (if not nil, defined by the package auther) should be used when PackageInstall.Spec.SyncPeriod is nil.

	desiredApp.Name = pkgInstall.Name
	desiredApp.Namespace = pkgInstall.Namespace
	desiredApp.Spec = *pkgVersion.Spec.Template.Spec
	desiredApp.Spec.ServiceAccountName = pkgInstall.Spec.ServiceAccountName
	if pkgInstall.Spec.SyncPeriod == nil {
		desiredApp.Spec.SyncPeriod = &metav1.Duration{Duration: time.Minute * 10}
	} else {
		desiredApp.Spec.SyncPeriod = pkgInstall.Spec.SyncPeriod
	}

The Package author knows the proper deployment options, such as SyncPeriod and deploy.kapp.RawOptions.'--wait-timeout=30s', but can be override by PackageInstall. If we think SyncPeriod should only be controlled by PackageInstall (the package consumer), should PackageInstall provides a default deploy.kapp.RawOptions as well?

[cppforlife]

The Package author knows the proper deployment options

syncPeriod of a package is not something that should "affect" the package itself. ie it should not matter at all if consumer decides to run reconciliation every 30s or every 10h. authors should have no opinion on how frequently consumers decide to reconcile their system.

should PackageInstall provides a default deploy.kapp.RawOptions as well?

that's an interesting question because there are definitely cases where wait timeout is more specific to package content, but there are also cases where some consumers may have particular opinion on the waiting logic. i think it would be good to see more requests around wait timeout from consumers to see why they are trying to configure it.

[jessehu]

I understand your point. Shall we add a hint on https://carvel.dev/kapp-controller/docs/v0.36.1/packaging/#package to explicitly describe only fetch/template/deploy fields from App CR should be specified in Package.Spec.Template.Spec? That doc currently says:

# App template used to create the underlying App CR.
# See 'App CR Spec' docs for more info
template:
  spec:
    fetch:
    - imgpkgBundle:
        image: registry.tkg.vmware.run/tkg-fluent-bit@sha256:...
    template:
    - ytt:
        paths:
        - config/
    - kbld:
        paths:
        # - must be quoted when included with paths
        - .imgpkg/images.yml
        - "-"
    deploy:
    - kapp: {}

[jessehu]

just recall a possible use case that the package author might want to set Package.Spec.Template.Spec.SyncPeriod of package A to a shorter time as this package A depends on other packages, and all packages are reconciled in parallel. As a result, package A can be reconciled again in shorter period and set its status to ReconcileSucceeded.

and I would prefer if pkgInstall.Spec.SyncPeriod == nil, it means the pacakge consumer has no option/idea on App.SyncPeriod, so in this case if Package.Spec.Template.Spec.SyncPeriod is set, why not use it?