-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add UploadLogCommand #11
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ release.properties | |
*.ipr | ||
*.iml | ||
*.iws | ||
.cache/ | ||
|
||
# Plugin directory | ||
/.quarkus/cli/plugins/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
package org.jboss.pnc.konfluxtooling.logging; | ||
|
||
import java.io.File; | ||
import java.math.BigInteger; | ||
import java.net.URI; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.security.MessageDigest; | ||
import java.time.OffsetDateTime; | ||
|
||
import org.jboss.pnc.bifrost.upload.BifrostLogUploader; | ||
import org.jboss.pnc.bifrost.upload.LogMetadata; | ||
import org.jboss.pnc.bifrost.upload.TagOption; | ||
|
||
import io.quarkus.logging.Log; | ||
import picocli.CommandLine; | ||
|
||
import static org.apache.commons.lang3.StringUtils.isBlank; | ||
|
||
@CommandLine.Command(name = "upload-log") | ||
public class UploadLogCommand implements Runnable { | ||
|
||
private static final int DEFAULT_MAX_RETRIES = 4; | ||
|
||
private static final int DEFAULT_DELAY_SECONDS = 60; | ||
|
||
@CommandLine.Option(names = "--file", required = true) | ||
String logFile; | ||
|
||
@CommandLine.Option(names = "--bifrost-url") | ||
String bifrostURL; | ||
|
||
@CommandLine.Option(names = "--max-retries") | ||
int maxRetries = DEFAULT_MAX_RETRIES; | ||
|
||
@CommandLine.Option(names = "--delay-seconds", | ||
description = "in case of retries this is the delay in seconds before next retry") | ||
int delaySeconds = DEFAULT_DELAY_SECONDS; | ||
|
||
@CommandLine.Option(names = "--process-context", | ||
description = "id of an long running operation (in this case the build-id is used)") | ||
String processContext; | ||
|
||
@CommandLine.Option(names = "--process-context-variant", | ||
description = "in case there are subtasks or retries of individual steps this field can be used to add another ID") | ||
String processContextVariant; | ||
|
||
@CommandLine.Option(names = "--tmp", | ||
description = "temp build or not, used for a log clean-up") | ||
String tmp = "false"; | ||
|
||
@CommandLine.Option(names = "--request-context", | ||
description = "an id of the initial (http) request that triggered this and potentially other processes") | ||
String requestContext; | ||
|
||
public void run() { | ||
try { | ||
if (isBlank(bifrostURL)) { | ||
Log.info("No bifrost url specified and no log upload is performed"); | ||
return; | ||
} | ||
var logFilePath = Path.of(logFile); | ||
var file = logFilePath.toFile(); | ||
if (!file.exists()) { | ||
throw new RuntimeException(String.format( | ||
"No log file found at %s. Has the build been correctly done?", logFilePath)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following on from https://github.com/project-ncl/konflux-tooling/pull/11/files#r1896576530 while currently the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean something like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No just /var/workdir but this only makes sense if everything is in the same tool. Let's discuss in the new year when everyone is back :-) |
||
} | ||
var md5 = getMD5(logFilePath); | ||
uploadLogsToBifrost(file, md5); | ||
} catch (Exception e) { | ||
Log.error("Upload log failed", e); | ||
throw new RuntimeException(e); | ||
} | ||
} | ||
|
||
private String getMD5(Path logFilePath) throws Exception { | ||
byte[] data = Files.readAllBytes(logFilePath); | ||
byte[] hash = MessageDigest.getInstance("MD5").digest(data); | ||
return new BigInteger(1, hash).toString(16); | ||
} | ||
|
||
private void uploadLogsToBifrost(File logFile, String md5) { | ||
BifrostLogUploader logUploader = new BifrostLogUploader(URI.create(bifrostURL), | ||
maxRetries, | ||
delaySeconds, | ||
() -> System.getProperty("ACCESS_TOKEN")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're reusing DeployCommand then accessToken is a configProperty in https://github.com/project-ncl/konflux-tooling/blob/main/src/main/java/org/jboss/pnc/konfluxtooling/deploy/DeployCommand.java#L28 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does the tool set value for "@ConfigProperty(name = "access.token")"? I did not see any Quarkus config file like application.properties or application.yaml. Or are there any way to provide the config file in runtime? And why do we need to use a config property? because I see the pipeline use an env var: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quarkus detects it as an env var and converts it to a configuration property |
||
|
||
LogMetadata logMetadata = LogMetadata.builder() | ||
.tag(TagOption.BUILD_LOG) | ||
.endTime(OffsetDateTime.now()) | ||
.loggerName("org.jboss.pnc._userlog_.build-agent") | ||
.processContext(processContext) | ||
.processContextVariant(processContextVariant) | ||
.tmp(tmp) | ||
.requestContext(requestContext) | ||
.build(); | ||
|
||
logUploader.uploadFile(logFile, logMetadata, md5); | ||
} | ||
} |
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.
If I understand correctly this is uploading the single
build.log
that is saved in redhat-appstudio/jvm-build-service#2327 ?So IMHO I think it would be better if the 'final upload' of both jars (etc) and the build.log is handled in the same command (i.e. https://github.com/project-ncl/konflux-tooling/blob/main/src/main/java/org/jboss/pnc/konfluxtooling/deploy/DeployCommand.java ) (whether or not is should be renamed is a separate discussion). Compressing them into the same java command means only a single tekton step action is being invoked and in future we can optimise in this single place.
I don't think the bifrost parameters should be
required = true
; instead if they are not specified then no log upload should be performed? Also can we have some breakdown / comments / etc as to what they are?Also did the PNC code have any tests to be ported across with it?
In terms of adding these new parameters to the pipeline they would be added all the way though I think i.e. the https://github.com/redhat-appstudio/jvm-build-service/blob/main/deploy/pipeline/mw-pipeline-v0.1.yaml , the deployment task, https://github.com/project-ncl/konflux-build-driver/blob/main/src/main/java/org/jboss/pnc/konfluxbuilddriver/Driver.java#L108 etc. And maybe the parameters (at least for now) should all be optional or have default values?
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 would go for a generic uploader that does not care about the content and can be used in any step, like wget or curl.
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.
@matejonnet I guess this code for uploading log is a bit specific for PNC. I don't know the details but looking at the those metadata properties would reveal that. Dustin also mentioned the current code has advanced features like streaming the file transfer. I'd suggest we stick to it.
@rnc I updated the code adding comments for those params. I don't know, at the moment, which one is required so I removed all 'required = true'. Also, as you suggested, skip it if bifrostURL is blank.
Regarding test, I didn't find unit test after looking around. I guess it needs a Bifrost server and we have to test it manually.
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.
@rnc In my viewpoint, it is better to separate the maven deployment with log upload. Although this means a new step in pipeline, it is easy to understand and maintain. As you can see, uploading log introduces a bunch of arguments. If we mix them to one command, it won't be easy to tell which is for which when people look at the pipeline args.
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 agree with both statements, keep the existing tool for log upload and keep the separate steps.
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.
@matejonnet I'm not convinced we need a generic tool - we already have the maven upload tooling but that is using the maven deployment code so isn't applicable. This tool is highly specific to PNC so how would the extra parameters be passed?
Further, I'm not convinced about keeping it separate - while it means the task parameters will be mixed it reduces the task step cpu/ memory cost (and bundle size for that matter) - I'm already seeing issues with running the buildah-oci task in some environments so I think we should reconsider this approach. Invoking as part of the deploy step will reduce the overall impact IMHO.
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.
Regarding cpu/memory, my personal experience is that it is a question of quota, something to be argued at later stage. Also, I see the current deployment step limits the cpu/mem to a minimal level. Maybe we don't need to worry too much about it at this moment?