pkg/chartmuseum,cmd: introduce the new `keep-chart-always-up-to-date` flag and the default cache interval when not set. by scbizu · Pull Request #593 · helm/chartmuseum

scbizu · 2022-06-05T15:03:20Z

Closes #444

The flags works to always get the realtime index for the chart repository(which is the behavior before v0.13.0), it will cost more fs I/O.
Default interval is now set to 5m when cache-interval flag not set which we suggests in the issue list.

cbuto · 2022-06-15T16:11:37Z

looks like there is a new data race

WARNING: DATA RACE
Read at 0x00c000c2eb10 by goroutine 89:
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServer).rebuildIndex()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/cache.go:568 +0x91
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServer).initCacheTimer.func1()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/cache.go:481 +0xb8
Previous write at 0x00c000c2eb10 by goroutine 31:
2022-06-12T18:50:07.522Z	DEBUG	index.yaml regenerated	{"repo": ""}
2022-06-12T18:50:07.522Z	DEBUG	Entry saved in cache store	{"repo": ""}
  [failed to restore the stack]
Goroutine 89 (running) created at:
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServer).initCacheTimer()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/cache.go:478 +0xd8
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.NewMultiTenantServer()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server.go:190 +0xe68
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServerTestSuite).SetupSuite()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server_test.go:278 +0x1cf7
  github.com/stretchr/testify/suite.Run()
      /home/runner/work/chartmuseum/chartmuseum/vendor/github.com/stretchr/testify/suite/suite.go:128 +0x5b7
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.TestMultiTenantServerTestSuite()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server_test.go:1312 +0x44
  testing.tRunner()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1306 +0x47
Goroutine 31 (finished) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /home/runner/work/chartmuseum/chartmuseum/vendor/github.com/stretchr/testify/suite/suite.go:213 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/runner/work/chartmuseum/chartmuseum/vendor/github.com/stretchr/testify/suite/suite.go:186 +0x994
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.TestMultiTenantServerTestSuite()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server_test.go:1312 +0x44
  testing.tRunner()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1306 +0x47
==================

scbizu · 2022-06-16T02:47:59Z

looks like there is a new data race

WARNING: DATA RACE
Read at 0x00c000c2eb10 by goroutine 89:
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServer).rebuildIndex()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/cache.go:568 +0x91
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServer).initCacheTimer.func1()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/cache.go:481 +0xb8
Previous write at 0x00c000c2eb10 by goroutine 31:
2022-06-12T18:50:07.522Z	DEBUG	index.yaml regenerated	{"repo": ""}
2022-06-12T18:50:07.522Z	DEBUG	Entry saved in cache store	{"repo": ""}
  [failed to restore the stack]
Goroutine 89 (running) created at:
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServer).initCacheTimer()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/cache.go:478 +0xd8
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.NewMultiTenantServer()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server.go:190 +0xe68
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.(*MultiTenantServerTestSuite).SetupSuite()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server_test.go:278 +0x1cf7
  github.com/stretchr/testify/suite.Run()
      /home/runner/work/chartmuseum/chartmuseum/vendor/github.com/stretchr/testify/suite/suite.go:128 +0x5b7
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.TestMultiTenantServerTestSuite()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server_test.go:1312 +0x44
  testing.tRunner()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1306 +0x47
Goroutine 31 (finished) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /home/runner/work/chartmuseum/chartmuseum/vendor/github.com/stretchr/testify/suite/suite.go:213 +0x18f
  github.com/stretchr/testify/suite.Run()
      /home/runner/work/chartmuseum/chartmuseum/vendor/github.com/stretchr/testify/suite/suite.go:186 +0x994
  helm.sh/chartmuseum/pkg/chartmuseum/server/multitenant.TestMultiTenantServerTestSuite()
      /home/runner/work/chartmuseum/chartmuseum/pkg/chartmuseum/server/multitenant/server_test.go:1312 +0x44
  testing.tRunner()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /opt/hostedtoolcache/go/1.17.8/x64/src/testing/testing.go:1306 +0x47
==================

Yes , I will try to resolve it

