Wait until CM is created when installing a carvel pkg #4139

antgamdia · 2022-01-20T18:28:34Z

Description of the change

After merging #4068, I've noticed there was an error every time you installed a package. This is because right after a successful install, you get redirected to the details view, meaning that every expected resource is supposed to be available.

We had that problem in the past (we needed the App CR to be created, something that happens a few seconds after creating the PackageInstall). However, now we are watching the generated resources, we need to query a ConfigMap to get the App ID (something generated by kapp-ctrl ); therefore, we need to wait until the CM is created. The thorny thing here is it takes up to 30s.

This PR just follows the similar approach we did for querying the existence of anApp, but I'm open to any other suggestions you may have.

Benefits

No ugly error when the user got redirected to the installed app view.

Possible drawbacks

The installation process will take longer (meaning that the user will see the spinner for 10-30 seconds).

Applicable issues

related Remaining work for running Kubeapps with Carvel #3849

Additional information

N/A

absoludity · 2022-01-24T23:10:13Z

Description of the change

After merging #4068, I've noticed there was an error every time you installed a package. This is because right after a successful install, you get redirected to the details view, meaning that every expected resource is supposed to be available.

We had that problem in the past (we needed the App CR to be created, something that happens a few seconds after creating the PackageInstall). However, now we are watching the generated resources, we need to query a ConfigMap to get the App ID (something generated by kapp-ctrl ); therefore, we need to wait until the CM is created. The thorny thing here is it takes up to 30s.

This PR just follows the similar approach we did for querying the existence of anApp, but I'm open to any other suggestions you may have.

Why not return immediately from the create request, browser redirects to details displaying basic info, but request for all resource (refs) takes 30s? Wouldn't that be a better UX?

Anyway, +1. Thanks!

antgamdia · 2022-01-25T12:51:29Z

Why not return immediately from the create request, browser redirects to details displaying basic info, but request for all resource (refs) takes 30s? Wouldn't that be a better UX?

It is indeed a better UX, I agree. We opted for using the kapp-ctrl code for fetching the installed resources, so, at the moment, we are not interacting with any ConfigMap there (as opposed to #4062), so I decided to keep it that way.

I wasn't sure whether we want to define an installed app in kubeapps as "something fully loaded and ready to query", or we might want to relax it and accept that kind of delay.

Considering the current status of the dashboard regarding the installed resources (that is, no progress indication of the fetching status whatsoever), I'm not 100% sure about what the best UX is.

Scenarios I see (sorted by required effort):

Assume the active wait in the install action (this PR)
- Pros: every time a user goes to the installed app view, it will be consistent (no first-time delays)
- Cons: long wait [but similar/comparable to install a helm release]
Move the active wait to the fetch resources
- Pros: users will get redirected to the installed view sooner, though it only will have some very basic info
- Cons: since there is no progress update, users will notice some elements are popping up randomly. But, before they notice, they should remain at least 30s hanging in a very basic view.
Idem as 2., but also refactor the app view to show some progress bar, spinner or something that let users know that there is something in background being loaded.
- Pros: same as 2.
- Cons: none from a UX POV, but some tech questions arise (is it an active polling in the UI? is it just another message from the resources API the UI should listen to?)

I'd personally go with 1, but would create an issue for moving towards 3 when possible.

WDYT?

absoludity · 2022-01-26T20:29:55Z

Why not return immediately from the create request, browser redirects to details displaying basic info, but request for all resource (refs) takes 30s? Wouldn't that be a better UX?

It is indeed a better UX, I agree. We opted for using the kapp-ctrl code for fetching the installed resources, so, at the moment, we are not interacting with any ConfigMap there (as opposed to #4062), so I decided to keep it that way.

I wasn't sure whether we want to define an installed app in kubeapps as "something fully loaded and ready to query", or we might want to relax it and accept that kind of delay.

I'm confused here because the CreateInstalledPackageResponse contains an installed package reference, and even fetching an installed package still doesn't include returning the resources (or even the resource refs, by design). So fetching the installed app can be queried right away, whereas the delay here will be when fetching the related resources right? (I think that's what you've said here multiple times, but was just confused by the above statement).

Considering the current status of the dashboard regarding the installed resources (that is, no progress indication of the fetching status whatsoever), I'm not 100% sure about what the best UX is.

Scenarios I see (sorted by required effort):

1. Assume the active wait in the install action (this PR)
   
   * Pros: every time a user goes to the installed app view, it will be consistent (no first-time delays)
   * Cons: long wait [but similar/comparable to install a helm release]

2. Move the active wait to the fetch resources
   
   * Pros: users will get redirected to the installed view sooner, though it only will have some very basic info
   * Cons: since there is no progress update, users will notice some elements are popping up randomly. But, before they notice, they should remain at least 30s hanging in a very basic view.

3. Idem as 2., but also refactor the app view to show some progress bar, spinner or something that let users know that there is something in background being loaded.
   
   * Pros: same as 2.
   * Cons: none from a UX POV, but some tech questions arise (is it an active polling in the UI? is it just another message from the resources API the UI should listen to?)

I'd personally go with 1, but would create an issue for moving towards 3 when possible.

WDYT?

Up to you. I would have thought that going with (2) initially would leave us one step closer to (3) without setting a precedent of tech-debt (stalling the createinstalledpackage) that may be followed in other plugins. But you're much closer to the details here, so I'll trust your judgement over mine :)

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-02-02T20:35:23Z

I'm confused here because the CreateInstalledPackageResponse contains an installed package reference, and even fetching an installed package still doesn't include returning the resources (or even the resource refs, by design). So fetching the installed app can be queried right away, whereas the delay here will be when fetching the related resources right? (I think that's what you've said here multiple times, but was just confused by the above statement).

Well, not exactly. There are actually two required waits before we can provide a "full" experience:

CreateInstalledPackage creates a PackageInstall, which, under the hood, creates an App. In order to fully query what we understand by "App", we need the combination of PackageInstall + App.
- Since it is just a matter of 1-5 seconds, this wait will be assumed in the CreateInstalledPackage function.
GetInstalledPackageResourceRefs requires an internal ConfigMap to be created before querying the list of related resources. Rationale: this map stores the "appId" , which is used by the discovery k8s client (like select * from k8s where app.id=appId). There is no other way as of today (confirmed by carvel), but they are working on a new approach.
- Problem: this ConfigMap takes too long (as it depends on kapp to actually reconcile the app), so as suggested, let's move it out of the CreateInstalledPackage.

Up to you. I would have thought that going with (2) initially would leave us one step closer to (3) without setting a precedent of tech-debt (stalling the createinstalledpackage) that may be followed in other plugins. But you're much closer to the details here, so I'll trust your judgement over mine :)

Thanks! I went for option 1 since I just was prioritizing user usability over maintainability. However, as you already split some UI logic apart, I'm absolutely in for opt 2, which is indeed what I have just pushed :)

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-02-02T20:37:15Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_utils.go

@@ -445,3 +465,21 @@ func (f *ConfigurableConfigFactoryImpl) ConfigureRESTConfig(config *rest.Config)
 func (f *ConfigurableConfigFactoryImpl) RESTConfig() (*rest.Config, error) {
 	return f.config, nil
 }
+
+func WaitForResource(ctx context.Context, ri dynamic.ResourceInterface, name string, interval, timeout time.Duration) error {


I've extracted this fn to this utils pkg, but if it were to be useful for other plugins I can move it.

Using a dynamic resource interface so that we just have to build the GRV and get the resource from a dynamic client.

Yes, I think it could be useful for other plugins (eg. for #4213)

Great, I'm on in (in a separate PR, since I wanna move a couple of functions as well)

antgamdia · 2022-02-02T20:42:45Z

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_ctrl_packages.go

+	// The InstalledPackage is considered as created once the associated kapp App gets created,
+	// so we actively wait for the App CR to be present in the cluster before returning OK
+	err = WaitForResource(ctx, resource, newPkgInstall.Name, time.Second*1, time.Second*time.Duration(s.pluginConfig.timeoutSeconds))
+	if err != nil {


Adding more clean-up

antgamdia mentioned this pull request Jan 20, 2022

Remaining work for running Kubeapps with Carvel #3849

Closed

21 tasks

absoludity approved these changes Jan 24, 2022

View reviewed changes

antgamdia added 3 commits February 2, 2022 19:18

Use the core config to set the timeout

f4be34d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Refactor resource wait

75b407f

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add wait for CM in the get refs funciton

feecf0a

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia force-pushed the addWaitCM branch from 3932021 to feecf0a Compare February 2, 2022 20:24

Add missing fragment

81b49b0

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia commented Feb 2, 2022

View reviewed changes

antgamdia mentioned this pull request Feb 3, 2022

Fluxv2: GetInstalledPackageResourceRefs() returns not-found initially before flux creates helm release #4213

Closed

antgamdia merged commit 2367b98 into vmware-tanzu:main Feb 3, 2022

antgamdia deleted the addWaitCM branch February 3, 2022 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait until CM is created when installing a carvel pkg #4139

Wait until CM is created when installing a carvel pkg #4139

antgamdia commented Jan 20, 2022

absoludity commented Jan 24, 2022

Description of the change

antgamdia commented Jan 25, 2022

absoludity commented Jan 26, 2022

antgamdia commented Feb 2, 2022

antgamdia Feb 2, 2022

absoludity Feb 2, 2022

absoludity Feb 2, 2022

antgamdia Feb 3, 2022

antgamdia Feb 2, 2022

Wait until CM is created when installing a carvel pkg #4139

Wait until CM is created when installing a carvel pkg #4139

Conversation

antgamdia commented Jan 20, 2022

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity commented Jan 24, 2022

Description of the change

antgamdia commented Jan 25, 2022

absoludity commented Jan 26, 2022

antgamdia commented Feb 2, 2022

antgamdia Feb 2, 2022

Choose a reason for hiding this comment

absoludity Feb 2, 2022

Choose a reason for hiding this comment

absoludity Feb 2, 2022

Choose a reason for hiding this comment

antgamdia Feb 3, 2022

Choose a reason for hiding this comment

antgamdia Feb 2, 2022

Choose a reason for hiding this comment