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

8932 container base image #8933

Merged
merged 64 commits into from
Dec 16, 2022
Merged

8932 container base image #8933

merged 64 commits into from
Dec 16, 2022

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Aug 24, 2022 •
edited

What this PR does / why we need it:

This PR adds a Maven submodule to build a base container image containing a tuned Payara app server and all things necessary to deploy Dataverse WAR in it.

Which issue(s) this PR closes:

Closes #8932

Special notes for your reviewer:

  • It might be debateable if this image should also do the heavy lifting for JHove config.
  • This pull request will be the first of more to come. It is necessary to talk about the review/QA process here, as discussed in-person. Looks like Spike: Backstop the local Dataverse QA process #8910 is related.
  • It does not yet contain a Github Actions to build and push the image to a registry.

TODOs:

  • Create flattened POM and use install target instead of package to make the build dependable from app image build
  • Make uid and gid configurable build args
  • Add Github Action to build and push the image to Docker Hub (needs credentials secrets; limit runs to IQSS repo)
  • Include with pushing the image also pushing a description. (Description should again state the community-only support)
  • Add performance tweaks from LIBIS image
  • Think about removing expired certs, but at build time
  • Look at the OpenAPI configuration as done by LIBIS
  • Think about a /data folder for future workaround of storing temporary upload files (see here) - maybe also to be used with <dataverse.files.directory>/temp location in case of containers?
  • Add a place/volume for the Make Data Count files (to be processed by another container, but logs written by us)
  • Probably make some places like /docroot, /data, /dumps a VOLUME by default? (Not likely to create a pitfall for Docker volumes vs. Kubernetes/Bindmounts chaos here)
  • Add some notes about the multiarch build nature on Linux (see 1)

Suggestions on how to test this:

Simply install Docker/... and follow the provided instructions within the new container guide.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.

Is there a release notes update needed for this change?:
I don't know yet. Maybe wait for having the Dataverse container ready and not just the base image.

Additional documentation:
Provided.

@poikilotherm poikilotherm added Component: Containers Anything related to cloudy Dataverse, shipped in containers. Working Group: SWC labels Aug 24, 2022
@poikilotherm poikilotherm marked this pull request as draft August 24, 2022 13:06
@coveralls
Copy link

coveralls commented Aug 24, 2022 •
edited

Coverage Status

Coverage remained the same at 19.997% when pulling 5122515 on poikilotherm:8932-ct-base-image into be48cfd on IQSS:develop.

@poikilotherm poikilotherm changed the title 8932 ct base image 8932 container base image Aug 24, 2022
@Kris-LIBIS
Copy link
Contributor

Since you create the payara user in the Dockerfile, I would make the user (and group) ID configurable - aka as ARG. Setting this to the ID of the deployment user allows easy access to data on mounted volumes for backups and troubleshooting.

BTW, if you are interested to have a look at our generic docker images, the repository is public: https://github.com/libis/rdm-build.git. There I start from the Payara docker image and we change the user and group ID of the payara user if needed.

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Aug 25, 2022 •
edited

Thanks @Kris-LIBIS for your feedback!

Maybe this base image is of help for you folks, too. I'll add the ARGS, sounds like a nice feature!

Browsing through the scripts I see you inherited from gdcc/dataverse-kubernetes so I'm just going to re-inherit some pretty nice ideas from you folks! 😄

@pdurbin pdurbin self-assigned this Aug 26, 2022
@poikilotherm poikilotherm force-pushed the 8932-ct-base-image branch 4 times, most recently from b449150 to ab5c03c Compare August 31, 2022 00:07
@poikilotherm poikilotherm moved this from On Deck 🛎 to Community Dev 🌎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Aug 31, 2022
@carlsonp
Copy link
Contributor

carlsonp commented Sep 1, 2022

I was able to successfully compile and run this image. I didn't try anything else at this point but it builds on my machine. Nice!

@poikilotherm poikilotherm force-pushed the 8932-ct-base-image branch 3 times, most recently from 6d8571b to 4164116 Compare September 19, 2022 23:00
@poikilotherm
feat(ct-base): add new base container image in submodule
139f0f7
@poikilotherm
feat(ct-base): remove the esh tool
2319a47 
Will be replaced with a capability to make API endpoints
for authentication providers read from MPCONFIG sources.
@poikilotherm
chore(deps): update container plugin and payara version for containers
f0202cb
@poikilotherm
fix(ct-base): unpack Payara to target/payara
2dc0596 
Payara 5 defaults to a "payara5" topmost dir, Payara 6 to
"payara6". To avoid adding different directories in the
assembly, cut the number from the directories name when
unpacking.

This does not prevent you from doing stupid things like
not cleaning before switching the version leading to an
unknown state of old and new libs, etc.
@poikilotherm
fix(ct-base): migrate base image from OpenJDK to Eclipse Temurin
246f8b8 
There was an ongoing discussion that the Docker Hub Image "openjdk"
is not backed by any official supported project but complete
goodwill of Oracle shipping their JRE/JDK.

