Skip to content
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.

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

flux plugin: clean up old charts from chart cache after repo update #4115

Closed
gfichtenholt opened this issue Jan 19, 2022 · 2 comments · Fixed by #5644
Closed

flux plugin: clean up old charts from chart cache after repo update #4115

gfichtenholt opened this issue Jan 19, 2022 · 2 comments · Fixed by #5644
Assignees
Labels
component/apis-server Issue related to kubeapps api-server component/plugin-flux Issue related to kubeapps plugin to manage Helm charts via Flux kind/enhancement An issue that reports an enhancement for an implemented feature

Comments

@gfichtenholt
Copy link
Contributor

Documenting leftover items for flux plugin per Pepe's request so we can prioritize it against other work

This is a leftover item from #3899
@antgamdia said:
"Think about an already cached repo, if the repo authors eventually sunset a given version and it gets removed from the index (case 1), what would happen then? What if a whole chart got removed (case 2)?
Let's suppose:

(case 1): my-chart: v1, v2 --> then my-chart:v2
(case 2): my-chart: v1, foo-chart: v1 --> then foo-chart: v1

"
case 1 is no problem but case 2 is an issue today. In both cases there will be an event 'repo XYZ' was modified. In handling this event, I will call SyncCharts(repo XYZ). Currently I just go through the most recent versions for every chart and add that. I don't delete any charts prior to that. So in case 1: you'll end up with my-chart:v2, same as before the update. In case 2, you'll end up with my-chart: v1, foo-chart: v1. The problem is my-chart: v1 does not belong in the cache anymore: it takes up precious space and the user might get details on a package which no longer exists.

There is another thing that kind of complicates case 2: my-chart: v1 might have been added explictly to to chart cache by the user in a scenario where the entry was first evicted due to lack of space and then added due to user actually performing getAvailableChartDetail(my-chart: v1). So in that case, assuming my-chart: v1 is still a valid chart, do I delete it or leave it alone because its in the cache due to user's request?

@gfichtenholt gfichtenholt self-assigned this Jan 19, 2022
@gfichtenholt gfichtenholt added component/apis-server Issue related to kubeapps api-server size/S labels Jan 19, 2022
@antgamdia
Copy link
Contributor

FWIW, we have had this (case 2) problem for quite a long time. In fact, we had this old issue tracking it: #164

I'd personally opt for clearing this cache entry in case of doubt. If we do so, the worst-case scenario is a user waiting some seconds. Otherwise, a user would see an item that does no longer exists, which is a worse UX, IMHO.

@ppbaena ppbaena added this to the Pluggable support for Flux v2 milestone Jan 19, 2022
@stale
Copy link

stale bot commented Feb 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Automatic label to stale issues due inactivity to be closed if no further action label Feb 3, 2022
@gfichtenholt gfichtenholt removed the stale Automatic label to stale issues due inactivity to be closed if no further action label Feb 4, 2022
@ppbaena ppbaena added kind/enhancement An issue that reports an enhancement for an implemented feature priority/low labels Feb 4, 2022
@ppbaena ppbaena added this to Kubeapps Mar 9, 2022
@ppbaena ppbaena moved this to 🗂. Backlog in Kubeapps Mar 9, 2022
@ppbaena ppbaena added this to Kubeapps Mar 31, 2022
@ppbaena ppbaena moved this to 🗂 Backlog in Kubeapps Mar 31, 2022
@ppbaena ppbaena added the component/plugin-flux Issue related to kubeapps plugin to manage Helm charts via Flux label Jun 8, 2022
@ppbaena ppbaena added the next-iteration Issues to be discussed in planning session label Nov 4, 2022
@ppbaena ppbaena moved this from 🗂 Backlog to 🗒 Todo in Kubeapps Nov 7, 2022
@gfichtenholt gfichtenholt moved this from 🗒 Todo to 🏗 In Progress in Kubeapps Nov 15, 2022
gfichtenholt added a commit that referenced this issue Nov 21, 2022
…update #4115  (#5644)

fix for long-standing issue
#4115

plus DRYing up a bunch of existing test code

Not 100% sure who to ask for review. Its not urgent. If no-one
materializes in the next few days, I will ask

The only thing of interest here is really a new func call 
```
chartCache.PurgeObsoleteChartVersions(charts)
```
executed before an old call to 
```
SyncCharts()
```

There is a new integration test I added for this scenario.
Most everything else is test code cleanup and doesn't need any review
Repository owner moved this from 🗂. Backlog to ✅. Done in Kubeapps Nov 21, 2022
Repository owner moved this from 🏗 In Progress to ✅ Done in Kubeapps Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/apis-server Issue related to kubeapps api-server component/plugin-flux Issue related to kubeapps plugin to manage Helm charts via Flux kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
Archived in project
3 participants