[JENKINS-38313] - Introduce LogStorage API in the core by oleg-nenashev · Pull Request #3575 · jenkinsci/jenkins

oleg-nenashev · 2018-08-03T12:31:49Z

Version of #3557, but with a single LogStorage as proposed by @jglick . The previous implementation had separate LoggingMethod and LogBrowser implementation directly in the core, but with this change I will need to move it to External Logging API.

Proposed Changelog Entries

Major RFE: JENKINS-52692 - Developer: Introduce new LogStorage API and LogStorageFactory extension point
RFE: JENKINS-52867 - Developer: Introduce new API for deleting Run logs

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

@carlossg @jenkinsci/code-reviewers I will appreciate feedback about the preferred approach

…bmissions from Elasticsearch

Hide launcher complexity from plugin.

…-task-logging-poc # Conflicts: # core/src/main/java/hudson/model/AbstractProject.java # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

# Conflicts: # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

…sisted for Runs + extend API

…es correctly

…I plugin

…tation

…Method

…-task-logging

…ation to External logging API

…ng/LogStorage

…er + documentation

oleg-nenashev · 2018-08-07T14:32:16Z

Test failures are unrelated - UC issues

oleg-nenashev · 2018-08-09T16:32:50Z

@jenkinsci/cloud-native-sig I have addressed all comments in this change. Tests have been also added. IMO it is ready for review.

carlossg

from my limited knowledge

jglick

The main questions are about the relationship to jenkinsci/workflow-job-plugin#27 and specifically when and how the log storage is selected; see https://github.com/jenkinsci/workflow-api-plugin/pull/17/files#diff-1d162ef4f3f75e09c30e1f8e52e36b01R118.

jglick · 2018-08-10T15:34:41Z

core/src/main/java/hudson/model/AbstractBuild.java


+            // Produce correct logger
+            // TODO: Consider merging with create Launcher
+            l = getLogStorage().decorateLauncher(l, getBuild(), currentNode, listener);


You should not need this.

jglick · 2018-08-10T15:38:02Z

core/src/main/java/jenkins/model/logging/LogStorage.java

