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

Write the "whole schema" version after successful update and check that version in $healthcheck #2756

Closed
lmsurpre opened this issue Sep 14, 2021 · 3 comments
Closed

Write the "whole schema" version after successful update and check that version in $healthcheck #2756

lmsurpre opened this issue Sep 14, 2021 · 3 comments
Assignees
@punktilious
Labels
enhancement New feature or request P2 Priority 2 - Should Have schema-change a schema change
Milestone
Sprint 2021-14

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Sep 14, 2021 •
edited
Loading

Is your feature request related to a problem? Please describe.
Currently, we write the version of each schema object to FHIR_ADMIN.VERSION_HISTORY.
However, we don't have a good way to check the overall version of the schema.

Describe the solution you'd like

  1. After a successful schema migration, we should write the overall schema version (as according to the FhirSchemaVersion enum).

  2. In our healthcheck operation, instead of just checking for a valid connection to the database, it would be great to also check that the schema version. However, this would require the db user thats being used by the server to both:

    • know about the FHIR_ADMIN schema (I think it already does since it needs to call a stored proc on this schema in the db2 multi-tenant case, right?)
    • have access to query the FHIR_ADMIN.VERSION_HISTORY table. this could get complicated in multi-tenant/multi-schema scenarios because the db user should only have access to their own tenant/schema info. we'd probably want to introduce a stored procedure for it so that they can only get the info we want them to get.

  3. Optionally, before we attempt a schema migration, we should check to see if its already at the desired version. If so, we can skip the update-schema. However, if we implement this option I think we should have a flag that forces the migration anyway "just in case".

Describe alternatives you've considered
I was thinking we could use the max of the existing objects as a stand-in, but because schema upgrades use multiple transactions, we'd think that we're on the latest version before it actually completes.

Acceptance Criteria

  1. GIVEN [a precondition]
    AND [another precondition]
    WHEN [test step]
    AND [test step]
    THEN [verification step]
    AND [verification step]

Additional context
#2751 is similar but broader in that it also aims to protect from concurrent update requests.

The healthcheck update would be particularly handy for preventing a server from becoming "ready" until the corresponding schema update has successfully completed. See
Alvearie/alvearie-helm#28 (comment) for more info.
@prb112 prb112 added the enhancement New feature or request label Sep 15, 2021
@lmsurpre lmsurpre mentioned this issue Sep 16, 2021
Merged
@lmsurpre
Copy link
Member Author

team discussion: we should set the overall schema version in a new table in the data schema (not FHIR_ADMIN). this way it will have similar security characteristics as the other tables.

suggestion: do an initial implementation, gather feedback from team, then finish

@lmsurpre lmsurpre added the P2 Priority 2 - Should Have label Oct 18, 2021
@lmsurpre lmsurpre added this to the Sprint 2021-15 milestone Oct 18, 2021
@lmsurpre lmsurpre added the schema-change a schema change label Oct 18, 2021
@punktilious punktilious self-assigned this Oct 20, 2021
@punktilious
Copy link
Collaborator

punktilious commented Oct 20, 2021 •
edited
Loading

A new table SCHEMA_VERSIONS is created in each data schema (not the FHIR_ADMIN schema). The version is checked before updates are applied, allowing those updates to be skipped if the schema is already at the latest version (this short-circuits code and makes the step much lighter-weight.

COLUMN_NAME         |TYPE_NAME|DEC&|NUM&|COLUM&|COLUMN_DEF|CHAR_OCTE&|IS_NULL&
------------------------------------------------------------------------------
RECORD_ID           |INTEGER  |0   |10  |10    |NULL      |NULL      |NO      
VERSION_ID          |INTEGER  |0   |10  |10    |NULL      |NULL      |NO      

select * from schema_versions;
RECORD_ID  |VERSION_ID 
-----------------------
1          |20

The RECORD_ID is a simple mechanism to make it easier to keep only one row in the table.

In addition #2751 handles locking (via a lease mechanism) to ensure that only one schema update can be applied concurrently, making it much more cluster-friendly.

@punktilious punktilious mentioned this issue Oct 21, 2021
Merged
@lmsurpre
Copy link
Member Author

lmsurpre commented Oct 26, 2021 •
edited
Loading

I built new versions of the ibm-fhir-server and ibm-fhir-schematool images and then executed a helm upgrade --install against an existing deployment with a large postgresql database.

The schematool ran for about 2 minutes.

During this time, the healthcheck of the old pods continued to return 200 OK and they remained healthy.
Meanwhile, the healthcheck of the new pods returned HTTP 500 errors as expected and the new fhir server pods did not become ready until the schema update was complete. Good.

The fhir server logs now contain messages like the following:

WARNING Schema update required: whole-schema-version not supported.
INFO Transaction failed - afterCompletion(status = 4)
SEVERE The persistence layer reported one or more issues
com.ibm.fhir.exception.FHIROperationException: The persistence layer reported one or more issues  [probeId=ac-11-b5-6b-e22b8acb-a38e-4797-9d8b-680e639f1ee3]
	at com.ibm.fhir.operation.healthcheck.HealthcheckOperation.checkOperationOutcome(HealthcheckOperation.java:85)
	at com.ibm.fhir.operation.healthcheck.HealthcheckOperation.doInvoke(HealthcheckOperation.java:58)
	at com.ibm.fhir.server.spi.operation.AbstractOperation.invoke(AbstractOperation.java:57)
	at com.ibm.fhir.server.util.FHIRRestHelper.doInvoke(FHIRRestHelper.java:1382)
	at com.ibm.fhir.server.resources.Operation.invoke(Operation.java:83)
	at com.ibm.fhir.server.resources.Operation$Proxy$_$$_WeldClientProxy.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.ibm.ws.jaxrs20.cdi.component.JaxRsFactoryImplicitBeanCDICustomizer.serviceInvoke(JaxRsFactoryImplicitBeanCDICustomizer.java:348)
	at com.ibm.ws.jaxrs20.server.LibertyJaxRsServerFactoryBean.performInvocation(LibertyJaxRsServerFactoryBean.java:641)
	at com.ibm.ws.jaxrs20.server.LibertyJaxRsInvoker.performInvocation(LibertyJaxRsInvoker.java:160)
	at org.apache.cxf.service.invoker.AbstractInvoker.invoke(AbstractInvoker.java:101)
	at com.ibm.ws.jaxrs20.server.LibertyJaxRsInvoker.invoke(LibertyJaxRsInvoker.java:273)
	at org.apache.cxf.jaxrs.JAXRSInvoker.invoke(JAXRSInvoker.java:213)
	at com.ibm.ws.jaxrs20.server.LibertyJaxRsInvoker.invoke(LibertyJaxRsInvoker.java:444)
	at org.apache.cxf.jaxrs.JAXRSInvoker.invoke(JAXRSInvoker.java:112)
	at org.apache.cxf.interceptor.ServiceInvokerInterceptor$1.run(ServiceInvokerInterceptor.java:59)
	at org.apache.cxf.interceptor.ServiceInvokerInterceptor.handleMessage(ServiceInvokerInterceptor.java:96)
	at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:308)
	at org.apache.cxf.transport.ChainInitiationObserver.onMessage(ChainInitiationObserver.java:123)
	at org.apache.cxf.transport.http.AbstractHTTPDestination.invoke(AbstractHTTPDestination.java:277)
	at com.ibm.ws.jaxrs20.endpoint.AbstractJaxRsWebEndpoint.invoke(AbstractJaxRsWebEndpoint.java:137)
	at com.ibm.websphere.jaxrs.server.IBMRestServlet.handleRequest(IBMRestServlet.java:146)
	at com.ibm.websphere.jaxrs.server.IBMRestServlet.doGet(IBMRestServlet.java:112)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:686)
	at com.ibm.websphere.jaxrs.server.IBMRestServlet.service(IBMRestServlet.java:96)
	at com.ibm.ws.webcontainer.servlet.ServletWrapper.service(ServletWrapper.java:1258)
	at com.ibm.ws.webcontainer.servlet.ServletWrapper.handleRequest(ServletWrapper.java:746)
	at com.ibm.ws.webcontainer.servlet.ServletWrapper.handleRequest(ServletWrapper.java:443)
	at com.ibm.ws.webcontainer.filter.WebAppFilterChain.invokeTarget(WebAppFilterChain.java:183)
	at com.ibm.ws.webcontainer.filter.WebAppFilterChain.doFilter(WebAppFilterChain.java:94)
	at com.ibm.fhir.server.filter.rest.FHIRRestServletFilter.doFilter(FHIRRestServletFilter.java:155)
	at javax.servlet.http.HttpFilter.doFilter(HttpFilter.java:127)
	at com.ibm.ws.webcontainer.filter.FilterInstanceWrapper.doFilter(FilterInstanceWrapper.java:201)
	at com.ibm.ws.webcontainer.filter.WebAppFilterChain.doFilter(WebAppFilterChain.java:91)
	at com.ibm.ws.security.jaspi.JaspiServletFilter.doFilter(JaspiServletFilter.java:56)
	at com.ibm.ws.webcontainer.filter.FilterInstanceWrapper.doFilter(FilterInstanceWrapper.java:201)
	at com.ibm.ws.webcontainer.filter.WebAppFilterChain.doFilter(WebAppFilterChain.java:91)
	at com.ibm.ws.webcontainer.filter.WebAppFilterManager.doFilter(WebAppFilterManager.java:1002)
	at com.ibm.ws.webcontainer.filter.WebAppFilterManager.invokeFilters(WebAppFilterManager.java:1140)
	at com.ibm.ws.webcontainer.filter.WebAppFilterManager.invokeFilters(WebAppFilterManager.java:1011)
	at com.ibm.ws.webcontainer.servlet.CacheServletWrapper.handleRequest(CacheServletWrapper.java:75)
	at com.ibm.ws.webcontainer40.servlet.CacheServletWrapper40.handleRequest(CacheServletWrapper40.java:85)
	at com.ibm.ws.webcontainer.WebContainer.handleRequest(WebContainer.java:938)
	at com.ibm.ws.webcontainer.osgi.DynamicVirtualHost$2.run(DynamicVirtualHost.java:279)
	at com.ibm.ws.http.dispatcher.internal.channel.HttpDispatcherLink$TaskWrapper.run(HttpDispatcherLink.java:1159)
	at com.ibm.ws.http.dispatcher.internal.channel.HttpDispatcherLink.wrapHandlerAndExecute(HttpDispatcherLink.java:428)
	at com.ibm.ws.http.dispatcher.internal.channel.HttpDispatcherLink.ready(HttpDispatcherLink.java:387)
	at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.handleDiscrimination(HttpInboundLink.java:566)
	at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.handleNewRequest(HttpInboundLink.java:500)
	at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.processRequest(HttpInboundLink.java:360)
	at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.ready(HttpInboundLink.java:327)
	at com.ibm.ws.http.channel.h2internal.H2StreamProcessor$Http2Ready.run(H2StreamProcessor.java:1793)
	at com.ibm.ws.threading.internal.ExecutorServiceImpl$RunnableWrapper.run(ExecutorServiceImpl.java:238)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:866)

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@punktilious punktilious

Labels
enhancement New feature or request P2 Priority 2 - Should Have schema-change a schema change
Projects
None yet
Milestone
Sprint 2021-14
Development

No branches or pull requests
3 participants
@prb112 @lmsurpre @punktilious