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

FISH-5799: access docker instances logs via docker rest #5544

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
* only if the new code is made subject to such option by the copyright
* holder.
*/
// Portions Copyright [2018] Payara Foundation and/or affiiates
// Portions Copyright [2018-2021] Payara Foundation and/or affiiates

package com.sun.enterprise.config.serverbeans;

Expand Down Expand Up @@ -333,7 +333,9 @@ public static boolean isLocal(Node node) {
return false;
}
// Although the Docker container may be local to this machine, it's not truly "local"
if (node.getType().equals("DOCKER")) {
// TEMP is a docker container as well.
if (node.getType().equals("DOCKER")
|| node.getType().equals("TEMP")) {
Copy link
Member

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"

return false;
}
return NetUtils.isThisHostLocal(nodeHost);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
* only if the new code is made subject to such option by the copyright
* holder.
*
* Portions Copyright [2017-2018] Payara Foundation and/or affiliates
* Portions Copyright [2017-2021] Payara Foundation and/or affiliates
*/

package com.sun.enterprise.server.logging.logviewer.backend;
Expand All @@ -53,11 +53,17 @@
import com.trilead.ssh2.SFTPv3DirectoryEntry;
import com.trilead.ssh2.SFTPv3FileAttributes;

import fish.payara.enterprise.server.logging.TrivialTarInputStream;
import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.List;
import java.util.logging.Logger;
Expand Down Expand Up @@ -178,14 +184,60 @@ public File downloadGivenInstanceLogFile(ServiceLocator habitat, Server targetSe

wrf.copyTo(instanceLogFile);
} catch (WindowsException ex) {
throw new IOException("Unable to download instance log file from DCOM Instance Node");
throw new IOException("Unable to download instance log file from DCOM Instance Node", ex);
}

} else if (node.getType().equals("DOCKER") || node.getType().equals("TEMP")) {
String dockerContainerId = targetServer.getDockerContainerId();
String nodeHost = node.getNodeHost();
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!
Copy link
Member

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

// temporary docker node, we need to access the original node, cut the added generated name
String originalInstanceName = instanceName.substring(0, instanceName.lastIndexOf('-'));
Copy link
Member

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.

Server originalInstance = domain.getServerNamed(originalInstanceName);
dockerContainerId = originalInstance.getDockerContainerId();
Copy link
Contributor

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

Copy link
Contributor Author

@aubi aubi Jan 4, 2022

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.

String originalNodeName = originalInstance.getNodeRef();
Node originalNode = nodes.getNode(originalNodeName);
nodeHost = originalNode.getNodeHost();
nodeDockerPort = Integer.valueOf(originalNode.getDockerPort());
logFileName = "server.log";
}
File logFileDirectoryOnServer = makingDirectory(domainRoot + File.separator + "logs"
+ File.separator + instanceName);

String loggingDir = getLoggingDirectoryForNode(instanceLogFileName, node, sNode, instanceName);

if (logFileName == null || logFileName.equals("")) {
logFileName = "server.log";
}
//"/opt/payara/appserver/glassfish/nodes/Hilarious-Angelfish/docker-instance-LovelyChar/logs/server.log"
String remotePath = loggingDir + "/" + logFileName;

String prefix = "http" + (Boolean.valueOf(node.getUseTls()) ? "s" : "");

// store the log file to the given path
URL logUrl = new URL(prefix, nodeHost, nodeDockerPort, "/containers/"
+ dockerContainerId
+ "/archive?path="
+ remotePath);
BufferedInputStream downloadFile = new BufferedInputStream(logUrl.openStream());
// ArchiveInputStream tarStream = new ArchiveStreamFactory().createArchiveInputStream(downloadFile);
TrivialTarInputStream tarStream = new TrivialTarInputStream(downloadFile);
Path instanceLogPath = Paths.get(logFileDirectoryOnServer.getAbsolutePath(), logFileName);
//Path instanceLogPathTar = Paths.get(logFileDirectoryOnServer.getAbsolutePath() + ".tar", logFileName);
Files.copy(tarStream, instanceLogPath, StandardCopyOption.REPLACE_EXISTING);
// Files.copy(downloadFile, instanceLogPath, StandardCopyOption.REPLACE_EXISTING);
instanceLogFile = instanceLogPath.toFile();
} 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);
}
Copy link
Contributor

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 😉


}

return instanceLogFile;

}

public void downloadAllInstanceLogFiles(ServiceLocator habitat, Server targetServer, Domain domain, Logger logger,
Expand Down Expand Up @@ -402,7 +454,7 @@ private File makingDirectory(String path) {
}

}
created = targetDir.mkdir();
created = targetDir.mkdirs();
if (!created) {
return null;
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
*
* Copyright (c) [2021] Payara Foundation and/or its affiliates. All rights reserved.
*
* The contents of this file are subject to the terms of either the GNU
* General Public License Version 2 only ("GPL") or the Common Development
* and Distribution License("CDDL") (collectively, the "License"). You
* may not use this file except in compliance with the License. You can
* obtain a copy of the License at
* https://github.com/payara/Payara/blob/master/LICENSE.txt
* See the License for the specific
* language governing permissions and limitations under the License.
*
* When distributing the software, include this License Header Notice in each
* file and include the License file at glassfish/legal/LICENSE.txt.
*
* GPL Classpath Exception:
* The Payara Foundation designates this particular file as subject to the "Classpath"
* exception as provided by the Payara Foundation in the GPL Version 2 section of the License
* file that accompanied this code.
*
* Modifications:
* If applicable, add the following below the License Header, with the fields
* enclosed by brackets [] replaced by your own identifying information:
* "Portions Copyright [year] [name of copyright owner]"
*
* Contributor(s):
* If you wish your version of this file to be governed by only the CDDL or
* only the GPL Version 2, indicate your decision by adding "[Contributor]
* elects to include this software in this distribution under the [CDDL or GPL
* Version 2] license." If you don't indicate a single choice of license, a
* recipient has the option to distribute your version of this file under
* either the CDDL, the GPL Version 2 or to extend the choice of license to
* its licensees as provided above. However, if you add GPL Version 2 code
* and therefore, elected the GPL Version 2 license, then the option applies
* only if the new code is made subject to such option by the copyright
* holder.
*/
package fish.payara.enterprise.server.logging;

import java.io.IOException;
import java.io.InputStream;

/**
* Trivial implementation for TAR file downloaded from Docker, expecting regular file, ignoring filename.
*
* @author Petr Aubrecht <[email protected]>
*/
public class TrivialTarInputStream extends InputStream {

private final InputStream sourceStream;
private byte[] header = null;
private long size;
private long readSoFar = 0;

public TrivialTarInputStream(InputStream sourceStream) {
this.sourceStream = sourceStream;
}

@Override
public int read() throws IOException {
if (header == null) {
header = new byte[512];
sourceStream.read(header);
byte fileLinkType = header[156];
if (fileLinkType != 0 && fileLinkType != 48 /*'0'*/) {
throw new IOException("Tar file is not a normal file, type is " + fileLinkType);
}
String sizeStr = new String(header, 124, 11, "ASCII");
size = Integer.valueOf(sizeStr, 8);
}
if (readSoFar >= size) {
// we are at the end of file
return -1;
}
readSoFar++;
return sourceStream.read();
}

}