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.

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

Added basic readiness check for registry API #2997

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- Fixed: some helm chart (gateway & web) deployment doesn't allow setting `replicas` value properly
- Added an opt-in for chart/map/table previews if files are large.
- Upgraded helm-docs to v1.2.1
- Added basic readiness check for registry API

## 0.0.57

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,18 @@ class Api(

val routes = handleExceptions(myExceptionHandler) {
pathPrefix("v0") {
path("ping") { complete("OK") } ~
path("ping") {
complete("OK")
} ~
pathPrefix("status") {
path("live") { complete("OK") } ~
path("ready") { complete(ReadyStatus(true)) }
new ProbesService(
config,
authApiClient,
system,
materializer,
recordPersistence,
eventPersistence
).route
} ~ roleDependentRoutes
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package au.csiro.data61.magda.registry

import java.util.concurrent.TimeoutException

import akka.actor.{ActorRef, ActorSystem}
import akka.event.Logging
import akka.http.scaladsl.model.StatusCodes
import akka.http.scaladsl.server.Directives._
import akka.http.scaladsl.server.Route
import akka.stream.Materializer
import au.csiro.data61.magda.client.AuthApiClient
import au.csiro.data61.magda.directives.AuthDirectives.requireIsAdmin
import au.csiro.data61.magda.directives.TenantDirectives.{
requiresTenantId,
requiresSpecifiedTenantId
}
import au.csiro.data61.magda.model.Registry._
import com.typesafe.config.Config
import gnieh.diffson.sprayJson._
import io.swagger.annotations._
import javax.ws.rs.Path
import scalikejdbc.DB
import org.everit.json.schema.ValidationException

import scala.concurrent.{Await, ExecutionContext, Future}
import scala.concurrent.duration._
import scala.util.{Failure, Success}

// @Path("/status")
class ProbesService(
config: Config,
authClient: RegistryAuthApiClient,
system: ActorSystem,
materializer: Materializer,
recordPersistence: RecordPersistence,
eventPersistence: EventPersistence
) extends Protocols {
val logger = Logging(system, getClass)
implicit val ec: ExecutionContext = system.dispatcher

def live: Route = get {
path("live") {
complete("OK")
}
}

def ready: Route = get {
path("ready") {
DB readOnly { session =>
recordPersistence.getReadiness(Some(logger))(session)
} match {
case Some(true) => complete("OK")
case Some(false) | None =>
complete(StatusCodes.InternalServerError, "Database not ready")
}
}
}
Comment on lines +48 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested in the cluster and it seems most exceptions are not captured.
e.g.

java.util.NoSuchElementException: key not found: to_regclass
OR org.postgresql.util.PSQLException: Connection to registry-db:5432 refused. Check that the hostname and port are correct and that the postmaster is accepting TCP/IP connections.
OR org.postgresql.util.PSQLException: FATAL: role "client" does not exist

I think you probably need to capture any failure from the DB.readonly method as the execution is happening there.
you might want to try scala's util.Try instead of try/catch --- it's more common in scala as it's more flexible / offer more streamlined exception processing
https://www.scala-lang.org/api/2.9.3/scala/util/Try.html

You can do something like this:

import scala.util.{Try, Failure, Success}
Try{
        DB readOnly { session =>
          recordPersistence.getReadiness(Some(logger))(session)
        }
    } match {
        case Success(Some(true)) => complete("OK")
        case Success(Some(false)) | Success(None) =>
          complete(StatusCodes.InternalServerError, "Database not ready")
        case Failure(exception) => complete(StatusCodes.InternalServerError, "Database not ready Other")
    }

The recordPersistence.getReadiness doesn't seem to need to capture any exceptions (so you don't have to capture at multiple place) nor has a chance to produce a NONE. I guess a Boolean could simpler


val route =
live ~
ready
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ trait RecordPersistence {
aspectOrQueries: Iterable[AspectQuery] = Nil
)(implicit session: DBSession): Long

def getReadiness(
logger: Option[LoggingAdapter] = None
)(implicit session: DBSession): Option[Boolean]

def getById(
tenantId: TenantId,
opaQueries: Option[List[(String, List[List[OpaQuery]])]],
Expand Down Expand Up @@ -245,6 +249,7 @@ class DefaultRecordPersistence(config: Config)

/**
* Given a list of recordIds, filter out any record that the current user has not access and return the rest of ids
*
* @param tenantId
* @param opaRecordQueries
* @param recordIds
Expand Down Expand Up @@ -323,6 +328,51 @@ class DefaultRecordPersistence(config: Config)
}
}

def getReadiness(
logger: Option[LoggingAdapter] = None
)(implicit session: DBSession): Option[Boolean] = {
def checkTableExists(table: String): Boolean = {
try {
val schemaAndTable = "public." + table
val regclass = sql"select to_regclass(${schemaAndTable})"

val tableExists = regclass
.map(_.toMap)
.list
.apply()
.map { res =>
{
res("to_regclass").toString == table
}
}
.forall(x => x);
tableExists
} catch {
case e: Exception => {
if (logger.isDefined) {
logger.get.error(
e,
s"Error occurred on getReadiness probe for table ${table}"
)
}
false
}
}
}

Some(
List(
checkTableExists("aspects"),
checkTableExists("events"),
checkTableExists("eventtypes"),
checkTableExists("recordaspects"),
checkTableExists("records"),
checkTableExists("webhookevents"),
checkTableExists("webhooks")
).forall(x => x)
)
}

def getAllWithAspects(
tenantId: TenantId,
aspectIds: Iterable[String],
Expand Down Expand Up @@ -676,7 +726,9 @@ where (RecordAspects.recordId, RecordAspects.aspectId)=($recordId, $aspectId) AN
case _ => Success(result)
}
// No failed aspects, so unwrap the aspects from the Success Trys.
resultAspects <- Try { patchedAspects.mapValues(_.get) }
resultAspects <- Try {
patchedAspects.mapValues(_.get)
}
} yield
result.copy(aspects = resultAspects, sourceTag = newRecord.sourceTag)
}
Expand Down Expand Up @@ -1368,7 +1420,7 @@ where (RecordAspects.recordId, RecordAspects.aspectId)=($recordId, $aspectId) AN
aspectIds: Iterable[String] = List()
)(implicit session: DBSession): Try[Iterable[String]] = {

/** For aspects that have links to other aspects, a map of the ids of those aspects to the location (first-level, not path) in their JSON where the link is */
/** For aspects that have links to other aspects, a map of the ids of those aspects to the location (first-level, not path) in their JSON where the link is */
val referenceMap = this.buildReferenceMap(aspectIds)

Try {
Expand Down Expand Up @@ -1952,6 +2004,7 @@ where (RecordAspects.recordId, RecordAspects.aspectId)=($recordId, $aspectId) AN
case _ => throw new Exception("Invalid tenant value " + tenantId)
}
}

private def aspectIdsToWhereClause(
tenantId: TenantId,
aspectIds: Iterable[String]
Expand Down