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

Use virtual Java packages on Red Hat and set java_bin #2

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ekohl
Copy link

@ekohl ekohl commented Feb 20, 2025

This is puppetlabs/ezbake#627 opened against OpenVoxProject. I have only done an initial rebase without resolving all the issues.

I'll add inline notes with where we need to have further discussion.

Copy link
Author

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

As discussed in chat: @nmburgan will take his specific changes from #3, we'll merge that and I'll rebase this.

# Location of your Java binary (version 8)
JAVA_BIN="/usr/bin/java"
# Location of your Java binary
#JAVA_BIN=""
Copy link
Author

Choose a reason for hiding this comment

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

I believe the CLI also uses this. There's places like https://github.com/OpenVoxProject/openvox-server/blob/da7e9916bcb456e0793d69a1e69e0edeb5bdfc05/resources/ext/cli_defaults/cli-defaults.sh.erb#L7 others in the puppetserver repo that use this.

When testing this, I think running puppetserver --help or puppetserver ruby should trigger any error if it isn't set.

Copy link
Author

Choose a reason for hiding this comment

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

Related discussion: #5.

@ekohl ekohl mentioned this pull request Feb 20, 2025
@nmburgan
Copy link
Member

@ekohl Ready for rebase

@@ -14,6 +14,7 @@
options.old_sles = 0
options.sles = 0
options.java = 'java-1.8.0-openjdk-headless'
options.java_bin = '/usr/bin/java'

Choose a reason for hiding this comment

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

I was never a fan of this path. I think there is a java11/java17 symlink as well that we could use? We don't really know to what usr/bin/java points and I had multiple issues at customers where this was pointing to a too new or too old jre.

Choose a reason for hiding this comment

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

Edit: Ah you are setting it properly below, nevermind.

case options.platform_version
when 8
# rpm on Redhat 7 may not support OR dependencies
if options.os_version == 7

Choose a reason for hiding this comment

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

Do we still need EL7 specific code?

Copy link
Member

Choose a reason for hiding this comment

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

We're still building el7 for the moment. But I'd be in favor of dropping it if everyone else is.

Choose a reason for hiding this comment

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

@ekohl I think you rely on puppet 7 for satellite, but do you also need it for EL7?

Copy link
Author

Choose a reason for hiding this comment

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

EL7 isn't entirely dead downstream but that's more on the agent side. I think we can stop building server because at least in Satellite we don't support that.

# rpm on Redhat 7 may not support OR dependencies
if options.os_version == 7
options.java = 'jre-11-headless'
options.java_bin = '/usr/lib/jvm/jre-11/bin/java'
Copy link
Member

Choose a reason for hiding this comment

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

Is this (and the 17) path a stable path for all distributions of Java?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it is. On debian 12 with openjdk 17, it's /usr/lib/jvm/java-17-openjdk-arm64/bin/java, for example.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, but that's jdk, not jre. Lemme try that.

Copy link
Member

@nmburgan nmburgan Feb 20, 2025

Choose a reason for hiding this comment

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

Nope, I'm wrong, that is with openjdk-17-jre-headless installed. So yeah, not sure this path is one we can rely on.

The if condition on line 190 already guarantees that os_version is at
least 7 so the else can never be reached. This further simplifies the
condition.
Making it a config option allows for differentation. The comment for
version 8 was outdated, since puppetserver 8 refuses to start up with
Java 8.
The EnvironmentFile paths are config locations so if a user ever
modified them, they're not replaced. By defining this in the service
file, the packaging will update the location. The result is that even
if a java location changes that a simple yum/apt upgrade will respect
the new value in most situations.
This is a noop refactor, but makes the later diffs easier to read.
This uses the virtual packages jre-VERSION-headless instead of
explicitly openjdk. This allows other JREs to provide the same.

It also uses an explicit path to the JRE specific java bin. At least on
EL9 this allows the following upgrade path to work:

    dnf install https://yum.puppet.com/puppet7-release-el-9.noarch.rpm
    dnf install puppetserver
    dnf install https://yum.puppet.com/puppet8-release-el-9.noarch.rpm
    dnf upgrade puppetserver

Prior to this patch it would break, because it used /usr/bin/java which
will point to Java 8. By using /usr/lib/jvm/jre-17/bin/java we know for
sure it is Java 17.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants