diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index 4bbe19d..b6d78e3 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -52,7 +52,7 @@ jobs: - name: Build Generated Code run: make generate - name: Generated Code Checked In - run: "[[ -z $(git status --porcelain) ]]" + run: "STAT=$(git status --porcelain); echo $STAT; [[ -z ${STAT} ]]" - name: Unit Test run: make test-unit - name: Archive code coverage results diff --git a/.golangci.yaml b/.golangci.yaml index ed1e5ac..d1c17a6 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -21,6 +21,8 @@ linters: - goconst - perfsprint - mnd + - exportloopref + - execinquery linters-settings: gci: sections: diff --git a/charts/application/crds/application.unikorn-cloud.org_applicationsets.yaml b/charts/application/crds/application.unikorn-cloud.org_applicationsets.yaml index 8494fc4..de207e9 100644 --- a/charts/application/crds/application.unikorn-cloud.org_applicationsets.yaml +++ b/charts/application/crds/application.unikorn-cloud.org_applicationsets.yaml @@ -26,7 +26,12 @@ spec: name: v1alpha1 schema: openAPIV3Schema: - description: ApplicationSet defines a Helm application. + description: |- + ApplicationSet defines a set of applications. + It works like a normal package manager, installing a package will automatically + install any dependencies and recommended packages. Removeing a package will also + remove any dependencies and recoomended packages unless they are kept alive by + another package in the set. properties: apiVersion: description: |- @@ -47,6 +52,44 @@ spec: type: object spec: properties: + applications: + description: Applications is a list of user requested applications + to install. + items: + properties: + application: + description: Application is a reference to the typed application. + properties: + apiGroup: + description: |- + APIGroup is the group for the resource being referenced. + If APIGroup is not specified, the specified Kind must be in the core API group. + For any other third-party types, APIGroup is required. + type: string + kind: + description: Kind is the type of resource being referenced + type: string + name: + description: Name is the name of resource being referenced + type: string + namespace: + description: |- + Namespace is the namespace of resource being referenced + Note that when a namespace is specified, a gateway.networking.k8s.io/ReferenceGrant object is required in the referent namespace to allow that namespace's owner to accept the reference. See the ReferenceGrant documentation for details. + (Alpha) This field requires the CrossNamespaceVolumeDataSource feature gate to be enabled. + type: string + required: + - kind + - name + type: object + version: + description: Version is the version of the application. + pattern: ^v?[0-9]+(\.[0-9]+)?(\.[0-9]+)?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?$ + type: string + required: + - application + type: object + type: array pause: description: Pause, if true, will inhibit reconciliation. type: boolean diff --git a/pkg/provisioners/managers/application/provisioner.go b/pkg/provisioners/managers/application/provisioner.go index 5e72966..f03bbde 100644 --- a/pkg/provisioners/managers/application/provisioner.go +++ b/pkg/provisioners/managers/application/provisioner.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "slices" "github.com/spf13/pflag" sat "github.com/spjmurray/go-sat" @@ -234,6 +233,7 @@ type solverVisitor struct { model *sat.Model[AppVersion] } +//nolint:cyclop func (v *solverVisitor) Visit(id string, enqueue func(string)) error { application, ok := v.applications[id] if !ok { @@ -253,7 +253,8 @@ func (v *solverVisitor) Visit(id string, enqueue func(string)) error { // dependent applications are also installed, but constrained to the allowed // set for this application. This also has the property that if a version has // no satisfiable deps e.g. then it will add a unary clause that prevents the - // version from being used. + // version from being used. If a version is installed, it also implies any + // recommended packages should be installed too. for version := range application.Versions() { av := AppVersion{application.Name, version.Version} @@ -265,7 +266,7 @@ func (v *solverVisitor) Visit(id string, enqueue func(string)) error { depVersions := make([]AppVersion, 0, len(dependantApplication.Spec.Versions)) - for _, depVersion := range slices.Backward(slices.Collect(dependantApplication.Versions())) { + for depVersion := range dependantApplication.Versions() { if dependency.Constraints == nil || dependency.Constraints.Check(&depVersion.Version) { depVersions = append(depVersions, AppVersion{dependency.Name, depVersion.Version}) } @@ -275,6 +276,23 @@ func (v *solverVisitor) Visit(id string, enqueue func(string)) error { enqueue(dependency.Name) } + + for _, recommendation := range version.Recommends { + recommendedApplication, ok := v.applications[recommendation.Name] + if !ok { + return fmt.Errorf("%w: requested application %s not in catalog", ErrResourceDependency, recommendation.Name) + } + + recVersions := make([]AppVersion, 0, len(recommendedApplication.Spec.Versions)) + + for recVersion := range recommendedApplication.Versions() { + recVersions = append(recVersions, AppVersion{recommendation.Name, recVersion.Version}) + } + + v.model.ImpliesAtLeastOneOf(av, recVersions...) + + enqueue(recommendation.Name) + } } return nil @@ -322,19 +340,10 @@ func SolveApplicationSet(ctx context.Context, client client.Client, namespace st continue } - // Otherise we must install at least one version. - // NOTE: we cheat a bit here, when making a choice the solver will pick - // the first undefined variable and set it to true, so we implicitly - // choose the most recent version by adding them in a descending order. - versions := slices.Collect(application.Versions()) - if len(versions) == 0 { - return nil, fmt.Errorf("%w: requested application %s has no versions", ErrResourceDependency, application.Name) - } - - l := make([]AppVersion, len(versions)) + l := make([]AppVersion, 0, len(application.Spec.Versions)) - for i, version := range slices.Backward(versions) { - l[i] = AppVersion{application.Name, version.Version} + for version := range application.Versions() { + l = append(l, AppVersion{application.Name, version.Version}) } model.AtLeastOneOf(l...) diff --git a/pkg/provisioners/managers/application/provisioner_test.go b/pkg/provisioners/managers/application/provisioner_test.go index c582e45..010d7ea 100644 --- a/pkg/provisioners/managers/application/provisioner_test.go +++ b/pkg/provisioners/managers/application/provisioner_test.go @@ -86,15 +86,12 @@ type applicationBuilder struct { versions []*unikornv1core.SemanticVersion // dependencies defines a per-version list of package dependencies. dependencies map[string][]unikornv1core.HelmApplicationDependency - // recommendations defines a per-version list of package recommendations. - recommendations map[string][]unikornv1core.HelmApplicationRecommendation } // newApplicationBuilder createa a new application builder. func newApplicationBuilder() *applicationBuilder { return &applicationBuilder{ - dependencies: map[string][]unikornv1core.HelmApplicationDependency{}, - recommendations: map[string][]unikornv1core.HelmApplicationRecommendation{}, + dependencies: map[string][]unikornv1core.HelmApplicationDependency{}, } } @@ -116,17 +113,6 @@ func (b *applicationBuilder) withDependency(id string, constraints *unikornv1cor return b } -// withRecommendation adds a reccomendation to the current version. -// -//nolint:unused -func (b *applicationBuilder) withRecommendation(id string) *applicationBuilder { - b.recommendations[b.currVersion] = append(b.recommendations[b.currVersion], unikornv1core.HelmApplicationRecommendation{ - Name: id, - }) - - return b -} - // get builds and returns the application resource. func (b *applicationBuilder) get() *unikornv1core.HelmApplication { app := &unikornv1core.HelmApplication{ @@ -148,10 +134,6 @@ func (b *applicationBuilder) get() *unikornv1core.HelmApplication { v.Dependencies = t } - if t, ok := b.recommendations[version.Original()]; ok { - v.Recommends = t - } - app.Spec.Versions[i] = v } @@ -311,6 +293,8 @@ func TestProvisionSingleWithConflictingTransitveDependency(t *testing.T) { require.NoError(t, err) } +// TestProvisionSingleWithChoice makes sure where multiple choices are available +// and the desirable outcome has a conflict, we satisfy the problem. func TestProvisionSingleWithChoice(t *testing.T) { t.Parallel() @@ -348,11 +332,72 @@ func TestProvisionSingleWithChoice(t *testing.T) { order, err := application.Schedule(context.Background(), client, namespace, solution) require.NoError(t, err) + // TODO: the order of the middle two doesn't really matter as they can be done in + // parallel, but it's non-deterministic, so we need a better way of checking this. + require.Len(t, order, 4) + require.Equal(t, application.NewAppVersion(dep.Name, getSemver(t, "1.0.0")), order[0]) + require.Equal(t, application.NewAppVersion(app.Name, getSemver(t, "1.0.0")), order[3]) +} + +// TestProvisionSingleWithChoiceAndConditionalDependency checks for the "phanom package" +// problem, where a dependency occurs on a specific version. The selection heuristic should +// not install it if it's not explicitly depended upon. +func TestProvisionSingleWithChoiceAndConditionalDependency(t *testing.T) { + t.Parallel() + + dep := newApplicationBuilder().withVersion(getSemver(t, "1.0.0")).get() + app := newApplicationBuilder(). + withVersion(getSemver(t, "1.0.0")). + withDependency(dep.Name, getConstraints(t, "=1.0.0")). + withVersion(getSemver(t, "2.0.0")). + get() + + applicationset := newApplicationSet().withApplication(app.Name, nil).get() + + client := fake.NewClientBuilder().WithScheme(scheme(t)).WithObjects(app, dep).Build() + solution, err := application.SolveApplicationSet(context.Background(), client, namespace, applicationset) + require.NoError(t, err) + + order, err := application.Schedule(context.Background(), client, namespace, solution) + require.NoError(t, err) + + expected := []application.AppVersion{ + application.NewAppVersion(app.Name, getSemver(t, "2.0.0")), + } + + require.Equal(t, expected, order) +} + +// TestProvisionSingleWithRecommendation tests recommendations are correctly picked up +// and applied. +func TestProvisionSingleWithRecommendation(t *testing.T) { + t.Parallel() + + app := newApplicationBuilder().withVersion(getSemver(t, "1.0.0")).get() + + rec := newApplicationBuilder(). + withVersion(getSemver(t, "1.0.0")). + withDependency(app.Name, getConstraints(t, "=1.0.0")). + get() + + app.Spec.Versions[0].Recommends = []unikornv1core.HelmApplicationRecommendation{ + { + Name: rec.Name, + }, + } + + applicationset := newApplicationSet().withApplication(app.Name, nil).get() + + client := fake.NewClientBuilder().WithScheme(scheme(t)).WithObjects(app, rec).Build() + solution, err := application.SolveApplicationSet(context.Background(), client, namespace, applicationset) + require.NoError(t, err) + + order, err := application.Schedule(context.Background(), client, namespace, solution) + require.NoError(t, err) + expected := []application.AppVersion{ - application.NewAppVersion(dep.Name, getSemver(t, "1.0.0")), - application.NewAppVersion(idep1.Name, getSemver(t, "1.0.0")), - application.NewAppVersion(idep2.Name, getSemver(t, "1.0.0")), application.NewAppVersion(app.Name, getSemver(t, "1.0.0")), + application.NewAppVersion(rec.Name, getSemver(t, "1.0.0")), } require.Equal(t, expected, order)