Add a bundle deployment reconciler to handle v1alpha2 API #743

varshaprasad96 · 2023-10-16T22:11:16Z

This PR does considerable refactoring to the existing code to introduce a new reconciler to handle v1alpha2 bundle deployment API. This does not remove or make any changes to the existing alpha1 code base. Once we have everything related to alpha2, we can cleanup and remove other controllers and manifests that are not required.

Major changes made:

A new reconciler introduced in: internal/v1alpha2/controllers/bundledeployment/bundledeployment.go
Store interface for creating a local fs for each bundle deployment to store unpacked contents: internal/v1alpha2/store
Unpacker interface and related code for unpacking multiple sources: internal/v1alpha2/source
Validator: internal/v1alpha2/validator
Deployer: internal/v1alpha2/deployer

The reconciler is not plugged into core/main.go. This PR splits #727 to make review process easier. All the features, not implemented here are tracked in there.

varshaprasad96 · 2023-10-16T22:11:32Z

cc: @perdasilva @ncdc

varshaprasad96 · 2023-10-16T22:12:36Z

api/v1alpha1/bundledeployment_types.go

@@ -86,6 +86,9 @@ type BundleDeploymentStatus struct {
 //+kubebuilder:printcolumn:name=Provisioner,type=string,JSONPath=`.spec.provisionerClassName`,priority=1

 // BundleDeployment is the Schema for the bundledeployments API
+// TODO(varsha): Marking this as unserved version for now. Remove this definition during cleanup, when the all the
+// work on alpha2 is done.
+// +kubebuilder:unservedversion


This PR does not wire in the reconciler to the controller yet. Can make this as a storage version until that is done.

If you mark v1alpha1 as unserved is the expectation that conversion from v1alpha1 to v1alpha2 and back exists?

codecov · 2023-10-16T22:16:39Z

Codecov Report

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

Comparison is base (61432e2) 24.67% compared to head (fb0fa79) 23.71%.

❗ Current head fb0fa79 differs from pull request most recent head 1237a3c. Consider uploading reports for the commit 1237a3c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
- Coverage   24.67%   23.71%   -0.96%     
==========================================
  Files          14       22       +8     
  Lines        1143     1923     +780     
==========================================
+ Hits          282      456     +174     
- Misses        807     1373     +566     
- Partials       54       94      +40

Files	Coverage Δ
internal/v1alpha2/validator/validator.go	`81.25% <81.25%> (ø)`
internal/v1alpha2/source/mock_store.go	`23.52% <23.52%> (ø)`
internal/v1alpha2/source/interface.go	`0.00% <0.00%> (ø)`
internal/v1alpha2/store/store.go	`40.00% <40.00%> (ø)`
...ollers/bundledeployment/bundledeployment_status.go	`0.00% <0.00%> (ø)`
internal/v1alpha2/source/image.go	`52.53% <52.53%> (ø)`
...2/controllers/bundledeployment/bundledeployment.go	`0.00% <0.00%> (ø)`
internal/v1alpha2/convert/registryV1.go	`14.01% <14.01%> (ø)`

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

varshaprasad96 · 2023-10-16T22:20:27Z

Please review from the second commit. The first one would be rebased on #713

stevekuznetsov · 2023-10-17T14:54:29Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+
+	controller        crcontroller.Controller
+	dynamicWatchMutex sync.RWMutex
+	dynamicWatchGVKs  map[schema.GroupVersionKind]struct{}


sets.Set[schema.GroupVersionKind]

stevekuznetsov · 2023-10-17T14:54:41Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+//+kubebuilder:rbac:groups=core.rukpak.io,resources=bundledeployments,verbs=list;watch
+//+kubebuilder:rbac:groups=core.rukpak.io,resources=bundledeployments/status,verbs=update;patch
+//+kubebuilder:rbac:groups=core.rukpak.io,resources=bundledeployments/finalizers,verbs=update
+//+kubebuilder:rbac:groups=*,resources=*,verbs=*


I know! But I have no idea on the intention behind this. Copied from

rukpak/internal/controllers/bundledeployment/bundledeployment.go

Line 160 in bcae27f

//+kubebuilder:rbac:groups=*,resources=*,verbs=*

stevekuznetsov · 2023-10-17T14:55:14Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+func (b *bundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	log := log.FromContext(ctx)
+
+	existingBD := &v1alpha2.BundleDeployment{}


nit: rukpackv1alpha2

stevekuznetsov · 2023-10-17T14:55:49Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+
+	// The controller is not updating spec, we only update the status. Hence sending
+	// a status update should be enough.
+	if !equality.Semantic.DeepEqual(existingBD.Status, reconciledBD.Status) {


Can we use applyconfigurations? I imagine the code generators create them already; new code should be written to use them if possible.

stevekuznetsov · 2023-10-17T14:57:29Z

internal/v1alpha2/store/interface.go

+// by the bundle deployment store. These methods facilitate the unpacking and storage
+// of contents from all the sources.
+type Store interface {
+	afero.Fs


I'm surprised you want to inline all the low-level bits from this.

Right now, the sources use limited number of helpers just for creating/removing/verifying contents inside dir. Instead of specifying them separately, inlining of afero.Fs would make it easier and save us from adding more helpers to this interface. I assume we would be using this only in rukpak, that too to access localdir, so expecting an afero.Fs implementation to be passed seemed reasonable.

internal/v1alpha2/store/store.go

stevekuznetsov · 2023-10-17T15:00:39Z

internal/v1alpha2/source/image.go

+		return nil, fmt.Errorf("validation unsuccessful during unpacking %v", err)
+	}
+	// storage path to store contents in local directory.
+	storagePath := filepath.Join(store.GetBundleDeploymentName(), filepath.Clean(source.Destination))


You pass store and source into i.unpack() so you can likely inline this logic there instead of passing it in as a parameter

stevekuznetsov · 2023-10-17T15:01:37Z

internal/v1alpha2/source/image.go

+	op, err := i.ensureUnpackPod(ctx, store.GetBundleDeploymentName(), *source, pod, opts)
+	if err != nil {
+		return nil, err
+	} else if op == controllerutil.OperationResultCreated || op == controllerutil.OperationResultUpdated || pod.DeletionTimestamp != nil {


If i.ensureUnpackPod() did not return Created or Updated, why is a deleting pod a pending unpack?

stevekuznetsov · 2023-10-17T15:04:36Z

internal/v1alpha2/source/image.go

+		if !apierrors.IsInvalid(err) {
+			return controllerutil.OperationResultNone, err
+		}
+		if err := i.Client.Delete(ctx, existingPod); err != nil {


Most of the Pod spec is immutable. What's the meaning of creating a broad apply-config & doing this dance? In what cases is updating the existing pod what you want? Why not delete + create always?

Or is this factored to start with an apply for the happy case where there's nothing to change? I imagine there's a way to compute whether or not a change has occurred on the Bundle object and determine if it's needed to update the pod without a call to the API server

Or is this factored to start with an apply for the happy case where there's nothing to change?

This has been taken from the current alpha1 implementation. But this is the only reason I see.

I imagine there's a way to compute whether or not a change has occurred on the Bundle object and determine if it's needed to update the pod without a call to the API server

This in general needs to done on a higher level, regardless of the image unpacking using a pod. Right now, even though there is no change, the design unpacks during every reconcile (whether image, git or http) which is unnecessary. Probably a caching mechanism, where we compute a hash of the bdName and bundleSource (which contain image digest, git commit etc) to figure out if there has been a change in the bundle content would be a better way to reduce time and resource during reconcile. I had implemented something similar in #727, but wanted to keep it separate for now. If this is implemented, then we need not patch, instead as you mentioned, delete+create as we would be sure of the change.

Whichever gets you iterating faster is fine. I think a more explicit input check makes the rest of those code simpler & easier to test and reason about, but since you're copying it's moot :)

stevekuznetsov · 2023-10-17T15:05:16Z

internal/v1alpha2/source/image.go

+
+	// make sure the passed in pod value is updated with the latest
+	// version of the pod
+	*pod = *updatedPod


Probably best to return the pod instead of mutating in place - is there motivation for the side-effect approach?

stevekuznetsov · 2023-10-17T15:06:18Z

internal/v1alpha2/source/image.go

+	// compare existingPod to newPod and return an appropriate
+	// OperatorResult value.
+	newPod := updatedPod.DeepCopy()
+	unsetNonComparedPodFields(existingPod, newPod)


This is slightly hard to follow. If what you want is to compare pod.Spec, why not just do that?

stevekuznetsov · 2023-10-17T15:29:27Z

internal/v1alpha2/source/image.go

+	if err != nil {
+		return fmt.Errorf("unpack failed: failed to retrieve failed pod logs: %v", err)
+	}
+	_ = i.Client.Delete(ctx, pod)


Likely want to ignore not found and return the error?

stevekuznetsov · 2023-10-17T15:30:36Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+// Options to configure bundleDeploymentReconciler
+type Option func(bd *bundleDeploymentReconciler)
+
+func WithUnpacker(u *source.Unpacker) Option {


When is it valid not to have an unpacker?

Never, which is why this validation: https://github.com/operator-framework/rukpak/pull/743/files#diff-509cbdc8bdbb6d4f857a9b4c4a0205f715c51d4850e57838e3f2141133029c1eR287

Shouldn't we just have this as a required field in the constructor instead of an option?

Shouldn't we just have this as a required field in the constructor instead of an option?

+1. An alternative would be to have a default unpacker that would be changed if this option is specified.

stevekuznetsov · 2023-10-17T15:31:17Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

 		return ctrl.Result{}, err
 	}

+	switch res.State {
+	case source.StateUnpackPending:
+		setUnpackStatusPending(&bundleDeployment.Status.Conditions, fmt.Sprintf("pending unpack"), bundleDeployment.Generation)


Shouldn't we be watching pods with some unpacking label and triggering off of them changing state?

stevekuznetsov · 2023-10-17T15:32:03Z

How much of this is straight copies from v1alpha1? Notably I think @everettraven has direct client unpacking merged, are you expecting to use that too?

varshaprasad96 · 2023-10-17T16:07:16Z

@stevekuznetsov The unpacking and deploy logic is copied from v1alpha1 bits. Implementing direct client unpacking is not a now a part of this right now. The design for this was to allow different implementations of store, unpacker and deployer. Some of this are to be passed per bundle deployment (store), whereas unpacker, validator and deployer are passed while creating the controller. I'll look at your comments and address those.

This commit intends to introduce the new v1alpha2 APIs Signed-off-by: Varsha Prasad Narsing <[email protected]> (cherry picked from commit a0238b9)

This commit adds alpha2 as storage version, and marks alpha1 to be unserved. Signed-off-by: Varsha Prasad Narsing <[email protected]>

This commit: 1. Adds skeleton for controller logic. 2. Adds "Store" interface, implementation for bundledeploymentstore and relevant tests. Signed-off-by: Varsha Prasad Narsing <[email protected]>

This commit adds the default unpackers, with the specific implementation and tests for unpacking from image source. Signed-off-by: Varsha Prasad Narsing <[email protected]>

This commit adds: 1. Logic for validation of helm, registryV1 and plain bundles. 2. Tests for image source and a mock store implementation for use. Signed-off-by: Varsha Prasad Narsing <[email protected]>

This commit introduces: 1. Deployer interface and Helm deployer - to deploy contents using helm 2. Wire deployer logic into reconciler. 3. Add SetupWithManager, so that this can be called from core/main.go in a follow up . Signed-off-by: Varsha Prasad Narsing <[email protected]>

…happy Signed-off-by: Varsha Prasad Narsing <[email protected]>

everettraven · 2023-10-19T15:26:02Z

api/v1alpha2/bundledeployment_types.go

+	// +optional
+	Paused bool `json:"paused,omitempty"`
+
+	// Config is helm spcific configuration to load


Suggested change

// Config is helm spcific configuration to load

// Config is helm specific configuration to load

everettraven · 2023-10-19T15:35:32Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+// Options to configure bundleDeploymentReconciler
+type Option func(bd *bundleDeploymentReconciler)
+
+func WithUnpacker(u *source.Unpacker) Option {


Shouldn't we just have this as a required field in the constructor instead of an option?

+1. An alternative would be to have a default unpacker that would be changed if this option is specified.

everettraven · 2023-10-19T18:12:32Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+	// result can be nil, when there is an error during unpacking. This indicates
+	// that unpacking was failed. Update the status accordingly.
+	if res == nil || err != nil {
+		setUnpackStatusFailing(&bundleDeployment.Status.Conditions, fmt.Sprintf("unpack unsuccessful %v", err), bundleDeployment.Generation)
+		return ctrl.Result{}, err
+	}


Is it possible for res == nil and err == nil at the same time? If so, I'd be cautious of lumping them into the same conditional here. From a ux perspective, if we hit the case where res == nil && err == nil the message in the status is going to be something like unpack unsuccessful <nil> which is a pretty non-descriptive message.

everettraven · 2023-10-19T18:19:12Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+		// Requeing after 5 sec for now since the average time to unpack an registry bundle locally
+		// was around ~4sec.
+		// Warning: This could end up requeing indefinitely, till an error has occurred.
+		return ctrl.Result{RequeueAfter: 5 * time.Second}, nil


Do we need to manually requeue? IIUC we have a watch on Pods because the Unpacker uses them to unpack the bundle images. It looks like the v1alpha2 unpacker only has an image source so it seems like changes to the unpack Pod should trigger a re-reconciliation of the BundleDeployment that owns it making this forced requeue unnecessary

everettraven · 2023-10-19T18:23:30Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+	case source.StateUnpacking:
+		setUnpackStatusPending(&bundleDeployment.Status.Conditions, "unpacking in progress", bundleDeployment.Generation)


IIRC having a returned state of Unpacking from the unpacker used to correlate to updating the status condition Unpacked's reason to Unpacking. Is there a particular reason to remove it and instead use Pending with the message unpacking in progress?

everettraven · 2023-10-19T18:58:43Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment_status.go

+// setUnpgradeStatusFailed sets the installed success to failing as there is an error while patching
+// objects on cluster.
+func setUnpgradeStatusFailed(conditions *[]metav1.Condition, message string, generation int64) {


Suggested change

// setUnpgradeStatusFailed sets the installed success to failing as there is an error while patching

// objects on cluster.

func setUnpgradeStatusFailed(conditions *[]metav1.Condition, message string, generation int64) {

// setUpgradeStatusFailed sets the installed success to failing as there is an error while patching

// objects on cluster.

func setUpgradeStatusFailed(conditions *[]metav1.Condition, message string, generation int64) {

everettraven · 2023-10-19T19:08:23Z

internal/v1alpha2/deployer/helm.go

Is this net new or has this code been moved+modified?

everettraven · 2023-10-19T19:11:18Z

internal/v1alpha2/deployer/interface.go

+}
+
+// Result conveys the progress information about deploying content.
+// TODO: Refactor to use the same result struct for unpacking and deployment.


What prevents us from doing this now?

Because of ResolvedSource field. This api was intended to accept multiple resolved sources for a single BD. So that would need more discussion.

everettraven · 2023-10-19T19:18:14Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+// deployContents calls the registered deployer to apply the bundle contents onto the cluster.
+func (b *bundleDeploymentReconciler) deployContents(ctx context.Context, store store.Store, bd *v1alpha2.BundleDeployment) (*deployer.Result, error) {
+	return b.deployer.Deploy(ctx, store, bd)
+}


This function seems unneccessary. We could just use the b.deployer.Deploy() function directly

everettraven · 2023-10-19T19:24:43Z

internal/v1alpha2/controllers/bundledeployment/bundledeployment.go

+	switch deployResult.State {
+	case deployer.StateIntallFailed:
+		setInstallStatusFailed(&bundleDeployment.Status.Conditions, err.Error(), bundleDeployment.Generation)
+		return ctrl.Result{}, err
+	case deployer.StateUnpgradeFailed:
+		setUnpgradeStatusFailed(&bundleDeployment.Status.Conditions, err.Error(), bundleDeployment.Generation)
+		return ctrl.Result{}, err
+	case deployer.StateReconcileFailed:
+		setReconcileStatusFailed(&bundleDeployment.Status.Conditions, err.Error(), bundleDeployment.Generation)
+		return ctrl.Result{}, err
+	case deployer.StateObjectFetchFailed:
+		setDynamicWatchFailed(&bundleDeployment.Status.Conditions, err.Error(), bundleDeployment.Generation)
+		return ctrl.Result{}, err
+	case deployer.StateDeploySuccessful:
+		setInstallStatusSuccess(&bundleDeployment.Status.Conditions, fmt.Sprintf("installed %s", bundleDeployment.GetName()), bundleDeployment.Generation)
+	default:
+		return ctrl.Result{}, fmt.Errorf("unknown deploy state %q for bundle deployment %s: %v", deployResult.State, bundleDeployment.GetName(), bundleDeployment.Generation)
+	}


Is there a way we could make deployContents return the condition that should be set?

ncdc · 2023-10-19T19:33:55Z

Have discussed with @varshaprasad96 @gallettilance @joelanford - we are going to pause on this for the moment.

During the pause, we are going to investigate using https://carvel.dev/kapp-controller/ an alternative to continuing to restructure the rukpak APIs. kapp-controller does almost everything rukpak currently does (it doesn't yet support registry+v1 bundles) and it will save us a lot of time if we can switch to using it.

Closing this for now. We can reopen depending on the results of our research.

grokspawn · 2023-10-20T18:11:01Z

Have discussed with @varshaprasad96 @gallettilance @joelanford - we are going to pause on this for the moment.

During the pause, we are going to investigate using https://carvel.dev/kapp-controller/ an alternative to continuing to restructure the rukpak APIs. kapp-controller does almost everything rukpak currently does (it doesn't yet support registry+v1 bundles) and it will save us a lot of time if we can switch to using it.

Closing this for now. We can reopen depending on the results of our research.

Haven't seen this in WG discussion. Have I missed a thread?

ncdc · 2023-10-20T18:19:55Z

No missed threads. This is something I've been talking about with @joelanford and a few other folks.

varshaprasad96 requested a review from a team as a code owner October 16, 2023 22:11

varshaprasad96 commented Oct 16, 2023

View reviewed changes

varshaprasad96 changed the title ~~Add a bundle deployment reconciler to handle v1alpha1 API~~ Add a bundle deployment reconciler to handle v1alpha2 API Oct 16, 2023

stevekuznetsov reviewed Oct 17, 2023

View reviewed changes

internal/v1alpha2/store/store.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Oct 17, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023

varshaprasad96 added 7 commits October 18, 2023 17:15

[v1aplha2] Introduce v1alpha2 APIs

050cb01

This commit intends to introduce the new v1alpha2 APIs Signed-off-by: Varsha Prasad Narsing <[email protected]> (cherry picked from commit a0238b9)

[API] Add alpha2 API as storage

82b1bfa

This commit adds alpha2 as storage version, and marks alpha1 to be unserved. Signed-off-by: Varsha Prasad Narsing <[email protected]>

[controller] Add skeleton for controller logic

9c30133

This commit: 1. Adds skeleton for controller logic. 2. Adds "Store" interface, implementation for bundledeploymentstore and relevant tests. Signed-off-by: Varsha Prasad Narsing <[email protected]>

[unpack] Add logic and tests for unpacking image source

59185e8

This commit adds the default unpackers, with the specific implementation and tests for unpacking from image source. Signed-off-by: Varsha Prasad Narsing <[email protected]>

[validate] Add logic for validation and test cases

9b82638

This commit adds: 1. Logic for validation of helm, registryV1 and plain bundles. 2. Tests for image source and a mock store implementation for use. Signed-off-by: Varsha Prasad Narsing <[email protected]>

[fix]: Add afero.Fs inline to bundledeployment store and make linter …

1237a3c

…happy Signed-off-by: Varsha Prasad Narsing <[email protected]>

varshaprasad96 force-pushed the refactor/v1alpha2-controller-image-source branch from aa3dfa5 to 1237a3c Compare October 18, 2023 21:26

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023

everettraven reviewed Oct 19, 2023

View reviewed changes

ncdc closed this Oct 19, 2023

	// Config is helm spcific configuration to load
	// Config is helm specific configuration to load

		case source.StateUnpacking:
		setUnpackStatusPending(&bundleDeployment.Status.Conditions, "unpacking in progress", bundleDeployment.Generation)

Add a bundle deployment reconciler to handle v1alpha2 API #743

Add a bundle deployment reconciler to handle v1alpha2 API #743

Conversation

varshaprasad96 commented Oct 16, 2023 • edited Loading

varshaprasad96 commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

varshaprasad96 commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Oct 17, 2023

varshaprasad96 commented Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Oct 19, 2023 • edited Loading

grokspawn commented Oct 20, 2023

ncdc commented Oct 20, 2023

varshaprasad96 commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

varshaprasad96 Oct 17, 2023 •

edited

Loading

varshaprasad96 commented Oct 17, 2023 •

edited

Loading

ncdc commented Oct 19, 2023 •

edited

Loading