… flag and the default cache interval when not set. * The flags works to always get the realtime index for the chart repository(which is the behavior before v0.13.0), it will cost more fs I/O. * Default interval is now set to 5m when `cache-interval` flag not set which we suggests in the issue list. The patch includes: * increases most of the museum api throughout by wrapping the event emitter into goroutine since the event listener already holds on the concurrent lock(#396). * adds the new server option `keep-chart-always-up-to-date` to force use the latest version. * `getIndexFile` rollbacks to fully reload index only if cacheinterval not set for better backward compatibility before v0.13.0(#444). Signed-off-by: scnace <[email protected]> Signed-off-by: scbizu <[email protected]>

…index refresher works. Signed-off-by: scbizu <[email protected]>

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

scbizu · 2022-06-19T13:06:06Z

Seems like resolved it , so many lock tricks XD

PTAL @cbuto

cbuto · 2022-06-21T19:51:12Z

 		PerChartLimit:          conf.GetInt("per-chart-limit"),
 		WebTemplatePath:        conf.GetString("web-template-path"),
 		ArtifactHubRepoID:      conf.GetStringMapString("artifact-hub-repo-id"),
+		AlwaysRegenerateIndex:  conf.GetBool("always-regenerate-index"),


to make sure this matches the flag name in pkg/config/vars.go

Suggested change

AlwaysRegenerateIndex: conf.GetBool("always-regenerate-index"),

AlwaysRegenerateIndex: conf.GetBool("always-regenerate-chart-index"),

nice pickup ~

cbuto · 2022-06-21T19:52:53Z

nice @scbizu! looks good aside from the one comment. is it worth running a quick loadtest locally to compare main vs these changes (mainly the new locks)?

scbizu · 2022-06-22T02:18:36Z

@cbuto Sorry for my low local machine setup , I need your help for the loadtest xD

cbuto · 2022-06-22T20:58:52Z

@cbuto Sorry for my low local machine setup , I need your help for the loadtest xD

no problem! I'll try to get some time to run a few and post the results

cbuto · 2022-06-24T01:58:28Z

I ran three tests:

Built from this branch - ./bin/darwin/amd64/chartmuseum --storage local --storage-local-rootdir /tmp/charts/ --always-regenerate-chart-index
Built from this branch - ./bin/darwin/amd64/chartmuseum --storage local --storage-local-rootdir /tmp/charts/
Built from main - ./bin/darwin/amd64/chartmuseum --storage local --storage-local-rootdir /tmp/charts/

Overall it seems like these changes without the flag set shouldn't have any affect, but setting this flags introduces a ton of latency, which I think is expected.

scbizu · 2022-06-24T02:31:01Z

I think so , thank you @cbuto

cbuto

Lgtm! We’ll just need to address the flag name issue in the other comment

mikesigs · 2022-07-18T15:53:28Z

Really eager to see this fix merged in :) The PR has been reviewed, does it need to also wait for @jdolitsky to review?

scbizu · 2022-07-19T02:27:05Z

It would be better to be so , and I think cbuto and me also can handle it and make the canary(main) release , thoughts @cbuto ?

cbuto · 2022-07-19T20:09:19Z

@scbizu I'd be good with merging this after #593 (comment) is resolved!

scbizu · 2022-07-20T01:53:42Z

okay , I will resolve it soon ~

Co-authored-by: Casey Buto <[email protected]> Signed-off-by: Nace Sc <[email protected]> Signed-off-by: scbizu <[email protected]>

scbizu · 2022-07-20T01:59:55Z

Solved

jdolitsky

👍

pull-request-size Bot added the size/L label Jun 5, 2022

scbizu force-pushed the feat/keep-chart-up-date-option branch 4 times, most recently from dea15fe to 44034ff Compare June 5, 2022 16:36

scbizu requested review from cbuto and jdolitsky June 5, 2022 16:41

scbizu mentioned this pull request Jun 5, 2022

proposal: work on increasing the server throughout. #457

Closed

cbuto reviewed Jun 6, 2022

View reviewed changes

scbizu force-pushed the feat/keep-chart-up-date-option branch 7 times, most recently from 739b0a7 to 7b02ed0 Compare June 12, 2022 18:46

cbuto reviewed Jun 15, 2022

View reviewed changes

Comment thread pkg/config/vars.go Outdated

scbizu force-pushed the feat/keep-chart-up-date-option branch 8 times, most recently from 810daba to e029f4c Compare June 19, 2022 12:46

scbizu added 3 commits June 19, 2022 20:47

pkg/chartmuseum/server: getRepoObjectSlice adds the read lock when …

763442f

…index refresher works. Signed-off-by: scbizu <[email protected]>

pkg/chartmuseum: rebuildIndex adds tenant lock

8af2bd2

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

scbizu force-pushed the feat/keep-chart-up-date-option branch 2 times, most recently from 123cf9f to d8e389d Compare June 19, 2022 13:00

cbuto reviewed Jun 21, 2022

View reviewed changes

cbuto approved these changes Jun 24, 2022

View reviewed changes

pkg/config: clarify the usage of always-regenerate-chart-index

a1cefb9

Co-authored-by: Casey Buto <[email protected]> Signed-off-by: Nace Sc <[email protected]> Signed-off-by: scbizu <[email protected]>

scbizu force-pushed the feat/keep-chart-up-date-option branch from d8e389d to a1cefb9 Compare July 20, 2022 01:58

scbizu requested a review from cbuto July 20, 2022 01:59

cbuto approved these changes Jul 29, 2022

View reviewed changes

jdolitsky approved these changes Aug 1, 2022

View reviewed changes

jdolitsky merged commit 3ae6ed2 into main Aug 1, 2022

	AlwaysRegenerateIndex: conf.GetBool("always-regenerate-index"),
	AlwaysRegenerateIndex: conf.GetBool("always-regenerate-chart-index"),

Uh oh!

Conversation

scbizu commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbuto commented Jun 15, 2022

Uh oh!

Uh oh!

scbizu commented Jun 16, 2022

Uh oh!

scbizu commented Jun 19, 2022

Uh oh!

cbuto Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

scbizu Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

cbuto commented Jun 21, 2022

Uh oh!

scbizu commented Jun 22, 2022

Uh oh!

cbuto commented Jun 22, 2022

Uh oh!

cbuto commented Jun 24, 2022

Uh oh!

scbizu commented Jun 24, 2022

Uh oh!

cbuto left a comment

Choose a reason for hiding this comment

Uh oh!

mikesigs commented Jul 18, 2022

Uh oh!

scbizu commented Jul 19, 2022

Uh oh!

cbuto commented Jul 19, 2022

Uh oh!

scbizu commented Jul 20, 2022

Uh oh!

scbizu commented Jul 20, 2022

Uh oh!

jdolitsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scbizu commented Jun 5, 2022 •

edited

Loading