Skip to content

[run bundle] Install plan generation and approval for subscription #3838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rashmigottipati merged 1 commit into from
Sep 2, 2020
Merged

[run bundle] Install plan generation and approval for subscription #3838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/cmd/operator-sdk/run/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Currently only the package manifests format is supported via the 'packagemanifes

cmd.AddCommand(
// TODO(joelanford): enable bundle command when implementation is complete
//bundle.NewCmd(),
// bundle.NewCmd(cfg),
packagemanifests.NewCmd(cfg),
)

Expand Down
2 changes: 1 addition & 1 deletion internal/olm/operator/bundle/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (i *Install) setup(ctx context.Context) error {
i.OperatorInstaller.CatalogSourceName = fmt.Sprintf("%s-catalog", i.OperatorInstaller.PackageName)
i.OperatorInstaller.StartingCSV = csv.Name
i.OperatorInstaller.Channel = strings.Split(labels["operators.operatorframework.io.bundle.channels.v1"], ",")[0]

i.IndexImageCatalogCreator.BundleImage = i.BundleImage
i.IndexImageCatalogCreator.PackageName = i.OperatorInstaller.PackageName
i.IndexImageCatalogCreator.InjectBundles = []string{i.BundleImage}
i.IndexImageCatalogCreator.InjectBundleMode = "replaces"
Expand Down
5 changes: 0 additions & 5 deletions internal/olm/operator/registry/catalog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,3 @@ import (
type CatalogCreator interface {
CreateCatalog(ctx context.Context, name string) (*v1alpha1.CatalogSource, error)
}

// TODO: modify this as necessary.
type InstallPlanApprover interface {
Approve(ctx context.Context, name string) error
}
11 changes: 11 additions & 0 deletions internal/olm/operator/registry/olm_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ func withPackageChannel(pkgName, channelName, startingCSV string) func(*v1alpha1
}
}

// withInstallPlanApproval sets the Subscription's install plan approval field
// to manual
func withInstallPlanApproval(approval v1alpha1.Approval) func(*v1alpha1.Subscription) {
return func(sub *v1alpha1.Subscription) {
if sub.Spec == nil {
sub.Spec = &v1alpha1.SubscriptionSpec{}
}
sub.Spec.InstallPlanApproval = approval
}
}

// newSubscription creates a new Subscription for a CSV with a name derived
// from csvName, the CSV's objectmeta.name, in namespace. opts will be applied
// to the Subscription object.
Expand Down
77 changes: 67 additions & 10 deletions internal/olm/operator/registry/operator_installer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
log "github.com/sirupsen/logrus"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"

olmclient "github.com/operator-framework/operator-sdk/internal/olm/client"
Expand Down Expand Up @@ -73,16 +74,20 @@ func (o OperatorInstaller) InstallOperator(ctx context.Context) (*v1alpha1.Clust
return nil, err
}

var subscription *v1alpha1.Subscription
// Create Subscription
if err = o.createSubscription(ctx, cs); err != nil {
if subscription, err = o.createSubscription(ctx, cs); err != nil {
return nil, err
}

// Approve Install Plan (if necessary)
if approver, ok := o.CatalogCreator.(InstallPlanApprover); ok {
if err = approver.Approve(ctx, o.PackageName); err != nil {
return nil, err
}
// Wait for the Install Plan to be generated
if err = o.waitForInstallPlan(ctx, subscription); err != nil {
return nil, err
}

// Approve Install Plan for the subscription
if err = o.approveInstallPlan(ctx, subscription); err != nil {
return nil, err
}

// Wait for successfully installed CSV
Expand Down Expand Up @@ -191,15 +196,17 @@ func (o OperatorInstaller) getOperatorGroup(ctx context.Context) (*v1.OperatorGr
return &ogList.Items[0], true, nil
}

func (o OperatorInstaller) createSubscription(ctx context.Context, cs *v1alpha1.CatalogSource) error {
func (o OperatorInstaller) createSubscription(ctx context.Context, cs *v1alpha1.CatalogSource) (*v1alpha1.Subscription, error) {
sub := newSubscription(o.StartingCSV, o.cfg.Namespace,
withPackageChannel(o.PackageName, o.Channel, o.StartingCSV),
withCatalogSource(cs.GetName(), o.cfg.Namespace))
withCatalogSource(cs.GetName(), o.cfg.Namespace),
withInstallPlanApproval(v1alpha1.ApprovalManual))
Copy link
Copy Markdown
Member

@estroz estroz Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if CI will pass because InstallPlans created by run packagemanifests will not be approved. IIRC we discussed that automatic approval is a bug in run packagemanifests since a version is specified but that version might not be the channel head specified in a Subscription, and setting approval to manual is correct for both the index and configmap CatalogCreator's; we would need to approve all Subscriptions manually with this change.

Copy link
Copy Markdown
Member

@joelanford joelanford Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the catalog source is created, the logic should be identical between run packagemanifests and run bundle.

We shouldn't have different implementations of the InstallPlanApprover interface (and we probably don't need that interface at all).


log.Info("Creating Subscription")
if err := o.cfg.Client.Create(ctx, sub); err != nil {
return fmt.Errorf("error creating OperatorGroup: %w", err)
return nil, fmt.Errorf("error creating subscription: %w", err)
}
return nil
return sub, nil
}

func (o OperatorInstaller) getInstalledCSV(ctx context.Context) (*v1alpha1.ClusterServiceVersion, error) {
Expand All @@ -226,3 +233,53 @@ func (o OperatorInstaller) getInstalledCSV(ctx context.Context) (*v1alpha1.Clust
}
return csv, nil
}

// approveInstallPlan approves the install plan for a subscription, which will
// generate a CSV
func (o OperatorInstaller) approveInstallPlan(ctx context.Context, sub *v1alpha1.Subscription) error {
ip := v1alpha1.InstallPlan{}

ipKey := types.NamespacedName{
Name: sub.Status.InstallPlanRef.Name,
Namespace: sub.Status.InstallPlanRef.Namespace,
}

if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
if err := o.cfg.Client.Get(ctx, ipKey, &ip); err != nil {
return fmt.Errorf("error in getting install plan: %v", err)
}
// approve the install plan by setting Approved to true
ip.Spec.Approved = true
if err := o.cfg.Client.Update(ctx, &ip); err != nil {
return fmt.Errorf("error in approving install plan: %v", err)
}
return nil
}); err != nil {
return err
}

return nil
}