There is no "real" release of OpenJDK . There exist only real
distributions like Oracle JDK, Eclipse Temurin, Azul JDK,
AWS Corretto etc (see https://whichjdk.com).

As for this reason the "openjdk" image has been deprecated,
switching to Eclipse Temurin JRE here.

See also: docker-library/openjdk#505
@poikilotherm
chore(deps): update Docker Maven Plugin to the latest release
76ea508
@poikilotherm
feat(ct-base): enable multiarch image build via docker buildx
f62dee2 
With the rise of Apple M1/M2 silicons, we need to provide ARM64
based images in addition to AMD64.
@poikilotherm
chore(ct-base): add maintainer details to POM
72935d4
@poikilotherm
docs(ct-base): update OCI tag labels
17d8b53
@poikilotherm
feat(ct-base): add debug/develop mode script
0a9947b
@poikilotherm
deps(ct-base): update to jattach v2.1
2e812dc
@poikilotherm
chore(ct-base): add JMX to exposed ports and make it default enabled …
7e836c7 
…as in Payara
@poikilotherm @BPeuch
docs(ct-base): apply some language tweaks to docs pages
4a79dcb 
Co-authored-by: Benjamin Peuch <benjamin.peuch@gmail.com>
pdurbin
pdurbin reviewed Nov 10, 2022
View reviewed changes
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@poikilotherm sorry to small chunk you, but this review is just for the docs. Overall, they look great!

doc/sphinx-guides/source/container/index.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/container/index.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/container/index.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/container/index.rst Outdated Show resolved Hide resolved
modules/container-base/README.md Outdated Show resolved Hide resolved
modules/container-base/README.md Outdated Show resolved Hide resolved
modules/container-base/README.md Outdated Show resolved Hide resolved
modules/container-base/README.md Outdated Show resolved Hide resolved
modules/container-base/README.md Outdated Show resolved Hide resolved
@poikilotherm
style(ct-base): rephrase container image tags IQSS#8932
426f746 
As requested by review from @pdurbin, aligning image tag names.
@poikilotherm poikilotherm mentioned this pull request Nov 11, 2022
Merged
@pdurbin pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Dec 2, 2022
@pdurbin pdurbin removed their assignment Dec 13, 2022
@mreekie
Copy link

mreekie commented Dec 14, 2022

added to sprint Dec 15, 2022

@poikilotherm
Copy link
Contributor Author

Todo inspired from @donsizemore #7530: make GC configurable

@pdurbin pdurbin self-assigned this Dec 16, 2022
@pdurbin pdurbin moved this from Ready for Review to Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Dec 16, 2022
pdurbin
pdurbin approved these changes Dec 16, 2022
View reviewed changes
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I'm approving this. All API tests are passing. I tweaked the docs a bit and ran mvn -Pct -f modules/container-base install as instructed in the new Container Guide.

It built the image, as expected:

$ docker images | head -2
REPOSITORY              TAG               IMAGE ID       CREATED          SIZE
gdcc/base               unstable          0a03fd50e8bf   11 minutes ago   588MB

@kcondon honestly I wouldn't worry too much about testing the mvn command above or any of the many configuration options that are documented in this PR (I didn't). This PR just lays the groundwork for a future "application image" (with Dataverse in it) to go on top of this base image. There are a few minor tweaks to our pom.xml but they shouldn't break anything. Again, all API tests are passing.

Thanks, @poikilotherm!

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🔎 to QA ✅ Dec 16, 2022
@pdurbin pdurbin removed their assignment Dec 16, 2022
@kcondon kcondon self-assigned this Dec 16, 2022
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Dec 16, 2022 •
edited

I don't think I'll add the ZGC option for now, as it is strongly discouraged to use the Java 11 preview version of it.

Please go ahead and merge if you like it. 🙂

@kcondon kcondon merged commit 71563c8 into IQSS:develop Dec 16, 2022
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Dec 16, 2022
@pdurbin pdurbin added this to the 5.13 milestone Dec 21, 2022
@GPortas GPortas mentioned this pull request Mar 3, 2023
Closed
@poikilotherm poikilotherm deleted the 8932-ct-base-image branch March 17, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannes-darms johannes-darms johannes-darms left review comments

@atrisovic atrisovic atrisovic left review comments

@BPeuch BPeuch BPeuch left review comments

@pdurbin pdurbin pdurbin approved these changes

Assignees

@kcondon kcondon

Labels
Component: Containers Anything related to cloudy Dataverse, shipped in containers. Size: 3 A percentage of a sprint. 2.1 hours.
Projects
IQSS Dataverse Project
Status: No status
IQSS/dataverse (TO BE RETIRED / DELET...
Done 🚀
Testy McTestFace
Status: Valuation
Milestone
5.13
Development

Successfully merging this pull request may close these issues.

Feature Request/Idea: create a base container image providing a Dataverse-tuned Payara application server
10 participants
@poikilotherm @coveralls @Kris-LIBIS @carlsonp @mreekie @pdurbin @johannes-darms @atrisovic @BPeuch @kcondon