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

Correct template file for AWS_BACKUP_MULTIPART_CHUNK_SIZE env #1449

Merged
merged 4 commits into from
Jan 4, 2018

Conversation

drubin
Copy link
Contributor

@drubin drubin commented Dec 26, 2017

Fixes #1293

Correctly set up and configure the template in order to use chunked
upload.

It seems that @solidnerd introduced this feature in 607df08 (thanks we require this as well)

But it doesn't appear to work. #1293 and we are also having issues with larger uploads

I have simply enabled parsing this file as the docs recommend https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/gitlab.yml.example#L544

I hope this is correct? Else please point me in the right direction.

Fixes sameersbn#1293

Correctly setup and configure the template in order to use chunked
upload.
# multipart_chunk_size: 104857600
# Use multipart uploads when file size reaches 100MB, see
# http://docs.aws.amazon.com/AmazonS3/latest/dev/uploadobjusingmpu.html
multipart_chunk_size: {{AWS_BACKUP_MULTIPART_CHUNK_SIZE}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be neat and for backwards compability to remove the multipart_chunk_size when AWS_BACKUP_MULTIPART_CHUNK_SIZE is not set. I think not everyone needs this directly.

@solidnerd
Copy link
Collaborator

Hey @drubin,
thanks for you work. Yeah It seems that I missed to implement it in directly. I made a short review to that I hope it helps. If not feel free to ask.

@drubin
Copy link
Contributor Author

drubin commented Dec 26, 2017

@solidnerd Thanks for the incredibly fast reply. I am not exactly sure how to do "if" statements in the current templating language. Could it be as simple as a sed replace to remove the # comment if the var is set?

I have tried updating the commit to conditionally remove the multipart section if the ENV var is not passed in. I hope this is ok.

It appears that Fog already has default https://github.com/fog/fog-aws/blob/master/lib/fog/aws/models/storage/file.rb#L214 which is lower than the gitlab default? Either way, this seems like a good option to allow us to configure it.

@solidnerd
Copy link
Collaborator

Hey @drubin,

thanks for your changes. I will test it later how it's working directly for the next release. I think everything is working as excepted.

The fog-aws default is ~5Mb (5242880B).

I will let you know if some changes are required again or not.

@drubin
Copy link
Contributor Author

drubin commented Dec 26, 2017

@solidnerd Sure any time just ping me if you require any changes.

Would you prefer I rebase/squash my commits into 1?

@drubin
Copy link
Contributor Author

drubin commented Dec 26, 2017

I finally got the docker-compose setup installed, it seems as if my regex is not actually removing the inside blocks between the #start-aws-multipart and #end-aws-multipart when the AWS_BACKUP_MULTIPART_CHUNK_SIZE is not set but when I run them inside the container as a simple sed command they work exactly as expected.

Am I doing something wrong?

@solidnerd
Copy link
Collaborator

solidnerd commented Dec 28, 2017

Hey @drubin,
i tried it by my self and get the same problem atm I have no clue why it's not working correctly. We need to dig deeper.

If prefer when the feature is complete a rebase to be easier revertable with the changes and are more clean history.

@drubin
Copy link
Contributor Author

drubin commented Dec 29, 2017

@solidnerd What happens if we default the value to "null" instead of leaving it blank it would have the same effect no?

@solidnerd
Copy link
Collaborator

solidnerd commented Dec 29, 2017 via email

@solidnerd
Copy link
Collaborator

solidnerd commented Dec 29, 2017

Ah okay Email replies doesn't support Markdown "${AWS_BACKUP_MULTIPART_CHUNK_SIZE}"

@drubin
Copy link
Contributor Author

drubin commented Dec 29, 2017

@solidnerd That isn't the issue because I added debug statements inside the IF statement from the functions.

I am going to keep trying to figure this out as this is something I would really like to see.

What is the policy about submitting releases that don't directly correspond to gitlab releases? Or would this fix have to wait for the next gitlab release?

@solidnerd
Copy link
Collaborator

solidnerd commented Dec 29, 2017 via email

@drubin drubin force-pushed the fix-aws-multi-part-upload branch from 75e60b6 to 94d5f68 Compare December 29, 2017 15:16
@drubin
Copy link
Contributor Author

drubin commented Dec 29, 2017

@solidnerd, in that case, would you mind me adding a flag for encryption: 'AES256' as well?

@solidnerd
Copy link
Collaborator

Feel free to do it. Also the storage_class is missing. For more information have a look in the upstream https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/gitlab.yml.example#L547

…kups

Also refactor the multi part config to match
@drubin drubin force-pushed the fix-aws-multi-part-upload branch from 68b6293 to 5ea41eb Compare December 29, 2017 15:42
@drubin
Copy link
Contributor Author

drubin commented Dec 29, 2017

@solidnerd I managed to get it working, I think there was an issue with the #start-aws starting the #start-aws-multipart LOTS of debugging later I changed the variable.

I have also added config options for AES encryption as well as storage class options and updated the readme.

Let me know if it works for you, or if you would like any other changes

@solidnerd
Copy link
Collaborator

It will be in the next release we didn't a separate release for this because 10.3.3 is available now.

@drubin
Copy link
Contributor Author

drubin commented Jan 2, 2018

Thanks

@solidnerd solidnerd merged commit 5430452 into sameersbn:master Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants