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.

Sign up for GitHub

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

Cache rendered centrals #1433

Closed
wants to merge 3 commits into from
Closed

Cache rendered centrals #1433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80fa426
X-Smart-Branch-Parent: sb/add-pprof-server
SimonBaeumer Nov 3, 2023
267e957
Cache rendered Centrals
SimonBaeumer Nov 2, 2023
39f6395
code fix
SimonBaeumer Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 74 additions & 4 deletions internal/dinosaur/pkg/gitops/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,26 @@ package gitops

import (
"encoding/json"
"fmt"
"strings"
"sync"
"text/template"

"github.com/pkg/errors"
"github.com/stackrox/acs-fleet-manager/fleetshard/pkg/util"
"github.com/stackrox/rox/operator/apis/platform/v1alpha1"
"k8s.io/apimachinery/pkg/util/strategicpatch"
"sigs.k8s.io/yaml"
)

var (
centralCRYAMLCache = make(map[string]string)
centralCacheMutex = sync.RWMutex{}
)

// Service applies GitOps configuration to Central instances.
type Service interface {
GetCentral(ctx CentralParams) (v1alpha1.Central, error)
GetCentral(ctx CentralParams) (string, error)
}

type service struct {
Expand All @@ -26,12 +34,74 @@ func NewService(configProvider ConfigProvider) Service {
}

// GetCentral returns a Central instance with the given parameters.
func (s *service) GetCentral(params CentralParams) (v1alpha1.Central, error) {
func (s *service) GetCentral(params CentralParams) (string, error) {
cfg, err := s.configProvider.Get()
if err != nil {
return v1alpha1.Central{}, errors.Wrap(err, "failed to get GitOps configuration")
return "", errors.Wrap(err, "failed to get GitOps configuration")
}

result, err := readFromCache(params, cfg)
if err != nil {
return "", err
}
if result != "" {
return result, nil
}

centralCR, err := renderCentral(params, cfg)
if err != nil {
return "", err
}

centralYaml, err := yaml.Marshal(centralCR)
if err != nil {
return "", errors.Wrap(err, "failed to marshal Central CR")
}

centralYAMLString := string(centralYaml)
err = addCache(params, cfg, centralYAMLString)
if err != nil {
return "", err
}
return centralYAMLString, nil
Comment on lines +43 to +66
Copy link
Collaborator

@ludydoo ludydoo Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to lock the cache key

Suggested change
result, err := readFromCache(params, cfg)
if err != nil {
return "", err
}
if result != "" {
return result, nil
}
centralCR, err := renderCentral(params, cfg)
if err != nil {
return "", err
}
centralYaml, err := yaml.Marshal(centralCR)
if err != nil {
return "", errors.Wrap(err, "failed to marshal Central CR")
}
centralYAMLString := string(centralYaml)
err = addCache(params, cfg, centralYAMLString)
if err != nil {
return "", err
}
return centralYAMLString, nil
cacheKey, err := getCacheKey(params, cfg)
if err != nil {
return "", err
}
km.Lock(cacheKey)
defer km.Unlock(cacheKey)
if result := readFromCache(cacheKey); result != "" {
return result, nil
}
centralCR, err := renderCentral(params, cfg)
if err != nil {
return "", err
}
centralYaml, err := yaml.Marshal(centralCR)
if err != nil {
return "", errors.Wrap(err, "failed to marshal Central CR")
}
centralYAMLString := string(centralYaml)
addCache(cacheKey, centralYAMLString)
return centralYAMLString, nil

}

// getCacheKey returns a key in the format of "<params_hash>:<config_hash>"
func getCacheKey(params CentralParams, config Config) (string, error) {
paramsHash, err := util.MD5SumFromJSONStruct(params)
if err != nil {
return "", fmt.Errorf("could not create MD5 from CentralParams name: %s, id: %s, %w", params.Name, params.ID, err)
}
configHash, err := util.MD5SumFromJSONStruct(config)
if err != nil {
return "", fmt.Errorf("could not create MD5 from gitops Configame: %s, id: %s, %w", params.Name, params.ID, err)
}
return fmt.Sprintf("%s:%s", string(paramsHash[:]), string(configHash[:])), nil
}

func addCache(params CentralParams, config Config, centralYAML string) error {
centralCacheMutex.Lock()
defer centralCacheMutex.Unlock()
key, err := getCacheKey(params, config)
if err != nil {
return fmt.Errorf("could not add to cache: %w", err)
}

centralCRYAMLCache[key] = centralYAML
return nil
Comment on lines +82 to +91
Copy link
Collaborator

@ludydoo ludydoo Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing the cache key directly

Suggested change
func addCache(params CentralParams, config Config, centralYAML string) error {
centralCacheMutex.Lock()
defer centralCacheMutex.Unlock()
key, err := getCacheKey(params, config)
if err != nil {
return fmt.Errorf("could not add to cache: %w", err)
}
centralCRYAMLCache[key] = centralYAML
return nil
func addCache(cacheKey string, centralYAML string) {
centralCacheMutex.Lock()
defer centralCacheMutex.Unlock()
centralCRYAMLCache[key] = centralYAML

}

Copy link
Collaborator

@ludydoo ludydoo Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of keyed mutex for locking cache keys

Suggested change
var km = keyedMutex{locks: map[string]*sync.Mutex{}}
type keyedMutex struct {
locks map[string]*sync.Mutex
mLock sync.Mutex
}
func (k keyedMutex) getLockBy(key string) *sync.Mutex {
k.mLock.Lock()
defer k.mLock.Unlock()
if ret, ok := k.locks[key]; ok {
return ret
}
lock := &sync.Mutex{}
k.locks[key] = lock
return lock
}
func (k keyedMutex) Lock(key string) {
k.getLockBy(key).Lock()
}
func (k keyedMutex) Unlock(key string) {
k.getLockBy(key).Unlock()
}

// readFromCache returns a CentralYAML from the cache
func readFromCache(params CentralParams, config Config) (string, error) {
cacheKey, err := getCacheKey(params, config)
if err != nil {
return "", fmt.Errorf("Could not get cache key: %w", err)
}

if val, ok := centralCRYAMLCache[cacheKey]; ok {
Comment on lines +95 to +101
Copy link
Collaborator

@ludydoo ludydoo Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you need to RLock the cache + passing the cache key directly

Suggested change
func readFromCache(params CentralParams, config Config) (string, error) {
cacheKey, err := getCacheKey(params, config)
if err != nil {
return "", fmt.Errorf("Could not get cache key: %w", err)
}
if val, ok := centralCRYAMLCache[cacheKey]; ok {
func readFromCache(cacheKey string) (string) {
centralCRYAMLCache.RLock()
defer centralCRYamlCache.RUnlock()
if val, ok := centralCRYAMLCache[cacheKey]; ok {

return val, nil
}
return renderCentral(params, cfg)
return "", nil
}

func renderCentral(params CentralParams, config Config) (v1alpha1.Central, error) {
Expand Down
47 changes: 46 additions & 1 deletion internal/dinosaur/pkg/gitops/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,56 @@ func TestService_GetCentral(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
svc := NewService(newMockProvider(tt.config))
got, err := svc.GetCentral(tt.params)
tt.assert(t, got, err)
centralGot := v1alpha1.Central{}
errUnmarshal := yaml.Unmarshal([]byte(got), &centralGot)
require.NoError(t, errUnmarshal)
tt.assert(t, centralGot, err)
})
}
}

func TestServiceGetCache(t *testing.T) {
c := Config{
Centrals: CentralsConfig{
Overrides: []CentralOverride{
{
InstanceIDs: []string{"*"},
Patch: `metadata: {"labels": {"foo": "bar"}}`,
},
},
},
}
params := CentralParams{
ID: "id-123",
Name: "Central-Name",
}

gitOpsService := NewService(newMockProvider(c))
centralYAML, err := gitOpsService.GetCentral(params)
require.NoError(t, err)

central := v1alpha1.Central{}
err = yaml.Unmarshal([]byte(centralYAML), &central)
require.NoError(t, err)

// assert that the rendered Central is present in the cache
require.Len(t, centralCRYAMLCache, 1)
key, err := getCacheKey(params, c)
assert.Equal(t, centralYAML, centralCRYAMLCache[key])
require.NoError(t, err)

// assert that the Central is returned correctly from the cache
centralYAMLFromCache, err := readFromCache(params, c)
require.NoError(t, err)
assert.Equal(t, centralYAML, centralYAMLFromCache)

// assert that the Central is NOT returned with a different config
c.Centrals = CentralsConfig{}
result, err := readFromCache(params, c)
require.NoError(t, err)
assert.Empty(t, result)
}

// TestDefaultTemplateIsValid tests that the default template is valid and
// can be unmarshaled to a functional v1alpha1.Central object.
func Test_defaultTemplate_isValid(t *testing.T) {
Expand Down
8 changes: 1 addition & 7 deletions internal/dinosaur/pkg/presenters/managedcentral.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/config"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/gitops"
"k8s.io/apimachinery/pkg/api/resource"
"sigs.k8s.io/yaml"
)

// ManagedCentralPresenter helper service which converts Central DB representation to the private API representation
Expand All @@ -32,16 +31,11 @@ func NewManagedCentralPresenter(config *config.CentralConfig, gitopsService gito
// PresentManagedCentral converts DB representation of Central to the private API representation
func (c *ManagedCentralPresenter) PresentManagedCentral(from *dbapi.CentralRequest) (private.ManagedCentral, error) {

centralCR, err := c.gitopsService.GetCentral(centralParamsFromRequest(from))
centralYaml, err := c.gitopsService.GetCentral(centralParamsFromRequest(from))
if err != nil {
return private.ManagedCentral{}, errors.Wrap(err, "failed to apply GitOps overrides to Central")
}

centralYaml, err := yaml.Marshal(centralCR)
if err != nil {
return private.ManagedCentral{}, errors.Wrap(err, "failed to marshal Central CR")
}

res := private.ManagedCentral{
Id: from.ID,
Kind: "ManagedCentral",
Expand Down
Loading