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

Duplicate versions for same chart #450

Closed
hobti01 opened this issue Apr 19, 2021 · 19 comments · Fixed by #454
Closed

Duplicate versions for same chart #450

hobti01 opened this issue Apr 19, 2021 · 19 comments · Fixed by #454
Assignees
Labels
Milestone

Comments

@hobti01
Copy link

hobti01 commented Apr 19, 2021

We are seeing many duplicate entries in index.yaml for the same chart version. In some cases there are thousands of duplicates for the same version. We expect only one entry per version/tag.

ChartMuseum version: 0.13.1
Running as part of Harbor with multiple instances of chartmuseum and settings:

ALLOW_OVERWRITE=true
CACHE: redis
DISABLE_STATEFILES: "false"
STORAGE: amazon

Is there a concurrency issue that we should be aware of?

Example:

  - apiVersion: v1
    created: "2021-04-10T14:59:57.826535265Z"
    digest: 1cd815901d2166307de13a7632ae04c053256863e486276c3411b26f7a3b2f0c
    name: ourrepo
    urls:
    - charts/ourrepo-0.0.0-84ace941.tgz
    version: 0.0.0-84ace941
  - apiVersion: v1
    created: "2021-04-19T08:59:10.291630412Z"
    digest: 1cd815901d2166307de13a7632ae04c053256863e486276c3411b26f7a3b2f0c
    name: ourrepo
    urls:
    - charts/ourrepo-0.0.0-84ace941.tgz
    version: 0.0.0-84ace941
@hobti01
Copy link
Author

hobti01 commented Apr 19, 2021

Perhaps this is a symptom of

If someone push the same chart to Chartmuseum and AllowOverwrite is true, cache will exist duplicate version info, like this

  - created: "2019-10-21T09:46:09.216Z"
    digest: 963a506b5523966b993fe68c118d438402121b0d53cb412b9fc50328a428bc6e
    name: account1
    urls:
    - charts/account1-0.0.1-595d1a1bdb2d00edf976c46cde9c7562fab490ba.tgz
    version: 0.0.1-595d1a1bdb2d00edf976c46cde9c7562fab490ba
  - created: "2019-10-21T09:46:09.216Z"
    digest: 963a506b5523966b993fe68c118d438402121b0d53cb412b9fc50328a428bc6e
    name: account1
    urls:
    - charts/account1-0.0.1-595d1a1bdb2d00edf976c46cde9c7562fab490ba.tgz
    version: 0.0.1-595d1a1bdb2d00edf976c46cde9c7562fab490ba

It's a lazy solution to look for 5 versions from back to front. If none of the 5 versions are same to the new pushing version, I think it is a new version and not has been pushed. If there is a same version, I would update it. Because every new version will just be append to the end, I thought this solution is safe. Actually just comparing with the last one version is enough. For insurance purposes, I look for 5 versions from back to front and compare them with the new pushing version.

index.Entries[chartVersion.Name] = append(index.Entries[chartVersion.Name], chartVersion)

I really hope you have a better way.

Originally posted by @Ailsa-Wu in #339 (comment)

@hobti01
Copy link
Author

hobti01 commented Apr 20, 2021

To be very clear, we are eventually seeing hundreds or thousands of duplicate versions after Harbor replications. Is there a more robust approach needed to ensure that duplicate versions do not exist in the index?

@Siegfriedk
Copy link

I think we run in the same issue.

We have a simple workflow which creates test charts per PR we create on our test environment.

For a short while now (after an upgrade) whenever the PR builder is resyncing the chart, it creates a new version.

@scbizu
Copy link
Contributor

scbizu commented Apr 20, 2021

Hi , Thanks for reporting . Do you bootstrap with Overwrites , and run in mostly concurrent environment ?

@Siegfriedk
Copy link

@scbizu we do it like this:
helm push ${chartPath} chartmuseum --force

@hobti01
Copy link
Author

hobti01 commented Apr 21, 2021

we use a curl POST and Harbor replication with overwrite

@scbizu
Copy link
Contributor

scbizu commented May 2, 2021

I think I know why . And the bugfix will be scheduled to next release.

@scbizu scbizu added the bug label May 2, 2021
@scbizu scbizu self-assigned this May 2, 2021
@scbizu scbizu added this to the v0.13.2 milestone May 2, 2021
@scbizu
Copy link
Contributor

scbizu commented May 3, 2021

@hobti01 sorry for disturb you again , but can you provide how many duplicate chart do you have ? According to the original design purpose as you paste:

It's a lazy solution to look for 5 versions from back to front. If none of the 5 versions are same to the new pushing version, I think it is a new version and not has been pushed. If there is a same version, I would update it. Because every new version will just be append to the end, I thought this solution is safe. Actually just comparing with the last one version is enough. For insurance purposes, I look for 5 versions from back to front and compare them with the new pushing version.

your duplicate charts will not increase since the latest version stored must be equal with the new chart you upload . So if your chart version is the same as the latest chart version you stored , the latest chart version will be updated to the chart which you upload .

However , as #220 said , the chart version equality has some bugs before version v0.13.1 . And the old duplicate chart will still exist even after we fixed this issue . The manually deletion for your old duplicate chart is needed after the new version is released.

Thanks for your patience : )

@hobti01
Copy link
Author

hobti01 commented May 3, 2021

In some cases we had > 40000 duplicates for a single version. For many other chart versions we have 10s or 100s of duplicates.

If the upload is only checking the last 5 charts in the local memory cache, then it seems to me that it is not checking the shared index in redis or the charts on disk. With multiple chartmuseum replicas, this seems like an opportunity for each replica to have its own memory cache being correct but the redis stored index is not. However, I have not dived into the code since there are several complicated interactions there.

I've tried to manually delete the duplicate charts but unfortunately the first deletion removes the file on storage and the entries remain in the index with no file in storage (S3 in our case). This causes subsequent 404s when attempting to download the chart.

I have worked around the issue by forcing chartmuseum to rescan every 5s so that the index is rebuilt from the files on disk, but this does not seem like a good solution since the memory cache and redis index cache should make this unnecessary.

@scbizu
Copy link
Contributor

scbizu commented May 4, 2021

I've tried to manually delete the duplicate charts but unfortunately the first deletion removes the file on storage and the entries remain in the index with no file in storage (S3 in our case). This causes subsequent 404s when attempting to download the chart.

It seems like mostly what #220 fixed .

If the upload is only checking the last 5 charts in the local memory cache, then it seems to me that it is not checking the shared index in redis or the charts on disk.

Upload will update the index entry directly and regenerate the repo index via emitting a addChart event(before)/ updateChart event(now for overwrite cases) , you can see some of these changes in my PR #454 and the event listener , but I only test it in local storage without redis caching .

If you can bump to v0.13.1 and try to delete the duplicate chart from index entry and test again , it will make a lot of sense for detecting what the real issue is .

@hobti01
Copy link
Author

hobti01 commented May 4, 2021

We've had all these issues with 0.13.1 so I'm not sure that #220 solves the issue that the file is removed but not all duplicate entries in the index are removed.

@hobti01
Copy link
Author

hobti01 commented May 10, 2021

Thank you for the fix @scbizu , do you have an estimate on when 0.13.2 can be released and used?

@scbizu
Copy link
Contributor

scbizu commented May 10, 2021

I am gonna to see if there is anything else urgent issue can be fixed in v0.13.2 🤔

And will have some talk with @jdolitsky when we should release it .

@jdolitsky jdolitsky modified the milestones: v0.13.2, v0.14.0 Sep 3, 2021
@ninjadq
Copy link
Contributor

ninjadq commented Sep 23, 2021

hi @scbizu, I tested in my environment, it seems this issue still exists.
In my test environment, I am using harbor push-based replication to replicate about 20+ charts from one harbor to chartmuseum inside another harbor instance.

the chartmuseum version in my harbor are following:

@scbizu
Copy link
Contributor

scbizu commented Sep 23, 2021

Hihi @ninjadq , can you give me some more details about the chartmuseum's configuration ?

And does these two harbor share the same index file ?

@ninjadq
Copy link
Contributor

ninjadq commented Sep 23, 2021

image

This is my test environment and test steps

  • Harbor 1 and Harbor 2 are two different instances in different host
  • Harbor 1 has some charts and is used as the source of push-based replication
  • Harbor 2 is used as the destination of push-based replication and didn't has any chart yet
  • Create a push-based replication rule to push 46 charts from harbor 1 to harbor 2
  • manually trigger that replication job 3 times.
  • The number of charts showed in destination harbor 2 is 137. But the actual number of chart files on the disk is the same as the source. Only the index file is different which has 137 entries almost every entry has another two replicas.

this is env file for chartmuseum in harbor

## Settings should be set
PORT=9999

# Only support redis now. If redis is setup, then enable cache
CACHE=redis
CACHE_REDIS_ADDR=redis:6379
CACHE_REDIS_PASSWORD=
CACHE_REDIS_DB=3

# Credential for internal communication
BASIC_AUTH_USER=chart_controller
BASIC_AUTH_PASS=

# Multiple tenants
# Must be set with 1 to support project namespace
DEPTH=1

# Backend storage driver: e.g. "local", "amazon", "google" etc.
STORAGE=local
# Storage driver settings
STORAGE_LOCAL_ROOTDIR=/chart_storage
## Settings with default values. Just put here for future changes
DEBUG=false
LOG_JSON=true
DISABLE_METRICS=false
DISABLE_API=false
DISABLE_STATEFILES=false
ALLOW_OVERWRITE=true
CHART_URL=
AUTH_ANONYMOUS_GET=false
CONTEXT_PATH=
INDEX_LIMIT=0
MAX_STORAGE_OBJECTS=0
MAX_UPLOAD_SIZE=20971520
CHART_POST_FORM_FIELD_NAME=chart
PROV_POST_FORM_FIELD_NAME=prov
STORAGE_TIMESTAMP_TOLERANCE=1s

PS: the chartmuseum binary inside harbor was replaced by the one I build from the source code of chartmuseum's main branch

@scbizu
Copy link
Contributor

scbizu commented Sep 24, 2021

Thanks for your so-detail report , I will try to reproduce this this weekend .

ninjadq added a commit to ninjadq/chartmuseum that referenced this issue Sep 24, 2021
* The detailed issue is described in helm#450
* And there is a PR helm#454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with helm#454

Signed-off-by: DQ <[email protected]>
@ninjadq
Copy link
Contributor

ninjadq commented Sep 25, 2021

I think I got the reason why this duplicate still exists. The harbor replication job replicates charts with the prov filed in request. So it go to another path in the code, and in that path, the issue didn't fix. I've opened a new PR #492 to fix it

ninjadq added a commit to ninjadq/chartmuseum that referenced this issue Sep 25, 2021
* The detailed issue is described in helm#450
* And there is a PR helm#454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with helm#454

Signed-off-by: DQ <[email protected]>
ninjadq added a commit to ninjadq/chartmuseum that referenced this issue Sep 25, 2021
* The detailed issue is described in helm#450
* And there is a PR helm#454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with helm#454

Signed-off-by: DQ <[email protected]>
@scbizu
Copy link
Contributor

scbizu commented Sep 25, 2021

Thank you @ninjadq , I forget the handler for prov XD

ninjadq added a commit to ninjadq/chartmuseum that referenced this issue Sep 25, 2021
* The detailed issue is described in helm#450
* And there is a PR helm#454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with helm#454

Signed-off-by: DQ <[email protected]>
ninjadq added a commit to ninjadq/chartmuseum that referenced this issue Sep 26, 2021
* The detailed issue is described in helm#450
* And there is a PR helm#454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with helm#454

Signed-off-by: DQ <[email protected]>
ninjadq added a commit to ninjadq/chartmuseum that referenced this issue Sep 26, 2021
* The detailed issue is described in helm#450
* And there is a PR helm#454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with helm#454

Signed-off-by: DQ <[email protected]>
scbizu pushed a commit that referenced this issue Sep 30, 2021
* Fix duplicate versions for same chart

* The detailed issue is described in #450
* And there is a PR #454 fixed one scenario of this issue
* But there is another ocassion in which users upload chart with prov
* in this PR is to handle this situation with the way similar with #454

Signed-off-by: DQ <[email protected]>

* Enhance: optimize loop in `getChartAndProvFiles`

* If conflict, it didn't need to do the left logic, just return the file
* move out file format check logic out of `validateChartOrProv`
* these changes are discussed in #492 (comment)

Signed-off-by: DQ <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants