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

Fix/repo locking #838

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Fix/repo locking #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9407d68
chore: removed unused code
laszlocph Mar 13, 2024
ef83b77
fix: improved repo locking
laszlocph Mar 13, 2024
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
13 changes: 7 additions & 6 deletions pkg/dashboard/server/commits.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,14 @@ func commits(w http.ResponseWriter, r *http.Request) {
}

var hash plumbing.Hash
gitRepoCache.PerformAction(repoName, func(repo *git.Repository) error {
hash = nativeGit.BranchHeadHash(repo, branch)
return nil
})

if hashString != "head" {
hash = plumbing.NewHash(hashString)
} else {
gitRepoCache.PerformAction(repoName, func(repo *git.Repository) error {
logrus.Debugf("getting branchheadhash for %s", branch)
hash = nativeGit.BranchHeadHash(repo, branch)
return nil
})
}

var commitWalker object.CommitIter
Expand All @@ -64,7 +65,7 @@ func commits(w http.ResponseWriter, r *http.Request) {
return err
})
if err != nil {
logrus.Errorf("cannot walk commits: %s", err)
logrus.Errorf("cannot walk commits from %s: %s", hash, err)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
return
}
Expand Down
42 changes: 2 additions & 40 deletions pkg/dashboard/server/gitContents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,37 +72,7 @@ func getMetas(w http.ResponseWriter, r *http.Request) {
gitRepoCache, _ := ctx.Value("gitRepoCache").(*nativeGit.RepoCache)
repoName := fmt.Sprintf("%s/%s", owner, repo)

var hasGithubActionsConfig bool
var githubActionsShipper *string
var err error
githubActionsConfigPath := filepath.Join(".github", "workflows")
githubActionsShipperCommand := "gimlet-io/gimlet-artifact-shipper-action"
err = gitRepoCache.PerformAction(repoName, func(repo *git.Repository) error {
var innerErr error
hasGithubActionsConfig, githubActionsShipper, innerErr = hasCiConfigAndShipper(repo, githubActionsConfigPath, githubActionsShipperCommand)
return innerErr
})
if err != nil {
logrus.Errorf("cannot determine ci status: %s", err)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
return
}

var hasCircleCiConfig bool
var circleCiShipper *string
circleCiConfigPath := ".circleci"
circleCiShipperCommand := "gimlet/gimlet-artifact-create"
err = gitRepoCache.PerformAction(repoName, func(repo *git.Repository) error {
var innerErr error
hasCircleCiConfig, circleCiShipper, innerErr = hasCiConfigAndShipper(repo, circleCiConfigPath, circleCiShipperCommand)
return innerErr
})
if err != nil {
logrus.Errorf("cannot determine ci status: %s", err)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
return
}

var files map[string]string
err = gitRepoCache.PerformAction(repoName, func(repo *git.Repository) error {
var innerErr error
Expand Down Expand Up @@ -133,11 +103,7 @@ func getMetas(w http.ResponseWriter, r *http.Request) {
}

gitRepoM := gitRepoMetas{
GithubActions: hasGithubActionsConfig,
CircleCi: hasCircleCiConfig,
GithubActionsShipper: githubActionsShipper,
CircleCiShipper: circleCiShipper,
FileInfos: fileInfos,
FileInfos: fileInfos,
}

gitRepoMString, err := json.Marshal(gitRepoM)
Expand Down Expand Up @@ -312,11 +278,7 @@ type fileInfo struct {
}

type gitRepoMetas struct {
GithubActions bool `json:"githubActions"`
CircleCi bool `json:"circleCi"`
GithubActionsShipper *string `json:"githubActionsShipper"`
CircleCiShipper *string `json:"circleCiShipper"`
FileInfos []fileInfo `json:"fileInfos"`
FileInfos []fileInfo `json:"fileInfos"`
}

// envConfig fetches all environment configs from source control for a repo
Expand Down
118 changes: 67 additions & 51 deletions pkg/git/nativeGit/repoCache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type RepoCache struct {
repos map[string]*repoData
reposMapLock sync.Mutex // lock this if you add or remove items from the repos map
stopCh chan os.Signal
repoLocks KeyedMutex

// For webhook registration
config *dashboardConfig.Config
Expand All @@ -50,7 +51,6 @@ type RepoCache struct {
type repoData struct {
repo *git.Repository
withHistory bool
lock sync.Mutex // lock this if you do git operation on this repo
}

const BRANCH_DELETED_WORKER_SUBPATH = "branch-deleted-worker"
Expand All @@ -73,6 +73,7 @@ func NewRepoCache(
clientHub: clientHub,
gitUser: gitUser,
triggerArtifactGeneration: triggerArtifactGeneration,
repoLocks: KeyedMutex{},
}

const DirRwxRxR = 0754
Expand Down Expand Up @@ -165,8 +166,8 @@ func (r *RepoCache) syncGitRepo(repoName string) {
opts.Depth = 0
}

repoData.lock.Lock()
defer repoData.lock.Unlock()
unlock := r.repoLocks.Lock(repoName)
defer unlock()

err := repo.Fetch(opts)
if err == git.NoErrAlreadyUpToDate {
Expand Down Expand Up @@ -221,44 +222,52 @@ func (r *RepoCache) cleanRepo(repoName string) {
}

func (r *RepoCache) PerformAction(repoName string, fn func(repo *git.Repository) error) error {
repo, err := r.instanceForRead(repoName, false)
if err != nil {
return err
}
unlock := r.repoLocks.Lock(repoName)
defer unlock()

repo.lock.Lock()
err = fn(repo.repo)
repo.lock.Unlock()
var repo *git.Repository
if existingRepoData, ok := r.repos[repoName]; ok {
repo = existingRepoData.repo
} else {
repoData, err := r.clone(repoName, false)
if err != nil {
return err
}
repo = repoData.repo
}

return err
return fn(repo)
}

func (r *RepoCache) PerformActionWithHistory(repoName string, fn func(repo *git.Repository)) error {
repo, err := r.instanceForRead(repoName, true)
repo.lock.Lock()
fn(repo.repo)
repo.lock.Unlock()
func (r *RepoCache) PerformActionWithHistory(
repoName string,
fn func(repo *git.Repository),
) error {
unlock := r.repoLocks.Lock(repoName)
defer unlock()

return err
}

func (r *RepoCache) instanceForRead(repoName string, withHistory bool) (repo *repoData, err error) {
var repoData repoData
var repo *git.Repository
if existingRepoData, ok := r.repos[repoName]; ok {
if withHistory && !existingRepoData.withHistory {
repoData, err = r.clone(repoName, withHistory)
repo = &repoData
go r.registerWebhook(repoName)
if !existingRepoData.withHistory {
repoData, err := r.clone(repoName, true)
if err != nil {
return err
}
repo = repoData.repo
} else {
repo = existingRepoData
repo = existingRepoData.repo
}
} else {
repoData, err = r.clone(repoName, withHistory)
repo = &repoData
go r.registerWebhook(repoName)
repoData, err := r.clone(repoName, false)
if err != nil {
return err
}
repo = repoData.repo
}

return repo, err
fn(repo)

return nil
}

func (r *RepoCache) InstanceForWrite(repoName string) (*git.Repository, string, error) {
Expand All @@ -275,23 +284,20 @@ func (r *RepoCache) instanceForWrite(repoName string, withHistory bool) (*git.Re
errors.WithMessage(err, "couldn't get temporary directory")
}

unlock := r.repoLocks.Lock(repoName)
defer unlock()

if repoData, ok := r.repos[repoName]; ok {
if withHistory && !repoData.withHistory {
_, err = r.clone(repoName, withHistory)
go r.registerWebhook(repoName)
_, err = r.clone(repoName, true)
}
} else {
_, err = r.clone(repoName, withHistory)
go r.registerWebhook(repoName)
}
if err != nil {
return nil, "", err
return nil, tmpPath, err
}

repoData := r.repos[repoName]
repoData.lock.Lock()
defer repoData.lock.Unlock()

repoPath := filepath.Join(r.config.RepoCachePath, strings.ReplaceAll(repoName, "/", "%"))
err = copy.Copy(repoPath, tmpPath)
if err != nil {
Expand All @@ -315,19 +321,12 @@ func (r *RepoCache) Invalidate(repoName string) {
r.syncGitRepo(repoName)
}

func (r *RepoCache) clone(repoName string, withHistory bool) (repoData, error) {
if repoName == "" {
return repoData{}, fmt.Errorf("repo name is mandatory")
}

func (r *RepoCache) clone(repoName string, withHistory bool) (*repoData, error) {
repoPath := filepath.Join(r.config.RepoCachePath, strings.ReplaceAll(repoName, "/", "%"))

r.reposMapLock.Lock()
defer r.reposMapLock.Unlock()
os.RemoveAll(repoPath)
err := os.MkdirAll(repoPath, Dir_RWX_RX_R)
if err != nil {
return repoData{}, errors.WithMessage(err, "couldn't create folder")
return nil, errors.WithMessage(err, "couldn't create folder")
}

var auth *http.BasicAuth
Expand All @@ -343,7 +342,7 @@ func (r *RepoCache) clone(repoName string, withHistory bool) (repoData, error) {
url = fmt.Sprintf("%s/%s", r.dynamicConfig.ScmURL(), repoName)
token, _, err := r.tokenManager.Token()
if err != nil {
return repoData{}, errors.WithMessage(err, "couldn't get scm token")
return nil, errors.WithMessage(err, "couldn't get scm token")
}
auth = &http.BasicAuth{
Username: "123",
Expand All @@ -363,7 +362,7 @@ func (r *RepoCache) clone(repoName string, withHistory bool) (repoData, error) {

repo, err := git.PlainClone(repoPath, false, opts)
if err != nil {
return repoData{}, errors.WithMessage(err, "couldn't clone")
return nil, errors.WithMessage(err, "couldn't clone")
}

err = repo.Fetch(&git.FetchOptions{
Expand All @@ -376,11 +375,16 @@ func (r *RepoCache) clone(repoName string, withHistory bool) (repoData, error) {
opts.Depth = 0
}
if err != nil && err != git.NoErrAlreadyUpToDate {
return repoData{}, errors.WithMessage(err, "couldn't fetch")
return nil, errors.WithMessage(err, "couldn't fetch")
}

go r.registerWebhook(repoName)

r.reposMapLock.Lock()
r.repos[repoName] = &repoData{repo: repo, withHistory: withHistory}
return repoData{repo: repo, withHistory: withHistory}, nil
r.reposMapLock.Unlock()

return r.repos[repoName], nil
}

func (r *RepoCache) registerWebhook(repoName string) {
Expand All @@ -407,3 +411,15 @@ func (r *RepoCache) registerWebhook(repoName string) {
logrus.Warnf("could not register webhook for %s: %s", repoName, err)
}
}

type KeyedMutex struct {
mutexes sync.Map // Zero value is empty and ready for use
}

func (m *KeyedMutex) Lock(key string) func() {
value, _ := m.mutexes.LoadOrStore(key, &sync.Mutex{})
mtx := value.(*sync.Mutex)
mtx.Lock()

return func() { mtx.Unlock() }
}
36 changes: 0 additions & 36 deletions web/dashboard/src/views/repo/repo.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,42 +415,6 @@ export default class Repo extends Component {
this.props.history.push(encodeURI(`/repo/${owner}/${repo}/envs/${env}/config/${config}/new`));
}

ciConfigAndShipperStatuses(repoName) {
const { repoMetas } = this.state;
const shipper = repoMetas.githubActionsShipper || repoMetas.circleCiShipper
let shipperColor = "text-gray-500";
let ciConfigColor = "text-gray-500";
let ciConfig = "";

if (repoMetas.githubActions) {
ciConfigColor = "text-green-500";
ciConfig = ".github/workflows"
} else if (repoMetas.circleCi) {
ciConfigColor = "text-green-500";
ciConfig = ".circleci"
}
if (shipper) {
shipperColor = "text-green-500";
}

return (
<>
<span title={repoMetas.githubActions || repoMetas.circleCi ? "This repository has CI config" : "This repository doesn't have CI config"}>
<a href={`${this.state.scmUrl}/${repoName}/tree/main/${ciConfig}`} target="_blank" rel="noopener noreferrer" className={(!repoMetas.githubActions && !repoMetas.circleCi) ? "cursor-default pointer-events-none" : ""}>
<svg xmlns="http://www.w3.org/2000/svg" className={`inline ml-1 h-4 w-4 ${ciConfigColor}`} fill="none" viewBox="0 0 24 24" stroke="currentColor" strokeWidth={2}>
<path strokeLinecap="round" strokeLinejoin="round" d="M9 12l2 2 4-4m6 2a9 9 0 11-18 0 9 9 0 0118 0z" />
</svg>
</a>
</span>
<span title={shipper ? "This repository has shipper" : "This repository doesn't have shipper"}>
<a href={`${this.state.scmUrl}/${repoName}/tree/main/${ciConfig}/${shipper}`} target="_blank" rel="noopener noreferrer" className={(!repoMetas.githubActionsShipper && !repoMetas.circleCiShipper) ? "cursor-default pointer-events-none" : ""}>
<svg xmlns="http://www.w3.org/2000/svg" className={`inline ml-1 h-4 w-4 ${shipperColor}`} fill="none" viewBox="0 0 24 24" stroke="currentColor" strokeWidth={2}>
<path strokeLinecap="round" strokeLinejoin="round" d="M9 12l2 2 4-4M7.835 4.697a3.42 3.42 0 001.946-.806 3.42 3.42 0 014.438 0 3.42 3.42 0 001.946.806 3.42 3.42 0 013.138 3.138 3.42 3.42 0 00.806 1.946 3.42 3.42 0 010 4.438 3.42 3.42 0 00-.806 1.946 3.42 3.42 0 01-3.138 3.138 3.42 3.42 0 00-1.946.806 3.42 3.42 0 01-4.438 0 3.42 3.42 0 00-1.946-.806 3.42 3.42 0 01-3.138-3.138 3.42 3.42 0 00-.806-1.946 3.42 3.42 0 010-4.438 3.42 3.42 0 00.806-1.946 3.42 3.42 0 013.138-3.138z" /> </svg>
</a>
</span>
</>)
}

fileMetasByEnv(envName) {
return this.state.fileInfos.filter(fileInfo => fileInfo.envName === envName)
}
Expand Down
Loading