+     * @throws InterruptedException Was interrupted while decorating listeners
+     */
+    @Nonnull
+    public Launcher decorateLauncher(@Nonnull Launcher original,


Remove. I cannot foresee any reason why we would ever need this, even with external sinks implemented via external processes—these should not be touching the user process being launched.

jglick · 2018-08-10T15:38:32Z

core/src/main/java/hudson/model/Run.java


+    /**
+     * Log storage associated with this build, if any.
+     * @since TODO


No need for @since on private stuff, only public APIs.

jglick · 2018-08-10T15:39:32Z

core/src/main/java/hudson/model/Run.java

        }

+        if (logStorage == null) {
+            logStorage = new CompatFileLogStorage(this);


Rather keep it null and just return CompatFileLogStorage on demand so we do not touch the serial form unless you are actually using a nondefault implementation. Compare Run.getArtifactManager() vs. Run.artifactManager. You might need to introduce a pickLogStorage() correspondingly.

jglick · 2018-08-10T15:42:12Z

core/src/main/java/jenkins/model/logging/Loggable.java

+    /**
+     * Returns {@code true} if the log file is no longer being updated.
+     */
+    public boolean isLoggingFinished();


Consider isLoggingComplete (or isLoggingCompleted) by analogy with LargeText parameters?

jglick · 2018-08-10T16:07:48Z

core/src/main/java/jenkins/model/logging/LogStorage.java

+@ExportedBean
+public abstract class LogStorage<T extends Loggable> {
+
+    protected transient T loggable;


Better to delete this and pass a T to any method which might need it. Make this an interface while you are at it.

jglick · 2018-08-10T16:12:06Z

core/src/main/java/jenkins/model/logging/LogStorage.java

+     * @throws InterruptedException Operation was interrupted
+     */
+    @Nonnull
+    public abstract AnnotatedLargeText<T> overallLog() throws IOException, InterruptedException;


Rename to getLogText for consistency with Run.

jglick · 2018-08-10T16:18:09Z

core/src/main/java/hudson/model/Run.java

@@ -1435,39 +1467,60 @@ public Collection<Fingerprint> getBuildFingerprints() {
        return rawF;


Consider removing the historical gzip hacks and at best make this into a LogStorageFactory that detects log.gz existence for compatibility reasons (but never does this for new builds).

jglick · 2018-08-10T16:20:35Z

core/src/main/java/jenkins/model/logging/impl/NoopLogStorage.java

+ * @since TODO
+ */
+@Restricted(Beta.class)
+public class NoopLogStorage extends LogStorage {


Still in use?

jglick · 2018-08-10T16:22:13Z

core/src/main/java/jenkins/model/logging/impl/StreamLogStorage.java

+ * @since TODO
+ */
+@Restricted(Beta.class)
+public abstract class StreamLogStorage extends LogStorage {


Maybe just delete it, since the hard parts are the read methods anyway; createBuildListener should become trivial.

jglick

A tip for killing StreamLogStorage.

jglick · 2018-08-10T16:51:34Z

core/src/main/java/jenkins/model/logging/impl/StreamLogStorage.java

+
+        // Global log filter
+        for (ConsoleLogFilter filter : ConsoleLogFilter.all()) {
+            logger = filter.decorateLogger(build, logger);


Suggest moving this to a protected method in RunExecution…

jglick · 2018-08-10T16:53:12Z

core/src/main/java/jenkins/model/logging/impl/StreamLogStorage.java

+        final Job<?, ?> project = build.getParent();
+
+        // Project specific log filters
+        if (project instanceof BuildableItemWithBuildWrappers && build instanceof AbstractBuild) {


…and then override that in AbstractBuildExecution, cleaning this mistake up at last.

WorkflowRun does not yet support ConsoleLogFilter.all() but that can be dealt with separately: https://github.com/jenkinsci/workflow-job-plugin/pull/27/files#diff-68f690dbd3fa2d9fb89d6af3f8fd7216R218

jglick · 2019-04-02T20:35:04Z

core/src/main/java/hudson/model/Run.java

     * @since 1.349
     */
    public @Nonnull InputStream getLogInputStream() throws IOException {
-    	File logFile = getLogFile();


For more current ideas about these methods, see overloads in WorkflowRun edited during jenkinsci/workflow-job-plugin#114 etc.

jglick · 2019-04-02T20:36:26Z

This was abandoned and does not need to be left open.

Xing Yan and others added 29 commits September 17, 2016 11:42

Initial interface of LoggingMethod.

4c6355b

Skeleton implementation of the remote logging extension point

28386a5

HACK: Get rid of the special symbol in order to be able to process su…

79ef4c0

…bmissions from Elasticsearch

Polish Javadoc a bit

69a7ef4

Hide launcher complexity from plugin.

5c287b3

Update LoggingDefinitionLauncherWrapper.java

febb8bf

Update LoggingMethod.java

b0d8e64

Merge pull request #5 from xyan0607/jw-hackaton-external-logging

948db76

Hide launcher complexity from plugin.

Save the progress

5224058

Merge commit 'ac05680bb6eba6efc8a825ba59fda6fbad753d22' into external…

1c37b73

…-task-logging-poc # Conflicts: # core/src/main/java/hudson/model/AbstractProject.java # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

Merge branch 'master' into external-task-logging

5ffea98

# Conflicts: # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java

[JENKINS-38313] - Facelift the implementation

e64d206

[JENKINS-38313] - Add the LogBrowser stub

d5c4651

[JENKINS-38313] - References to log storages and browsers are now per…

02fd209

…sisted for Runs + extend API

Cleanup FindBugs + Beta annotations

86d8e43

Revert custom patches

21aae74

[JENKINS-38313] - Adjust initialization logic so that Pipeline Operat…

d26d38a

…es correctly

[JENKINS-38313] - Move Launcher wrapping logic to External Logging AP…

5f6213e

…I plugin

[JENKINS-38313] - Refactor LoggingMethod API to simplify the implemen…

da4441a

…tation

[JENKINS-38313] - Align TaskListener and BuildListener API in Logging…

b193607

…Method

[JENKINS-38313] - CloseableStreaMBuildListener is not required

a2e0317

[JENKINS-38313] - Cleanup formatting, imports and licenses

48d03e0

Add TODO from @jglick, unrestrict API

f56ea7a

Merge commit '25da480fab1da7d2a01ab68a56faf00cae0c23b1' into external…

ac9633a

…-task-logging

Merge branch 'master' into external-task-logging

e15bdc1

[JENKINS-38313] - Move LogBrowser#getFile() default caching implement…

62024d7

…ation to External logging API

[JENKINS-38313] - Introduce API For Deleting logs

7ebcb15

[JENKINS-38313] - Adapt NoopLogFile to the new API

70e154e

[JENKINS-38313] - Squash LogStorage code into a single class

44e9333

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Aug 3, 2018

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Aug 3, 2018

oleg-nenashev mentioned this pull request Aug 3, 2018

External Logging API: Align with new Core version, fix Findbugs jenkinsci/external-logging-api-plugin#2

Merged

2 tasks

oleg-nenashev added 3 commits August 6, 2018 09:45

[JENKINS-38313] - Move more default implementations out of the core

ed05773

Merge commit '7b2698db89183b4d701107485db61eb36442ac25' into extloggi…

eaa71b4

…ng/LogStorage

[JENKINS-38131] - Implementations should be exportable

84158a2

oleg-nenashev requested a review from jglick August 7, 2018 09:04

[JENKINS-38313] - stepLog() is not required for now, can be added lat…

7c426d8

…er + documentation

This was referenced Aug 7, 2018

[JENKINS-52914] - Allow decorating Launchers for runs, introduce a createLauncher API on the Run level #3577

Closed

[JEP-207, JEP-212] - Address comments from @jglick jenkinsci/jep#169

Merged

[JENKINS-38313] - All LogStorage methods are now interruptable

6120801

oleg-nenashev requested a review from carlossg August 9, 2018 12:59

oleg-nenashev added 3 commits August 9, 2018 16:01

[JENKINS-38313] - Generalize FileLogStorage, add a test stub

f959341

[JENKINS-38313] - Add tests for Log storage

5e20ac9

[JENKINS-52867] - Add tests for Log deletion in builds

1113413

oleg-nenashev removed the work-in-progress The PR is under active development, not ready to the final review label Aug 9, 2018

carlossg approved these changes Aug 10, 2018

View reviewed changes

jglick reviewed Aug 10, 2018

View reviewed changes

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Aug 11, 2018

jglick reviewed Apr 2, 2019

View reviewed changes

jglick closed this Apr 2, 2019

		@@ -1435,39 +1467,60 @@ public Collection<Fingerprint> getBuildFingerprints() {
		return rawF;

Uh oh!

Conversation

oleg-nenashev commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changelog Entries

Submitter checklist

Uh oh!

oleg-nenashev commented Aug 7, 2018

Uh oh!

oleg-nenashev commented Aug 9, 2018

Uh oh!

carlossg left a comment

Choose a reason for hiding this comment

Uh oh!

jglick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oleg-nenashev commented Aug 3, 2018 •

edited

Loading

jglick left a comment •

edited

Loading