fix(upgrade): Fetch package from its installed cluster #3525

aanthonyrizzo · 2021-09-29T17:35:36Z

Description of the change

This PR addresses (what I think is a bug) regarding the fetching of Packages when upgrading.

Current behavior

When navigating to the upgrade form for deployments that are installed in the "default" cluster. The upgrade form fails to fetch the applicable package details if it is looking in the wrong cluster.

New behavior

When fetching the package details, we use the cluster that is defined in the AvailablePackageDetail property when fetching the package. This should lead to correct fetching behavior.

Benefits

Global packages that are installed in the "" cluster but are deployed in the "default" can now be upgraded.

Possible drawbacks

I don't believe there are any.

Applicable issues

I did not create an issue since I was experimenting w/ solutions locally and seemed to have solved it.

Additional information

I believe this is a relatively impactful bug that is present in 05775ff and should be addressed before the next major release. I have not pinpointed the exact commit that broke this behavior.

@antgamdia @absoludity for visibility

antgamdia · 2021-09-29T22:04:09Z

dashboard/src/components/UpgradeForm/UpgradeForm.tsx

@@ -81,6 +81,8 @@ function UpgradeForm({

  const { availablePackageDetail, versions, schema, values, pkgVersion } = selected;

+  const packageCluster = availablePackageDetail?.availablePackageRef?.context?.cluster;


Since it is a child component depending upon the AppUpgrade view, I wonder whether we should get perform the changes in the view instead.

Given that a UpgradeForm will always be rendered if the app is defined, why not reducing the number of attributes being passed to this component?

But, perhaps we can just add this simple change and later on, review all the namespace/cluster being passed through. (I've added a note in our project board for having a discussion about it)

Also, the tests are failing, I think (not the e2e, but the dashboard_test).

As usual, thanks a lot for contributing to Kubeapps!

Ah okay interesting, yeah maybe putting it in the parent component would be best. I can give it another one over when I fix the tests.

mecampbellsoup · 2021-09-30T19:39:38Z

@aanthonyrizzo @antgamdia so looking at our config.json response, does "global cluster" correspond to kubeappsCluster while our "default" cluster corresponds to clusters[0]?

{
  "kubeappsCluster": "",
  "kubeappsNamespace": "kubeapps",
  "appVersion": "v2.3.4",
  "authProxyEnabled": true,
  "oauthLoginURI": "/",
  "oauthLogoutURI": "/",
  "authProxySkipLoginPage": true,
  "featureFlags": {"invalidateCache":true,"kubeappsAPIsServer":true},
  "clusters": ["default"],
  "theme": "dark",
  "remoteComponentsUrl": "https://cloud.coreweave.com/static/6befb660ea1eeca2b5e78d452e4a52d9b51aca5e.kubeapps.bundle.js"
}

antgamdia · 2021-09-30T20:22:37Z

@aanthonyrizzo @antgamdia so looking at our config.json response, does "global cluster" correspond to kubeappsCluster while our "default" cluster corresponds to clusters[0]?

You're right. "Global cluster" (perhaps not the best word for it) stands for "the cluster on which Kubeapps is installed". This value is calculated here [1] (that is, the cluster configured without a cluster URL or the one marked with isKubeappsCluster: true), and should correspond with one of the clusters[].
I don't think "" is a proper value for this kubeappsCluster property. Perhaps the "{{ template "kubeapps.kubeappsCluster" . -}}", in the chart is not working as expected? I'd try using --dry-run or ' --debug` when installing the helm chart to ensure this value is being set.

This is my local config, for example:

appVersion: "DEVEL"
authProxyEnabled: false
authProxySkipLoginPage: false
clusters: ["default"]
customAppViews: []
kubeappsCluster: "default"
kubeappsNamespace: "kubeapps"
oauthLoginURI: "/oauth2/start"
oauthLogoutURI: "/oauth2/sign_out"
remoteComponentsUrl: ""
theme: "light"

[1] https://github.com/kubeapps/kubeapps/blob/master/chart/kubeapps/templates/_helpers.tpl#L139

aanthonyrizzo · 2021-09-30T20:40:18Z

hey @antgamdia just FYI 6838601 (#3525) fixes a diff detection bug between deployed and the actual chart. This was caused by deployed always being {}, causing https://github.com/kubeapps/kubeapps/blob/master/dashboard/src/components/UpgradeForm/UpgradeForm.tsx#L102 to never evaluate to true. Should be correct now.

mecampbellsoup · 2021-09-30T20:57:52Z

Nice so #3525 should be good to go now, hopefully that can get merged quickly @antgamdia - thanks for chiming in here!

antgamdia · 2021-09-30T21:12:43Z

Awesome! Thanks for fixing it. Even if we try our best to test and ensure everything is working as expected, with this UI revamp to support multiple plugins we have still to polish up some edge cases.
I'll try to squeeze a thorough review of the cluster and namespace params usage in the next iteration.

Also FYI, our CI ("just" the e2e tests) is failing right now. This matter needs to be discussed (more info at #3526), meaning the PR merge process (and the release 2.4.1) has to be postponed to the next week (Michael is on PTO this week).

aanthonyrizzo · 2021-09-30T22:22:58Z

Awesome! Thanks for fixing it. Even if we try our best to test and ensure everything is working as expected, with this UI revamp to support multiple plugins we have still to polish up some edge cases. I'll try to squeeze a thorough review of the cluster and namespace params usage in the next iteration.

Yeah I've been poking around and I think there are a few places were some things can be refactored/cleaned up (possibly just renaming some variables and such). It also seems like there is some duplicated data which might be muddying the source of truth and causing some of these little edge case issues.

Also FYI, our CI ("just" the e2e tests) is failing right now. This matter needs to be discussed (more info at #3526), meaning the PR merge process (and the release 2.4.1) has to be postponed to the next week (Michael is on PTO this week).

Okay awesome thank you for the update. Let me know if theres anything else I can do. (also not sure why but it seems its trying to push my images causing my tests to fail)

absoludity · 2021-10-05T01:01:52Z

dashboard/src/components/AppUpgrade/AppUpgrade.tsx

@@ -75,7 +75,10 @@ function AppUpgrade() {
    dispatch(
      actions.charts.getDeployedChartVersion(
        {
-          context: { cluster: cluster, namespace: repoNamespace ?? "" },
+          context: {
+            cluster: app?.availablePackageRef?.context?.cluster ?? cluster,


Thanks Anthony. Yes, this should definitely be defaulting to the cluster of the available package, rather than the cluster of the installed app. +1 from me. Is the other discussion below resolved Antonio?

Yep, everything is ready to go, I think. Indeed, given how messy the upgrade view was becoming I gave it a shot and try to perform some improvements (#3540).
However, the scope of the change is much bigger and I don't think we should release with my PR, but with this simpler and laser-focus one.

Let me jump in and merge the latest changes in the main branch (which hopefully should fix the CI tests...)

@antgamdia that PR is def gonna be a good improvement. There are a lot of rerenders that end up going all the way down the chain and they could be optimized and improve the performance of the page for sure

There are a lot of rerenders that end up going all the way down the chain

Yeah... in fact we have been facing some CI issues because of that (the dropdown was being re-rendered multiple times making our e2e engine chose the wrong version)

Let me jump in and merge the latest changes in the main branch (which hopefully should fix the CI tests...)

I've tried but no permissions to edit this PR, I think. Please, @aanthonyrizzo, pull the latest changes in master and merge them into your branch. Let's see if tomorrow we can start tagging a release :P

AH. Sorry we changed the ownership of the repo from my user to our organization. I just rebased so should be up to date.

Mmm, perhaps it is why the CI is failing when building the images? Not sure, but maybe we should create another PR from this repo?

Ah that could be. I'm going to be away for a bit but feel free to cherry pick the commits into another branch and merge :)

Mmm, don't know, re-triggering it made the trick. CI things...
Merging! Thanks again for the contributions :)

antgamdia reviewed Sep 29, 2021

View reviewed changes

aanthonyrizzo force-pushed the fix-package-fetch-logic branch from cc676bc to 3cc30a4 Compare September 29, 2021 23:58

antgamdia mentioned this pull request Sep 30, 2021

Release 2.4.1 #3531

Closed

10 tasks

mecampbellsoup force-pushed the fix-package-fetch-logic branch from 6838601 to ef65b23 Compare October 4, 2021 17:20

aanthonyrizzo force-pushed the fix-package-fetch-logic branch from ef65b23 to 6838601 Compare October 4, 2021 17:26

mecampbellsoup force-pushed the fix-package-fetch-logic branch from 6838601 to 60ef713 Compare October 4, 2021 17:28

aanthonyrizzo force-pushed the fix-package-fetch-logic branch from 60ef713 to 6838601 Compare October 4, 2021 17:32

mecampbellsoup mentioned this pull request Oct 4, 2021

fix(upgrade): Fetch package from its installed cluster coreweave/kubeapps#4

Closed

absoludity approved these changes Oct 5, 2021

View reviewed changes

aanthonyrizzo added 2 commits October 5, 2021 14:36

fix(upgrade): Fetch package from its installed cluster

fb5a60d

fix(cluster): Fetch correct cluster for deployed chart

7fec7dd

aanthonyrizzo force-pushed the fix-package-fetch-logic branch from 6838601 to 7fec7dd Compare October 5, 2021 18:42

antgamdia mentioned this pull request Oct 5, 2021

Duplicated - fix(upgrade): Fetch package from its installed cluster #3545

Closed

antgamdia merged commit fb1ec00 into vmware-tanzu:master Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(upgrade): Fetch package from its installed cluster #3525

fix(upgrade): Fetch package from its installed cluster #3525

aanthonyrizzo commented Sep 29, 2021 •

edited

Loading

antgamdia Sep 29, 2021

aanthonyrizzo Sep 29, 2021

mecampbellsoup commented Sep 30, 2021 •

edited

Loading

antgamdia commented Sep 30, 2021

aanthonyrizzo commented Sep 30, 2021 •

edited

Loading

mecampbellsoup commented Sep 30, 2021

antgamdia commented Sep 30, 2021

aanthonyrizzo commented Sep 30, 2021 •

edited

Loading

absoludity Oct 5, 2021

antgamdia Oct 5, 2021

aanthonyrizzo Oct 5, 2021

antgamdia Oct 5, 2021

aanthonyrizzo Oct 5, 2021

antgamdia Oct 5, 2021

aanthonyrizzo Oct 5, 2021

antgamdia Oct 5, 2021

		@@ -81,6 +81,8 @@ function UpgradeForm({

		const { availablePackageDetail, versions, schema, values, pkgVersion } = selected;

		const packageCluster = availablePackageDetail?.availablePackageRef?.context?.cluster;

fix(upgrade): Fetch package from its installed cluster #3525

fix(upgrade): Fetch package from its installed cluster #3525

Conversation

aanthonyrizzo commented Sep 29, 2021 • edited Loading

Description of the change

Current behavior

New behavior

Benefits

Possible drawbacks

Applicable issues

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mecampbellsoup commented Sep 30, 2021 • edited Loading

antgamdia commented Sep 30, 2021

aanthonyrizzo commented Sep 30, 2021 • edited Loading

mecampbellsoup commented Sep 30, 2021

antgamdia commented Sep 30, 2021

aanthonyrizzo commented Sep 30, 2021 • 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

aanthonyrizzo commented Sep 29, 2021 •

edited

Loading

mecampbellsoup commented Sep 30, 2021 •

edited

Loading

aanthonyrizzo commented Sep 30, 2021 •

edited

Loading

aanthonyrizzo commented Sep 30, 2021 •

edited

Loading