-
Notifications
You must be signed in to change notification settings - Fork 307
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
FISH-5799: access docker instances logs via docker rest #5544
base: main
Are you sure you want to change the base?
Conversation
c7bbe7b
to
83c3ed5
Compare
// temporary docker node, we need to access the original node, cut the added generated name | ||
String originalInstanceName = instanceName.substring(0, instanceName.lastIndexOf('-')); | ||
Server originalInstance = domain.getServerNamed(originalInstanceName); | ||
dockerContainerId = originalInstance.getDockerContainerId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the originalInstance is null? If that is the case this will throw NullPointerException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole part is a temporary solution. We need to get the docker host and port from the instance/node, not by searching other instance. So we need to fix creation of the running docker instance+node, fill the information and use it here.
if (node.getType().equals("DOCKER")) { | ||
// TEMP is a docker container as well. | ||
if (node.getType().equals("DOCKER") | ||
|| node.getType().equals("TEMP")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a bit blanket - I don't think there's strictly anything preventing you creating a temp node outside of a container and on localhost.
The intent is definitely that TEMP nodes should only really exist in cloud environments, so I guess it depends how defensive you want to be - it's probably fine to just update the comment to something like "Assume TEMP instances are within a container"
int nodeDockerPort = Integer.valueOf(node.getDockerPort()); | ||
try { | ||
if (dockerContainerId == null && node.getType().equals("TEMP")) { | ||
// FIXME: it would be easies, if the TEMP server had docker container id set! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An instance on a TEMP node should have it set?
From the docker-node entrypoint script, it should get set when creating the instance:
function createNewInstance {
...
if [ ! -z "${DOCKER_CONTAINER_ID}" ]; then
# Register Docker container ID to DAS
echo "Setting Docker Container ID for instance ${PAYARA_INSTANCE_NAME}: ${DOCKER_CONTAINER_ID}"
ASADMIN_COMMAND="${ASADMIN} -I false -H ${PAYARA_DAS_HOST} -p ${PAYARA_DAS_PORT} -W ${PAYARA_PASSWORD_FILE} _set-docker-container-id --instance ${PAYARA_INSTANCE_NAME} --id ${DOCKER_CONTAINER_ID}"
echo "${ASADMIN_COMMAND}"
${ASADMIN_COMMAND}
fi
The script even makes efforts to ensure that it at least has something set right at the start to avoid failing that if check:
DOCKER_CONTAINER_ID="$(cat /proc/self/cgroup | grep :/docker/ | sed s/\\//\\n/g | tail -1)"
It also later on makes efforts to check that the container ID matches:
# Check if an instance with this name is actually registered
echo "Checking if an instance with name ${PAYARA_INSTANCE_NAME} has been registered with the DAS"
...
if [ ! -z "${INSTANCE_EXISTS}" ]; then
# Check if Docker container ID registered against the instance name is the same
echo "Found an instance with name ${PAYARA_INSTANCE_NAME} registered to the DAS, checking if registered Docker Container ID matches this container's ID"
ASADMIN_COMMAND="${ASADMIN} -I false -t -H ${PAYARA_DAS_HOST} -p ${PAYARA_DAS_PORT} -W ${PAYARA_PASSWORD_FILE} _get-docker-container-id --instance ${PAYARA_INSTANCE_NAME}"
echo "${ASADMIN_COMMAND}"
REGISTERED_DOCKER_CONTAINER_ID="$(${ASADMIN_COMMAND})" || true
if [ ! -z "${REGISTERED_DOCKER_CONTAINER_ID}" ]; then
# If they're the same, simply create the folders, otherwise create and register a new instance
echo "Registered Docker Container ID is: ${REGISTERED_DOCKER_CONTAINER_ID}"
if [ "${REGISTERED_DOCKER_CONTAINER_ID}" == "${DOCKER_CONTAINER_ID}" ]; then
echo "Docker Container IDs match, creating local instance filesystem: "
ASADMIN_COMMAND="${ASADMIN} -I false -T -H ${PAYARA_DAS_HOST} -p ${PAYARA_DAS_PORT} -W ${PAYARA_PASSWORD_FILE} _create-instance-filesystem --node ${PAYARA_NODE_NAME} --dockernode true ${PAYARA_INSTANCE_NAME}"
${ASADMIN_COMMAND}
else
echo "Docker Container IDs do not match, creating a new instance."
createNewInstance
fi
else
echo "Could not retrieve registered Docker Container ID, creating a new instance"
createNewInstance
fi
else
createNewInstance
fi
So I think the only way it couldn't be set is if you're not using the script? In which case I'd just exit out and reach for my "stop holding it wrong" bat
if (dockerContainerId == null && node.getType().equals("TEMP")) { | ||
// FIXME: it would be easies, if the TEMP server had docker container id set! | ||
// temporary docker node, we need to access the original node, cut the added generated name | ||
String originalInstanceName = instanceName.substring(0, instanceName.lastIndexOf('-')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is safe
All generated names have a hyphen in them, not just when name resolution to a conflict has occurred.
} catch (IOException ex) { | ||
// } catch (IOException | ArchiveException ex) { | ||
throw new IOException("Unable to download file from docker node at " + nodeHost + ":" + nodeDockerPort + ", instance " + instanceName + ", container " + dockerContainerId, ex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely increasing complexity with this addition 😉
Postponed as there is no safe way how to get the logs from the docker node in all scenarios (docker rest may be switched off, payara rest may not be accessible). |
Description
Docker instance wasn't able to see logs from inside the docker. With this PR the log is available in the detail of the instance. Both View Log Files and View Raw Log work as well as http://localhost:4848/management/domain/view-log address.
Important Info
Blockers
Current version uses "guess" during information acquisition. This needs improvement.
Testing
Testing Performed
Create new Docker node with this settings:
Type
DOCKER
Node Host:
localhost
Node Directory:
/opt/payara/appserver/glassfish/nodes
Installation Directory:
/opt/payara/appserver
Docker Image:
payara/server-node:5.2021.9
Docker Port:
2375
(Payra wrongly enters 2376, although TLS if off)
Docker Password File: your path to password file
The password file can look like this:
AS_ADMIN_SSHPASSWORD='payara'
Create new instance, select the new docker node.
Start the instance
Select the newly created instance, click View Log Files and View Raw Log buttons.
Testing Environment
Linux, Docker version 20.10.11+
Documentation
Probably not needed, this functionality was missing.