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

Jump to bottom

fix(RepositoryUpdater): make sure temp directories are deleted #2010

Merged
merged 5 commits into from Mar 4, 2022
Merged

fix(RepositoryUpdater): make sure temp directories are deleted #2010

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60c5ab1
fix(RepositoryUpdater): delete temp directories
mpro7 Mar 2, 2022
ba11afb
Merge branch 'main' into DEV-559-api-doesnt-cleanup-temp-folder-after…
mpro7 Mar 2, 2022
21626cb
review changes
mpro7 Mar 2, 2022
881cf90
remove unused code
mpro7 Mar 4, 2022
b5bfab3
Merge branch 'main' into DEV-559-api-doesnt-cleanup-temp-folder-after…
mpro7 Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 26 additions & 4 deletions webapi/src/main/scala/org/knora/webapi/store/triplestore/upgrade/RepositoryUpdater.scala
View file
Open in desktop
@@ -1,6 +1,8 @@
package org.knora.webapi.store.triplestore.upgrade

import java.nio.file.{Files, Path, Paths}
import java.io.File
import scala.reflect.io.Directory

import akka.actor.{ActorRef, ActorSystem}
import akka.http.scaladsl.util.FastFuture
Expand Down Expand Up @@ -72,6 +74,24 @@ class RepositoryUpdater(
log = log
)

private val tempDirNamePrefix: String = "knora"

/**
* Deletes directories inside temp directory starting with `tempDirNamePrefix`.
*/
def deleteTempDirectories(): Unit = {
val rootDir = new File("/tmp/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • You are missing a check for settings.upgradeDownloadDir. If this is set, then the temp directory might not be tmp. It is a valid setting that could be used in production.
  • Also, this will probably not work under Windows. You should use the Paths.get() to have a correct path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think adding that check doesn't make sense since that part of code is commented out and settings.upgradeDownloadDir in this check will always lead to None option, which goes then to deleteTempDirectories().

Hmmm, is the Paths.get() suggestion because Windows uses backslashes?

Copy link
Collaborator Author

@mpro7 mpro7 Mar 4, 2022 •
edited

Choose a reason for hiding this comment

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

@subotic I've removed unused code in 881cf90. I don't know why it was commented out, but if my implementation will not work, it can be brought back. That means maybe instead of creating random folders, only one would be used.

I've tried out Paths.get() and Path.of(). Although all compiles, test-repository-upgrade hangs on last script wait-for-knora.sh. Once again, if this implementation somehow fail, idea described above, should make possible to use either Paths.get() or Path.of().

And tmp is default temp folder defined in docker-compose.

Copy link
Collaborator

@subotic subotic Mar 4, 2022 •
edited

Choose a reason for hiding this comment

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

please don't merge a PR if the discussion is not finished.

The locally defined docker-compose.yml is irrelevant. The app is deployed in production with a different set of settings and another docker-compose.yml. Instead of adding 2 lines of code, you are now risking to break stuff in production. Not the right attitude in my opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sorry but thought you are not going to reply and because already accepting this PR there isn't any critical issue here.

If you feel so, this changes could be reverted. But this isn't deployed directly to the production, we have TEST and STAGE too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just checked PROD configuration and apparently /tmp:/tmp volume is also defined in docker-compose.yml, /tmp/ folder exists and contains knora* update directories to remove.

val getTempToDelete = rootDir.listFiles.filter(_.getName.startsWith(tempDirNamePrefix))

if (getTempToDelete.length != 0) {
getTempToDelete.foreach(dir => {
val dirToDelete = new Directory(dir)
dirToDelete.deleteRecursively()
})
log.info(s"Deleted temp directories: ${getTempToDelete.map(_.getName()).mkString(", ")}")
}
}

/**
* Updates the repository, if necessary, to work with the current version of Knora.
*
Expand All @@ -83,18 +103,20 @@ class RepositoryUpdater(
requiredRepositoryVersion = org.knora.webapi.KnoraBaseVersion

// Is the repository up to date?
repositoryUpToData = foundRepositoryVersion.contains(requiredRepositoryVersion)
repositoryUpToDate: Boolean = foundRepositoryVersion.contains(requiredRepositoryVersion)

repositoryUpdatedResponse: RepositoryUpdatedResponse <-
if (repositoryUpToData) {
if (repositoryUpToDate) {
// Yes. Nothing more to do.
FastFuture.successful(RepositoryUpdatedResponse(s"Repository is up to date at $requiredRepositoryVersion"))
} else {
// No. Construct the list of updates that it needs.

log.info(
s"Repository not up-to-date. Found: ${foundRepositoryVersion.getOrElse("None")}, Required: $requiredRepositoryVersion"
)

deleteTempDirectories()

val selectedPlugins: Seq[PluginForKnoraBaseVersion] = selectPluginsForNeededUpdates(foundRepositoryVersion)
log.info(s"Updating repository with transformations: ${selectedPlugins.map(_.versionString).mkString(", ")}")

Expand Down Expand Up @@ -175,7 +197,7 @@ class RepositoryUpdater(

case None =>
// No. Create a temporary directory.
val dirFile = Files.createTempDirectory("knora")
val dirFile = Files.createTempDirectory(tempDirNamePrefix)
log.info(s"Repository update using download directory $dirFile")
dirFile
}
Expand Down