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.

Sign up for GitHub

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

Jump to bottom

Pure CloudWatch Logs implementation with no Fluentd #8

Merged

jglick merged 46 commits into jenkinsci:master from jglick:pure-cloudwatch

Nov 20, 2018

Member

jglick commented Aug 22, 2018 •

edited

Loading

If this works out, it would be a lot simpler conceptually and for administrators—no need to host and configure a separate service (meaning Evergreen could pick it up directly à la jenkins-infra/evergreen#128), and much more precise control over timing of event ingestion. And the Fluentd implementation as it stands is not suitable for production use since it has no means of limiting log output from agents to specified jobs (or builds) only.

PoC using AssumeRole (tests fail with access denied error if policy() is deliberately mistyped)
clean up role detection using new APIs in aws-credentials: Correcting default credentials when using assumed roles aws-credentials-plugin#46
test implementation starting from permanent user credentials with GetFederationToken
separate log stream per Jenkins Node so DescribeLogStreams can be restricted to the master
LogStorageTestBase.remotingInDocker to verify temporary credentials and job-granularity access control
asynchronous buffered log pushes
remove obsolete sender and timestamp fields
refresh temporary credentials when they expire


          Sketch of pure CloudWatch Logs implementation with no Fluentd.

e7f6d36

jglick added the work-in-progress label

jglick requested review from carlossg and oleg-nenashev

August 22, 2018 03:07

jglick added 2 commits

August 22, 2018 07:02


          Comments.

47b97b5


          AssumeRole indeed works in a local test environment. Needs some new A…

83a5382

…PIs in aws-credentials to make it pretty.

jglick changed the title ~~Sketch of pure CloudWatch Logs implementation with no Fluentd~~ Pure CloudWatch Logs implementation with no Fluentd

NPE

821336c

jglick mentioned this pull request

Add fluentd host and port to AWS global config page #6

Closed

jglick added 19 commits

August 29, 2018 10:12


          Merge branch 'master' into pure-cloudwatch

ce43db7


          Merge branch 'master' into pure-cloudwatch

5723cd8


          FindBugs

6b173f0


          Trick to get PipelineBridgeTest.remoting to pass when AWS credentials…

72c3440

… are unclear.


          Fixed handling of newly created projects.

42d9ba8


          Dealing with service exceptions more gracefully.

04266f1


          Rewrote sender to run asynchronously.


          Updated most deps to release versions.

a3042cb


          Switching to one log stream per job × node, so that sequence tokens c…

b8ed746

…an be maintained locally.


          Overhaul of how log stream state is managed. Remote access is broken …

a66a4e2

…due to exported object issues.


          Another failed attempt.

7d18cdc


          Reworked security to use HMACConfidentialKey + SlaveToMasterCallable …

f66f638

…rather than Channel.export.


          For some reason StreamHandler no longer worked; fixing RemoteLogDumpe…

651b36f

…r accordingly.


          FindBugs

4bb2088


          Using native AWS Logs timestamp is more efficient and DRY.

fc211c7


          Obsolete message; we could no longer throw up exceptions even if we w…

85e44c1

…anted to, since we are processing records asynchronously.


          Restoring console link, though imperfect.

ba53253


          Comment.

381f74e


          jenkinsci/aws-credentials-plugin#46 fixes AWSCredentialsImpl handling…

f6f5425

… of roles.

dominicgunn reviewed

View reviewed changes

dominicgunn left a comment

Couple of comments, nice work!

pom.xml

                       <relativePath />
                   </parent>
                   <groupId>io.jenkins.plugins</groupId>
-                  <artifactId>pipeline-log-fluentd-cloudwatch</artifactId>
+                  <artifactId>pipeline-log-fluentd-cloudwatch</artifactId> <!-- TODO minus fluentd -->

dominicgunn Oct 16, 2018

Do we still need this?

Member Author

jglick Oct 16, 2018

I was waiting for a non-PR context to actually rename the plugin in full (artifact ID, package, repo…).

...ava/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchAwsGlobalConfiguration.java

		@@ -151,6 +151,7 @@ FormValidation validate(String logGroupName, String region, String credentialsId

		try {
		filter(client, logGroupName);

dominicgunn Oct 16, 2018

Do we have tickets in backlog to do these todos?

Member Author

jglick Oct 16, 2018

Afraid not, this is on an “as is” basis.

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchRetriever.java Outdated Show resolved Hide resolved

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchRetriever.java

                           List<FilteredLogEvent> events;
                           try {
-                              events = client.filterLogEvents(createFilter().withFilterPattern("{$.timestamp = " + timestamp + "}").withLimit(1)).getEvents();
+                              events = client.filterLogEvents(createFilter().withLimit(1).withStartTime(timestamp)).getEvents();

dominicgunn Oct 16, 2018

Nice!

Member Author

jglick Oct 16, 2018

By way of background, the timestamp field in JSON (duplicating the native AWS Logs input / result event field) was there due to a bug in the Fluentd plugin that forwarded messages: fluent-plugins-nursery/fluent-plugin-cloudwatch-logs#108

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchRetriever.java Outdated Show resolved Hide resolved

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/LogStreamState.java Show resolved Hide resolved

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/LogStreamState.java

+                          synchronized (agentLogStreamNames) {
+                              for (int i = 1; ; i++) {
+                                  String candidate = logStreamNameBase + "@agent" + i;
+                                  if (agentLogStreamNames.add(candidate)) {

dominicgunn Oct 16, 2018

Do we care this is unbounded? What's the likelihood of this Set getting a bit too large?

Member Author

jglick Oct 16, 2018

Ought not get particularly large I think. The maximum size should be roughly the maximum number of agents being used concurrently by all builds of a given job. So, on the order of a few to dozens typically.

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/LogStreamState.java Outdated Show resolved Hide resolved

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/LogStreamState.java

+                          return;
+                      }
+                      String sequenceToken = null;
+                      MAIN: while (true) {

dominicgunn Oct 16, 2018 •

edited

Loading

I don't think I've ever seen this used in Java before, interesting!

Member

oleg-nenashev Nov 9, 2018

When it happens in Java, probably the code needs some refactoring :)

jglick commented

View reviewed changes

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/LogStreamState.java Outdated Show resolved Hide resolved

jglick added 9 commits

November 7, 2018 18:19


          @dominicgunn points out that the permalink URL can be constructed mor…

239a3f1

…e comprehensibly using URLEncoder.


          Implemented flushing.

6fdec3e


          Implemented paging.

3e47bda


          Move warnings about policy restriction into validation screen.

d4047de


          Forgot to turn off autoflush.

cb1b9fc


          For flush to be effective, it needs to actually verify that the serve…

72814a6

…r received all delivered events.

NPE

6f3e5e6


          Force JENKINS-52165 to be enabled for purposes of this plugin.

b512b8d


          At least warn the user that they may need to browse other streams to …

45a6b92

…get the full build log.

jglick removed the work-in-progress label

jglick added 3 commits

November 9, 2018 13:46


          Implementing LogStorage.getLogFile from jenkinsci/workflow-api-plugin#82

c3d5417


          FindBugs

7148f88


          Pick up some releases.

95c10e9

oleg-nenashev approved these changes

View reviewed changes

Member

oleg-nenashev left a comment

I am fine with the current code if it targets the alpha release, it looks solid though I am not fully familiar with the codebase. Once AWS Credentials are fixed, it can be released IMO. TODOs can be addressed later, as well as any performance concerns

pom.xml

                   <licenses>
                       <license>
                           <name>MIT License</name>
                           <url>https://opensource.org/licenses/MIT</url>
                       </license>
                   </licenses>
-                  <url>https://wiki.jenkins.io/display/JENKINS/Pipeline+Log+Fluentd+CloudWatch+Plugin</url>
+                  <url>https://wiki.jenkins.io/display/JENKINS/Pipeline+Log+Fluentd+CloudWatch+Plugin</url> <!-- TODO minus fluentd -->

Member

oleg-nenashev Nov 9, 2018

Address these TODOs before the alpha release?

Member Author

jglick Nov 9, 2018

Yes, I would expect to do that along with renaming the repository and adjusting the entry in repository-permissions-updater.

pom.xml Outdated Show resolved Hide resolved

...ava/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchAwsGlobalConfiguration.java Outdated Show resolved Hide resolved

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchRetriever.java Outdated Show resolved Hide resolved

src/main/java/io/jenkins/plugins/pipeline_log_fluentd_cloudwatch/CloudWatchRetriever.java Outdated Show resolved Hide resolved

Member

oleg-nenashev commented Nov 9, 2018

I also agree with some of @dominicgunn's code improvement proposals. Adding TODOs or addressing them would make sense

carlossg approved these changes

View reviewed changes

Contributor

carlossg left a comment

seems to work fine

jglick added 9 commits

November 13, 2018 10:43


          Merge branch 'master' into pure-cloudwatch

9fca2cd


          Since it is unclear when jenkinsci/aws-credentials-plugin#46 will be …

f62c139

…released, do not depend on it for now.


          Made MasterState.authenticate a little shorter and more readable.

fd53756


          Prefer callAsync if the result is not being used.

0353c64


          Should suffice to catch Exception in validate.

7462cdf


          Forgotten rename of constructor parameter.

5fdf150


          Message template in logger.

b2dc602


          Further simplified MasterState.authenticate by moving out validation …

7b384fd

…logic.


          FindBugs

32d2e6d

jglick mentioned this pull request

pipeline-cloudwatch-logs jenkins-infra/repository-permissions-updater#932

Merged

9 tasks


          Reverting f62c139 as jenkinsci/aws-credentials-plugin#46 has been rel…

012afe1

…eased.

jglick merged commit c799639 into jenkinsci:master

jglick deleted the pure-cloudwatch branch

November 20, 2018 18:27

jglick mentioned this pull request

Renamed plugin #12

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet