From 324beb4190796db914aa700f17adb2a52297f230 Mon Sep 17 00:00:00 2001 From: kartik-579 <84493919+kartik-579@users.noreply.github.com> Date: Mon, 14 Aug 2023 15:11:05 +0530 Subject: [PATCH 1/2] updated lock/unlock for concurrent request handling, user update (#3760) --- pkg/user/UserService.go | 64 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/pkg/user/UserService.go b/pkg/user/UserService.go index 73a7f795ebf..49b89149eb0 100644 --- a/pkg/user/UserService.go +++ b/pkg/user/UserService.go @@ -36,9 +36,15 @@ import ( "go.uber.org/zap" "net/http" "strings" + "sync" "time" ) +const ( + ConcurrentRequestLockError = "there is an ongoing request for this user, please try after some time" + ConcurrentRequestUnlockError = "cannot block request that is not in process" +) + type UserService interface { CreateUser(userInfo *bean.UserInfo, token string, managerAuth func(resource, token string, object string) bool) ([]*bean.UserInfo, error) SelfRegisterUserIfNotExists(userInfo *bean.UserInfo) ([]*bean.UserInfo, error) @@ -62,6 +68,10 @@ type UserService interface { } type UserServiceImpl struct { + userReqLock sync.RWMutex + //map of userId and current lock-state of their serving ability; + //if TRUE then it means that some request is ongoing & unable to serve and FALSE then it is open to serve + userReqState map[int32]bool userAuthRepository repository2.UserAuthRepository logger *zap.SugaredLogger userRepository repository2.UserRepository @@ -77,6 +87,7 @@ func NewUserServiceImpl(userAuthRepository repository2.UserAuthRepository, userGroupRepository repository2.RoleGroupRepository, sessionManager2 *middleware.SessionManager, userCommonService UserCommonService, userAuditService UserAuditService) *UserServiceImpl { serviceImpl := &UserServiceImpl{ + userReqState: make(map[int32]bool), userAuthRepository: userAuthRepository, logger: logger, userRepository: userRepository, @@ -89,6 +100,36 @@ func NewUserServiceImpl(userAuthRepository repository2.UserAuthRepository, return serviceImpl } +func (impl *UserServiceImpl) getUserReqLockStateById(userId int32) bool { + defer impl.userReqLock.RUnlock() + impl.userReqLock.RLock() + return impl.userReqState[userId] +} + +// FreeUnfreeUserReqState - free sets the userId free for serving, meaning removing the lock(removing entry). Unfree locks the user for other requests +func (impl *UserServiceImpl) lockUnlockUserReqState(userId int32, lock bool) error { + var err error + defer impl.userReqLock.Unlock() + impl.userReqLock.Lock() + if lock { + //checking again if someone changed or not + if !impl.userReqState[userId] { + //available to serve, locking + impl.userReqState[userId] = true + } else { + err = &util.ApiError{Code: "409", HttpStatusCode: http.StatusConflict, UserMessage: ConcurrentRequestLockError} + } + } else { + if impl.userReqState[userId] { + //in serving state, unlocking + delete(impl.userReqState, userId) + } else { + err = &util.ApiError{Code: "409", HttpStatusCode: http.StatusConflict, UserMessage: ConcurrentRequestUnlockError} + } + } + return err +} + func (impl *UserServiceImpl) validateUserRequest(userInfo *bean.UserInfo) (bool, error) { if len(userInfo.RoleFilters) == 1 && userInfo.RoleFilters[0].Team == "" && userInfo.RoleFilters[0].Environment == "" && userInfo.RoleFilters[0].Action == "" { @@ -617,6 +658,23 @@ func (impl *UserServiceImpl) mergeGroups(oldGroups []string, newGroups []string) } func (impl *UserServiceImpl) UpdateUser(userInfo *bean.UserInfo, token string, managerAuth func(resource, token string, object string) bool) (*bean.UserInfo, bool, bool, []string, error) { + //checking if request for same user is being processed + isLocked := impl.getUserReqLockStateById(userInfo.Id) + if isLocked { + impl.logger.Errorw("received concurrent request for user update, UpdateUser", "userId", userInfo.Id) + return nil, false, false, nil, &util.ApiError{ + Code: "409", + HttpStatusCode: http.StatusConflict, + UserMessage: ConcurrentRequestLockError, + } + } else { + //locking state for this user since it's ready to serve + err := impl.lockUnlockUserReqState(userInfo.Id, true) + if err != nil { + impl.logger.Errorw("error in locking, lockUnlockUserReqState", "userId", userInfo.Id) + return nil, false, false, nil, err + } + } //validating if action user is not admin and trying to update user who has super admin polices, return 403 isUserSuperAdmin, err := impl.IsSuperAdmin(int(userInfo.Id)) if err != nil { @@ -802,6 +860,12 @@ func (impl *UserServiceImpl) UpdateUser(userInfo *bean.UserInfo, token string, m //loading policy for syncing orchestrator to casbin with newly added policies casbin2.LoadPolicy() + err = impl.lockUnlockUserReqState(userInfo.Id, false) + if err != nil { + impl.logger.Errorw("error in unlocking, lockUnlockUserReqState", "userId", userInfo.Id) + return nil, false, false, nil, err + } + return userInfo, rolesChanged, groupsModified, restrictedGroups, nil } From 01ff25cf355b625566ce1324a23682fd96c7ebc9 Mon Sep 17 00:00:00 2001 From: Shivam Nagar <124123645+Shivam-nagar23@users.noreply.github.com> Date: Mon, 14 Aug 2023 15:11:40 +0530 Subject: [PATCH 2/2] fix: Intermittent helm apps deployment fail deployed through gitops (#3756) * pushed requirements.yaml file while pushing chart * pushed values.yaml file while pushing chart * Order of return parameters * Order of return parameters * review comments * review comments --- .../common/AppStoreDeploymentCommonService.go | 124 ++++++++++++------ pkg/appStore/util/util.go | 11 ++ 2 files changed, 93 insertions(+), 42 deletions(-) create mode 100644 pkg/appStore/util/util.go diff --git a/pkg/appStore/deployment/common/AppStoreDeploymentCommonService.go b/pkg/appStore/deployment/common/AppStoreDeploymentCommonService.go index ad89f67c1d5..c5c027d5045 100644 --- a/pkg/appStore/deployment/common/AppStoreDeploymentCommonService.go +++ b/pkg/appStore/deployment/common/AppStoreDeploymentCommonService.go @@ -26,6 +26,7 @@ import ( appStoreBean "github.com/devtron-labs/devtron/pkg/appStore/bean" "github.com/devtron-labs/devtron/pkg/appStore/deployment/repository" appStoreDiscoverRepository "github.com/devtron-labs/devtron/pkg/appStore/discover/repository" + util2 "github.com/devtron-labs/devtron/pkg/appStore/util" repository2 "github.com/devtron-labs/devtron/pkg/cluster/repository" "github.com/go-pg/pg" "github.com/google/go-github/github" @@ -49,7 +50,7 @@ type AppStoreDeploymentCommonService interface { ParseGitRepoErrorResponse(err error) (bool, error) GetValuesAndRequirementGitConfig(installAppVersionRequest *appStoreBean.InstallAppVersionDTO) (*util.ChartConfig, *util.ChartConfig, error) CreateChartProxyAndGetPath(installAppVersionRequest *appStoreBean.InstallAppVersionDTO) (*util.ChartCreateResponse, error) - CreateGitOpsRepoAndPushChart(installAppVersionRequest *appStoreBean.InstallAppVersionDTO, builtChartPath string) (*util.ChartGitAttribute, error) + CreateGitOpsRepoAndPushChart(installAppVersionRequest *appStoreBean.InstallAppVersionDTO, builtChartPath string, requirementsConfig *util.ChartConfig, valuesConfig *util.ChartConfig) (*util.ChartGitAttribute, bool, string, error) CommitConfigToGit(chartConfig *util.ChartConfig) (gitHash string, err error) GetGitCommitConfig(installAppVersionRequest *appStoreBean.InstallAppVersionDTO, fileString string, filename string) (*util.ChartConfig, error) GetValuesString(chartName, valuesOverrideYaml string) (string, error) @@ -380,7 +381,7 @@ func (impl AppStoreDeploymentCommonServiceImpl) GenerateManifest(installAppVersi //} // CreateGitOpsRepo creates a gitOps repo with readme -func (impl AppStoreDeploymentCommonServiceImpl) CreateGitOpsRepo(installAppVersionRequest *appStoreBean.InstallAppVersionDTO) (string, error) { +func (impl AppStoreDeploymentCommonServiceImpl) CreateGitOpsRepo(installAppVersionRequest *appStoreBean.InstallAppVersionDTO) (string, bool, error) { if len(installAppVersionRequest.GitOpsRepoName) == 0 { //here gitops repo will be the app name, to breaking the mono repo structure @@ -393,7 +394,7 @@ func (impl AppStoreDeploymentCommonServiceImpl) CreateGitOpsRepo(installAppVersi err = nil gitOpsConfigBitbucket.BitBucketWorkspaceId = "" } else { - return "", err + return "", false, err } } //getting user name & emailId for commit author data @@ -406,18 +407,18 @@ func (impl AppStoreDeploymentCommonServiceImpl) CreateGitOpsRepo(installAppVersi BitBucketWorkspaceId: gitOpsConfigBitbucket.BitBucketWorkspaceId, BitBucketProjectKey: gitOpsConfigBitbucket.BitBucketProjectKey, } - repoUrl, _, detailedError := impl.gitFactory.Client.CreateRepository(gitRepoRequest) + repoUrl, isNew, detailedError := impl.gitFactory.Client.CreateRepository(gitRepoRequest) for _, err := range detailedError.StageErrorMap { if err != nil { impl.logger.Errorw("error in creating git project", "name", installAppVersionRequest.GitOpsRepoName, "err", err) - return "", err + return "", false, err } } - return repoUrl, err + return repoUrl, isNew, err } // PushChartToGitopsRepo pushes built chart to gitOps repo -func (impl AppStoreDeploymentCommonServiceImpl) PushChartToGitopsRepo(PushChartToGitRequest *appStoreBean.PushChartToGitRequestDTO) (*util.ChartGitAttribute, error) { +func (impl AppStoreDeploymentCommonServiceImpl) PushChartToGitopsRepo(PushChartToGitRequest *appStoreBean.PushChartToGitRequestDTO, requirementsConfig *util.ChartConfig, valuesConfig *util.ChartConfig) (*util.ChartGitAttribute, string, error) { space := regexp.MustCompile(`\s+`) appStoreName := space.ReplaceAllString(PushChartToGitRequest.ChartAppStoreName, "-") chartDir := fmt.Sprintf("%s-%s", PushChartToGitRequest.AppName, impl.chartTemplateService.GetDir()) @@ -426,12 +427,12 @@ func (impl AppStoreDeploymentCommonServiceImpl) PushChartToGitopsRepo(PushChartT clonedDir, err = impl.gitFactory.GitService.Clone(PushChartToGitRequest.RepoURL, chartDir) if err != nil { impl.logger.Errorw("error in cloning repo", "url", PushChartToGitRequest.RepoURL, "err", err) - return nil, err + return nil, "", err } } else { err = impl.chartTemplateService.GitPull(clonedDir, PushChartToGitRequest.RepoURL, appStoreName) if err != nil { - return nil, err + return nil, "", err } } acdAppName := fmt.Sprintf("%s-%s", PushChartToGitRequest.AppName, PushChartToGitRequest.EnvName) @@ -439,12 +440,22 @@ func (impl AppStoreDeploymentCommonServiceImpl) PushChartToGitopsRepo(PushChartT err := os.MkdirAll(dir, os.ModePerm) if err != nil { impl.logger.Errorw("error in making dir", "err", err) - return nil, err + return nil, "", err } err = dirCopy.Copy(PushChartToGitRequest.TempChartRefDir, dir) if err != nil { impl.logger.Errorw("error copying dir", "err", err) - return nil, err + return nil, "", err + } + err = impl.AddConfigFileToChart(requirementsConfig, dir, clonedDir) + if err != nil { + impl.logger.Errorw("error in adding requirements.yaml to chart", "err", err, "appName", PushChartToGitRequest.AppName) + return nil, "", err + } + err = impl.AddConfigFileToChart(valuesConfig, dir, clonedDir) + if err != nil { + impl.logger.Errorw("error in adding values.yaml to chart", "err", err, "appName", PushChartToGitRequest.AppName) + return nil, "", err } userEmailId, userName := impl.chartTemplateService.GetUserEmailIdAndNameForGitOpsCommit(PushChartToGitRequest.UserId) commit, err := impl.gitFactory.GitService.CommitAndPushAllChanges(clonedDir, "first commit", userName, userEmailId) @@ -453,38 +464,62 @@ func (impl AppStoreDeploymentCommonServiceImpl) PushChartToGitopsRepo(PushChartT impl.logger.Warn("re-trying, taking pull and then push again") err = impl.chartTemplateService.GitPull(clonedDir, PushChartToGitRequest.RepoURL, acdAppName) if err != nil { - return nil, err + impl.logger.Errorw("error in git pull", "err", err, "appName", acdAppName) + return nil, "", err } err = dirCopy.Copy(PushChartToGitRequest.TempChartRefDir, dir) if err != nil { impl.logger.Errorw("error copying dir", "err", err) - return nil, err + return nil, "", err } commit, err = impl.gitFactory.GitService.CommitAndPushAllChanges(clonedDir, "first commit", userName, userEmailId) if err != nil { impl.logger.Errorw("error in pushing git", "err", err) - return nil, err + return nil, "", err } } impl.logger.Debugw("template committed", "url", PushChartToGitRequest.RepoURL, "commit", commit) defer impl.chartTemplateService.CleanDir(clonedDir) - return &util.ChartGitAttribute{RepoUrl: PushChartToGitRequest.RepoURL, ChartLocation: filepath.Join("", acdAppName)}, nil + return &util.ChartGitAttribute{RepoUrl: PushChartToGitRequest.RepoURL, ChartLocation: acdAppName}, commit, err +} + +// AddConfigFileToChart will override requirements.yaml file in chart +func (impl AppStoreDeploymentCommonServiceImpl) AddConfigFileToChart(config *util.ChartConfig, dir string, clonedDir string) error { + filePath := filepath.Join(clonedDir, config.FileName) + file, err := os.Create(filePath) + if err != nil { + impl.logger.Errorw("error in creating file", "err", err, "fileName", config.FileName) + return err + } + defer file.Close() + _, err = file.Write([]byte(config.FileContent)) + if err != nil { + impl.logger.Errorw("error in writing file content", "err", err, "fileName", config.FileName) + return err + } + destinationFilePath := filepath.Join(dir, config.FileName) + err = util2.MoveFileToDestination(filePath, destinationFilePath) + if err != nil { + impl.logger.Errorw("error in moving file from source to destination", "err", err) + return err + } + return nil } // CreateGitOpsRepoAndPushChart is a wrapper for creating gitops repo and pushing chart to created repo -func (impl AppStoreDeploymentCommonServiceImpl) CreateGitOpsRepoAndPushChart(installAppVersionRequest *appStoreBean.InstallAppVersionDTO, builtChartPath string) (*util.ChartGitAttribute, error) { - repoURL, err := impl.CreateGitOpsRepo(installAppVersionRequest) +func (impl AppStoreDeploymentCommonServiceImpl) CreateGitOpsRepoAndPushChart(installAppVersionRequest *appStoreBean.InstallAppVersionDTO, builtChartPath string, requirementsConfig *util.ChartConfig, valuesConfig *util.ChartConfig) (*util.ChartGitAttribute, bool, string, error) { + repoURL, isNew, err := impl.CreateGitOpsRepo(installAppVersionRequest) if err != nil { impl.logger.Errorw("Error in creating gitops repo for ", "appName", installAppVersionRequest.AppName, "err", err) - return nil, err + return nil, false, "", err } pushChartToGitRequest := ParseChartGitPushRequest(installAppVersionRequest, repoURL, builtChartPath) - chartGitAttribute, err := impl.PushChartToGitopsRepo(pushChartToGitRequest) + chartGitAttribute, commitHash, err := impl.PushChartToGitopsRepo(pushChartToGitRequest, requirementsConfig, valuesConfig) if err != nil { impl.logger.Errorw("error in pushing chart to git", "err", err) - return nil, err + return nil, false, "", err } - return chartGitAttribute, err + return chartGitAttribute, isNew, commitHash, err } // CommitConfigToGit is used for committing values.yaml and requirements.yaml file config @@ -508,34 +543,39 @@ func (impl AppStoreDeploymentCommonServiceImpl) CommitConfigToGit(chartConfig *u func (impl AppStoreDeploymentCommonServiceImpl) GitOpsOperations(manifestResponse *AppStoreManifestResponse, installAppVersionRequest *appStoreBean.InstallAppVersionDTO) (*AppStoreGitOpsResponse, error) { appStoreGitOpsResponse := &AppStoreGitOpsResponse{} - chartGitAttribute, err := impl.CreateGitOpsRepoAndPushChart(installAppVersionRequest, manifestResponse.ChartResponse.BuiltChartPath) + chartGitAttribute, isNew, githash, err := impl.CreateGitOpsRepoAndPushChart(installAppVersionRequest, manifestResponse.ChartResponse.BuiltChartPath, manifestResponse.RequirementsConfig, manifestResponse.ValuesConfig) if err != nil { impl.logger.Errorw("Error in pushing chart to git", "err", err) return appStoreGitOpsResponse, err } - // step-2 commit dependencies and values in git - _, err = impl.CommitConfigToGit(manifestResponse.RequirementsConfig) - if err != nil { - impl.logger.Errorw("error in committing dependency config to git", "err", err) - return appStoreGitOpsResponse, err - } space := regexp.MustCompile(`\s+`) appStoreName := space.ReplaceAllString(installAppVersionRequest.AppName, "-") clonedDir := impl.gitFactory.GitWorkingDir + "" + appStoreName - err = impl.chartTemplateService.GitPull(clonedDir, chartGitAttribute.RepoUrl, appStoreName) - if err != nil { - impl.logger.Errorw("error in git pull", "err", err) - return appStoreGitOpsResponse, err - } - githash, err := impl.CommitConfigToGit(manifestResponse.ValuesConfig) - if err != nil { - impl.logger.Errorw("error in committing values config to git", "err", err) - return appStoreGitOpsResponse, err - } - err = impl.chartTemplateService.GitPull(clonedDir, chartGitAttribute.RepoUrl, appStoreName) - if err != nil { - impl.logger.Errorw("error in git pull", "err", err) - return appStoreGitOpsResponse, err + + // Checking this is the first time chart has been pushed , if yes requirements.yaml has been already pushed with chart as there was sync-delay with github api. + // step-2 commit dependencies and values in git + if !isNew { + _, err = impl.CommitConfigToGit(manifestResponse.RequirementsConfig) + if err != nil { + impl.logger.Errorw("error in committing dependency config to git", "err", err) + return appStoreGitOpsResponse, err + } + err = impl.chartTemplateService.GitPull(clonedDir, chartGitAttribute.RepoUrl, appStoreName) + if err != nil { + impl.logger.Errorw("error in git pull", "err", err) + return appStoreGitOpsResponse, err + } + + githash, err = impl.CommitConfigToGit(manifestResponse.ValuesConfig) + if err != nil { + impl.logger.Errorw("error in committing values config to git", "err", err) + return appStoreGitOpsResponse, err + } + err = impl.chartTemplateService.GitPull(clonedDir, chartGitAttribute.RepoUrl, appStoreName) + if err != nil { + impl.logger.Errorw("error in git pull", "err", err) + return appStoreGitOpsResponse, err + } } appStoreGitOpsResponse.ChartGitAttribute = chartGitAttribute appStoreGitOpsResponse.GitHash = githash diff --git a/pkg/appStore/util/util.go b/pkg/appStore/util/util.go new file mode 100644 index 00000000000..7367f847028 --- /dev/null +++ b/pkg/appStore/util/util.go @@ -0,0 +1,11 @@ +package util + +import "os" + +func MoveFileToDestination(filePath, destinationPath string) error { + err := os.Rename(filePath, destinationPath) + if err != nil { + return err + } + return nil +}