You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/08/24 04:28:58 UTC
[GitHub] [nifi] vitalyzhakov opened a new pull request, #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
vitalyzhakov opened a new pull request, #6329:
URL: https://github.com/apache/nifi/pull/6329
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-10360](https://issues.apache.org/jira/browse/NIFI-10360)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [x] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] vitalyzhakov commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
vitalyzhakov commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1239561682
@kevdoran , hello!
I'm OK, thank you!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
kevdoran commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1239566918
Closing per discussion. Will revisit as part of [larger changes](https://cwiki.apache.org/confluence/display/NIFI/NiFi+Docker+Container+Improvements), planned tentatively for sometime in the next couple of months.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1233437487
Thanks for perspective @kevdoran, that is helpful.
@vitalyzhakov It seems like the best approach at this point would be to close the pull request and following Kevin's recommendation to provide suggestions for the proposed Docker Container Improvements.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1231599292
Thanks for following up on this @vitalyzhakov.
@kevdoran is familiar with these Docker configurations, so he should be able to provide some additional perspective.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1228652118
@vitalyzhakov The standard Maven build does not create the Docker images, so testing the changes would require running a build with the `-P docker` profile activated.
Unless other contributors offer a different perspective, removing these volume declarations does not look like something that should be changed in light of other common examples.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] vitalyzhakov commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
vitalyzhakov commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1231365522
Hello, @exceptionfactory !
> Unless other contributors offer a different perspective
Can you invite people, who will help make right decision?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
kevdoran commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1235593717
@vitalyzhakov The use of `VOLUME` separates the data in the the configuration, state, and nifi repository directory from the rest of the container. This adds operational flexibility, for example, the volume can be mounted on another container, for example, for taking backups [1]. For example, someone might want to run this nightly:
```
docker run --rm --volumes-from <container-id> --volume ~/backup:/backup ubuntu bash -c "cd <path/to/volume/data> && tar -cvf /backup/<volume-backup>.tar ."
```
Source: https://www.ionos.com/digitalguide/server/security/docker-backup/
That said, as stated above, we are curerntly reevaluating our current approach to Docker images for the project, and a major area for improvement is making it easier to use bind mounts or named volumes for configuration or writeable storage. So I'd expect some changes and improvements in this area in the near future, but for now I'd prefer to keep the use of VOLUME in these Dockerfiles.
[1] https://docs.docker.com/storage/volumes/#backup-restore-or-migrate-data-volumes
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] vitalyzhakov commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
vitalyzhakov commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1226075915
Hello, @exceptionfactory !
Thanks for your work and fast answer!
Can we suppose, that this best practice on official site has a mistake?
[Opinion in the internet](https://boxboat.com/2017/01/23/volumes-and-dockerfiles-dont-mix/) are different for using VOLUME in Dockerfile. [Same opinion](https://stackoverflow.com/questions/44060341/is-it-a-docker-best-practice-to-use-volume-for-the-code).
Maybe we should discuss about use cases?
> The associated NiFi Jira issue shows copying the contents of the conf directory into the image itself, which is not the best approach for mutable and sensitive configuration files.
If user want to store data on host filesystem, he can use volume declaration in runtime.
>An alternative solution might include creating an alternative base image
I want to make this project some better (10M images downloaded * 50 MB ~ 500 TB disk space) and don't want to [multiply entities beyond necessity](https://en.wikipedia.org/wiki/Occam%27s_razor)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
kevdoran commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1233012799
Thanks for your proposal and reasoning @vitalyzhakov.
For now, I agree with @exceptionfactory that there are more benefits than downsides to defining the VOLUME inside the Dockerfile. The main downside is when trying to extend the image, but the current images for Apache NiFi are not optimized to be used as base images. Many other people in the community have come up with alternatives images for Apache NiFi that reuse some of the same Dockerfile commands, but do not inherit from apache/nifi.
That said, I'm planning to devote some time this fall to refactoring our approach to Docker images for the project, including separating out the base image from the final image, and also change our approach to sourcing external configurations, which will likely require re-evaluating our use of volumes. If you would like to offer suggestions for changes you would like to see in the Apache NiFi Docker images, we are collecting them here: https://cwiki.apache.org/confluence/display/NIFI/NiFi+Docker+Container+Improvements. You can request write access to the wiki on our [developer mailing list](https://nifi.apache.org/mailing_lists.html) or reply here and I'll be happy to add them to the wiki (with attribution).
Thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1226093108
Thanks for the reply @vitalyzhakov!
That post provides a helpful summary of the problems with declared volumes in Dockerfiles.
Taking a look at some of the official Docker images, however, it seems that the use of `VOLUME` is common for mutable locations. The [MongoDB](https://github.com/docker-library/mongo/blob/master/Dockerfile-linux.template#L111) and [MySQL](https://github.com/docker-library/mysql/blob/master/template/Dockerfile.debian#L92) templates use `VOLUME` for the data and configuration directories.
I agree that providing an optimal configuration in the standard community image is the best solution. With that being said, NIFI-5438 introduced volumes several years ago, so this approach has been the standard for several years.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran closed pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
kevdoran closed pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
URL: https://github.com/apache/nifi/pull/6329
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] vitalyzhakov commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
vitalyzhakov commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1227181265
Thanks for fast reply, @exceptionfactory !
>>I agree that providing an optimal configuration in the standard community image is the best solution. With that being said, NIFI-5438 introduced volumes several years ago, so this approach has been the standard for several years.
Maybe this was a mistake?
Users in NIFI-5799 also have problems for 4 years, caused by adding VOLUME instruction in Dockerfile.
Can your help me, executing `mvn clean install -P contrib-check` is enough for properly testing my changes?
Or I must make some additional tests?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] vitalyzhakov commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
vitalyzhakov commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1235111738
@kevdoran , hello!
Thanks for your comment!
>>there are more benefits than downsides to defining the VOLUME inside the Dockerfile
Can you describe also benefits?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran commented on pull request #6329: NIFI-10360 VOLUME declaration prevents updating flow from CI
Posted by GitBox <gi...@apache.org>.
kevdoran commented on PR #6329:
URL: https://github.com/apache/nifi/pull/6329#issuecomment-1239554932
@vitalyzhakov If you are ok with revisiting this as part of a larger refactoring to our approach to Docker images, we can go ahead and close this PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org