You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/07/18 06:14:44 UTC

[GitHub] [fineract] ptuomola opened a new pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

ptuomola opened a new pull request #1187:
URL: https://github.com/apache/fineract/pull/1187

## Description
Currently any imported files are stored in the Fineract container itself. Once the container is recreated, these files are lost and the database is out of synch. To fix this, create a volume for storing the imported files and mount that to the container.

## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!

- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.

- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.

- [ ] API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.

- [ ] Integration tests have been created/updated for verifying the changes made.

- [ ] All Integrations tests are passing with the new commits.

- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] edcable commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

edcable commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-676808316

@Nayan had this feedback to share on this issue:

The cause is the system trying to access a file that simply does not exist. Most importantly we need to understand what scenarios system can get into this state.

1. You get this error if you recreate your Fineract docker image but you do not recreate your MySQL database ( as mentioned in the FINERACT-1037), this rarely happens and not the real reason for the error.

2. When deployed Fineract in clustered mode but storage pointed to the local file system, instead of to a shared network mounted files system
3. In the system configuration if they switch from storage from file system to S3 and then from S3 back to file system ( Fineract does not remember the files was stored in S3 or Filesystem it simply tries to access with present configuration )

- This PR might only fix problem number two.
- The issue number one is more of DevOps, we should not worry about it.
- A most important and possible our user may face is the third one, we need to solve is the third one (I have not tested the third one on the latest codebase, assumed based on my past experience)

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] ptuomola closed pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

ptuomola closed pull request #1187:
URL: https://github.com/apache/fineract/pull/1187


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] ptuomola commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

ptuomola commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-708220030


   Closing this - as per above comment, Docker Compose is not for production use and therefore both database and container should always be thrown away after use. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] ptuomola commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

ptuomola commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-672791084


   @vorburger I think we agreed this would not fix your null pointer exception, but I still think it would be a good thing to do... your views?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] ptuomola commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

ptuomola commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-676858392


   @Nayan, @edcable - many thanks for the feedback - much appreciated!
   
   A couple of comments in reply:
   
   > 1. You get this error if you recreate your Fineract docker image but you do not recreate your MySQL database ( as mentioned in the [FINERACT-1037](https://issues.apache.org/jira/browse/FINERACT-1037)), this rarely happens and not the real reason for the error.
   
   Agreed - this is the scenario I set out to address. In terms of this happening only rarely: would this not happen every time if you run a Dockerized Fineract set-up and you upgrade your Fineract Docker image to the latest one e.g. from Docker Hub? Unless I'm missing something, in such case, all the files stored on the previous image would be lost. 
   
   > 2. When deployed Fineract in clustered mode but storage pointed to the local file system, instead of to a shared network mounted files system
   
   Agree that that would lead to the same issue. In case this is a Dockerised installation, then my PR should fix it as well. But if it is non-Dockerized, I don't think we can solve this in the Fineract distribution, as we have no control over how people set up their local filesystems. Maybe we can point this out in the README though?
   
   > 3. In the system configuration if they switch from storage from file system to S3 and then from S3 back to file system ( Fineract does not remember the files was stored in S3 or Filesystem it simply tries to access with present configuration )
   
   I'm not sure that's entirely accurate. As far as I can see, Fineract does remember the configuration that a specific document was stored with - it should be stored in m_document.storage_type_enum. The problem is that the specific logic for downloading the import template ignores this and assumes everything is stored in the local file system (see code in BulkImportWorkbookServiceImpl). As I stated in my JIRA comment, I fully agree that this should also be fixed. 
   
   > * This PR might only fix problem number two.
   
   I think this fixes scenarios 1 and 2 for dockerised set-ups. 
   
   > * The issue number one is more of DevOps, we should not worry about it.
   
   Not sure I understand this. Of course there are other ways to solve this as well (e.g. write some upgrade scripts to copy the files to a new container). But in my view storing data you want to retain directly in the container is not a good idea, and given this PR addresses that and fixes 1, would it not make sense to apply it?
   
   > * A most important and possible our user may face is the third one, we need to solve is the third one (I have not tested the third one on the latest codebase, assumed based on my past experience)
   
   I agree that the code in BulkImportWorkbookServiceImpl needs to be fixed to respect the stored StorageType. I don't have a test environment for S3 repository so it's not easy for me to do this. I can raise a separate JIRA for this and let's hope someone picks this up...


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] vorburger commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

vorburger commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-660642893


   I need to refresh my memories about Volumes in Docker to be able to LGTM this... how does this work again - don't you then have to "map" them? Or I'm a confusing Docker Volumes with Kube PV & PVCs :smiling_imp: and this "just works" as is? - More importantly, I think there is another approach for FINERACT-1037... I'll comment there.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] vorburger commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

vorburger commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-707970001


   @ptuomola about this one.. I think we should EITHER forget about this, OR we should, for consistency, make it preserve the DB as well? Personally I would just forget about it... the docker-compose set up isn't meant for "production", as is.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [fineract] github-actions[bot] commented on pull request #1187: FINERACT-1037: Adding persistent volume for Fineract imports

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #1187:
URL: https://github.com/apache/fineract/pull/1187#issuecomment-695372110


   This pull request seems to be stale.  Are you still planning to work on it?  We will automatically close it in 30 days.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org