Skip to content

Commit

Permalink
Fixes #19398: Git error when deleting a node or archiving everything,…
Browse files Browse the repository at this point in the history
… and very slow git
  • Loading branch information
fanf authored and Jenkins CI committed Sep 2, 2021
1 parent 05e9f51 commit e536797
Show file tree
Hide file tree
Showing 4 changed files with 178 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,13 @@ import org.eclipse.jgit.api.Git
import org.eclipse.jgit.lib.Repository
import org.eclipse.jgit.lib.ObjectId
import com.normation.errors._
import zio.Semaphore

/**
* A service that gives access to the Git
* porcelain API of the repository.
*
* For reference about how to use JGit: https://github.com/centic9/jgit-cookbook
*/
trait GitRepositoryProvider {
/**
Expand All @@ -54,6 +57,14 @@ trait GitRepositoryProvider {
def git: Git

def db: Repository

/*
* Git (and JGit) are not thread safe. When there is two concurrent write operations, we can get a
* `JGitInternalException: Exception caught during execution of add command` (which is not very
* informative - see https://issues.rudder.io/issues/19398).
* So we need to protect at the repository level write operation with a semaphore.
*/
def semaphore: Semaphore
}

object GitRepositoryLogger extends NamedZioLogger() { def loggerName = "git-repository" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import com.normation.zio._
*/
class GitRepositoryProviderImpl(override val db: Repository) extends GitRepositoryProvider { //we expect to have a .git here
override val git = new Git(db)
override val semaphore = Semaphore.make(1).runNow
}

object GitRepositoryProviderImpl {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ import zio._
import scala.jdk.CollectionConverters._
import scala.util.control.NonFatal
import scala.xml.Elem
import com.normation.zio._

/**
* Utility trait that factor out file commits.
Expand All @@ -70,12 +69,6 @@ trait GitArchiverUtils {
}
}

// semaphores to replace `synchronized`
val semaphoreAdd = Semaphore.make(1).runNow
val semaphoreMove = Semaphore.make(1).runNow
val semaphoreDelete = Semaphore.make(1).runNow


def gitRepo : GitRepositoryProvider
def gitRootDirectory : File
def relativePath : String
Expand Down Expand Up @@ -123,7 +116,7 @@ trait GitArchiverUtils {
* commitMessage is used for the message of the commit.
*/
def commitAddFile(modId : ModificationId, commiter:PersonIdent, gitPath:String, commitMessage:String) : IOResult[GitCommitId] = {
semaphoreAdd.withPermit(
gitRepo.semaphore.withPermit(
for {
_ <- GitArchiveLoggerPure.debug(s"Add file '${gitPath}' from configuration repository")
add <- IOResult.effect(gitRepo.git.add.addFilepattern(gitPath).call)
Expand All @@ -147,7 +140,7 @@ trait GitArchiverUtils {
* commitMessage is used for the message of the commit.
*/
def commitRmFile(modId : ModificationId, commiter:PersonIdent, gitPath:String, commitMessage:String) : IOResult[GitCommitId] = {
semaphoreDelete.withPermit(
gitRepo.semaphore.withPermit(
for {
_ <- GitArchiveLoggerPure.debug(s"remove file '${gitPath}' from configuration repository")
rm <- IOResult.effect(gitRepo.git.rm.addFilepattern(gitPath).call)
Expand All @@ -173,7 +166,7 @@ trait GitArchiverUtils {
* commitMessage is used for the message of the commit.
*/
def commitMvDirectory(modId : ModificationId, commiter:PersonIdent, oldGitPath:String, newGitPath:String, commitMessage:String) : IOResult[GitCommitId] = {
semaphoreMove.withPermit(
gitRepo.semaphore.withPermit(
for {
_ <- GitArchiveLoggerPure.debug(s"move file '${oldGitPath}' from configuration repository to '${newGitPath}'")
update <- IOResult.effect {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/*
*************************************************************************************
* Copyright 2021 Normation SAS
*************************************************************************************
*
* This file is part of Rudder.
*
* Rudder is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* In accordance with the terms of section 7 (7. Additional Terms.) of
* the GNU General Public License version 3, the copyright holders add
* the following Additional permissions:
* Notwithstanding to the terms of section 5 (5. Conveying Modified Source
* Versions) and 6 (6. Conveying Non-Source Forms.) of the GNU General
* Public License version 3, when you create a Related Module, this
* Related Module is not considered as a part of the work and may be
* distributed under the license agreement of your choice.
* A "Related Module" means a set of sources files including their
* documentation that, without modification of the Source Code, enables
* supplementary functions or services in addition to those offered by
* the Software.
*
* Rudder is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with Rudder. If not, see <http://www.gnu.org/licenses/>.
*
*************************************************************************************
*/

package com.normation.cfclerk.services

import better.files.File
import com.normation.cfclerk.services.impl.GitRepositoryProviderImpl
import com.normation.errors.IOResult
import com.normation.errors.Inconsistency
import com.normation.errors.effectUioUnit
import com.normation.eventlog.ModificationId
import com.normation.rudder.db.DB
import com.normation.rudder.repository.GitCommitId
import com.normation.rudder.repository.GitModificationRepository
import com.normation.rudder.repository.xml.GitArchiverUtils
import com.normation.rudder.repository.xml.RudderPrettyPrinter
import org.apache.commons.io.FileUtils
import org.junit.runner.RunWith
import org.specs2.mutable.Specification
import org.specs2.runner.JUnitRunner
import org.specs2.specification.AfterAll
import net.liftweb.common.Loggable
import org.joda.time.DateTime
import com.normation.zio._
import org.eclipse.jgit.lib.PersonIdent
import org.eclipse.jgit.lib.Repository
import org.eclipse.jgit.revwalk.RevWalk
import zio.syntax._
import zio._

import scala.util.Random

/**
* Details of tests executed in each instances of
* the test.
* To see values for gitRoot, ptLib, etc, see at the end
* of that file.
*/
@RunWith(classOf[JUnitRunner])
class JGitRepositoryTest extends Specification with Loggable with AfterAll {

val gitRoot = File("/tmp/test-jgit-" + DateTime.now().toString())

// Set sequential execution
sequential

/**
* Add a switch to be able to see tmp files (not clean themps) with
* -Dtests.clean.tmp=false
*/
override def afterAll() = {
if(System.getProperty("tests.clean.tmp") != "false") {
logger.info("Deleting directory " + gitRoot.pathAsString)
FileUtils.deleteDirectory(gitRoot.toJava)
}
}

gitRoot.createDirectories()

val repo = GitRepositoryProviderImpl.make(gitRoot.pathAsString).runNow
val archive = new GitArchiverUtils {
override val gitRepo = repo
override val gitRootDirectory = gitRoot.toJava
override def relativePath: String = ""
override def xmlPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2)
override def encoding: String = "UTF-8"
override def gitModificationRepository: GitModificationRepository = new GitModificationRepository {
override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed
override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = DB.GitCommitJoin(commit, modId).succeed
}
}

// listing files at a commit is complicated

import org.eclipse.jgit.treewalk.TreeWalk

def readElementsAt(repository: Repository, commit: String) = {
val ref = repository.findRef(commit)

// a RevWalk allows to walk over commits based on some filtering that is defined
val walkM = ZManaged.make(IOResult.effect(new RevWalk(repository)))(x => effectUioUnit(x.close()))
val treeWalkM = ZManaged.make(IOResult.effect(new TreeWalk(repository)))(x => effectUioUnit(x.close()))

walkM.use(walk =>
for {
commit <- IOResult.effect(walk.parseCommit(ref.getObjectId))
tree <- IOResult.effect(commit.getTree)
res <- treeWalkM.use{treeWalk =>
treeWalk.setRecursive(true) // ok, can't throw exception

IOResult.effect(treeWalk.addTree(tree)) *>
ZIO.loop(treeWalk)(_.next, identity)(x => IOResult.effect(x.getPathString))
}
} yield res
)
}

"The test lib" should {
"not throw JGitInternalError on concurrent write" in {

// you can remove `gitRepo.semaphore.withPermit` in `commitAddFile`
// to check that you get the JGitInternalException

val actor = new PersonIdent("test", "test@test.com")

def getName(length: Int) = {
if(length < 1) Inconsistency("Length must be positive").fail
else {
IOResult.effect("")(Random.alphanumeric.take(length).toList.mkString(""))
}
}
def add(i: Int) = (for {
name <- getName(8).map(s => i.toString + "_" + s)
file = gitRoot / name
f <- IOResult.effect(file.write("something in " + name))
_ <- archive.commitAddFile(ModificationId(name), actor, name, "add " + name)
} yield (name))

logger.debug(s"Commiting files in: " + gitRoot.pathAsString)
val files = ZIO.foreachParN(16)(1 to 50) { i => add(i) }.runNow

val created = readElementsAt(repo.db, "refs/heads/master").runNow
created must containTheSameElementsAs(files)

}
}

}

0 comments on commit e536797

Please sign in to comment.