// waitForInstallPlan verifies if an Install Plan exists through subscription status
func (o OperatorInstaller) waitForInstallPlan(ctx context.Context, sub *v1alpha1.Subscription) error {
subKey := types.NamespacedName{
Namespace: sub.GetNamespace(),
Name: sub.GetName(),
}

ipCheck := wait.ConditionFunc(func() (done bool, err error) {
if err := o.cfg.Client.Get(ctx, subKey, sub); err != nil {
return false, err
}
if sub.Status.InstallPlanRef != nil {
return true, nil
}
return false, nil
})

if err := wait.PollImmediateUntil(200*time.Millisecond, ipCheck, ctx.Done()); err != nil {
return fmt.Errorf("install plan is not available for the subscription %s: %v", sub.Name, err)
}
return nil
}
Copy link
Copy Markdown
Member

@joelanford joelanford Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: In one of these install plan functions, do we need to do a sanity check and ensure the install plan is for the CSV that we created in the subscription.

In the case of an upgrade being available, there can be multiple install plans and I think the subscription may reference the next available.

It may not be strictly necessary based in this case since we would need to approve the initial install plan to do the initial install and let OLM get to the point of doing an upgrade, BUT if it's a simple extra sanity check, it may be a good idea to go ahead and add.

Copy link
Copy Markdown
Member Author

@rashmigottipati rashmigottipati Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I guess we can add the below check:

if ip.Status.Plan != nil {
		for _, installPlan := range ip.Status.Plan {
			if installPlan.Resource.Kind == "ClusterServiceVersion" {
				if installPlan.Resolving != sub.Status.CurrentCSV {
					return fmt.Errorf("install plan CSV doesn't match the CSV in the subscription")
				}
			}
		}
	}

I will do this in a follow-up as this is non-blocking.