Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

feat: Include logs of all containers in error message #214

Merged
merged 5 commits into from
May 6, 2022

Conversation

Raffy23
Copy link
Member

@Raffy23 Raffy23 commented Apr 6, 2022

This PR

Solves

@Raffy23 Raffy23 requested a review from pchila April 6, 2022 14:57
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Unit Test Results

    1 files  13 suites   1s ⏱️
101 tests 95 ✔️ 6 💤 0

Results for commit 5b376da.

♻️ This comment has been updated with latest results.

@pchila
Copy link
Contributor

pchila commented Apr 7, 2022

Just by quickly looking at the code: I am not sure we want to include always the full logs of all containers all the time (think about the successful runs the would include logs from the init container every time) because the output could become polluted.
I would prefer to have the logs of the container that failed (init or main container) in case of failure and only the logs of the main container in case of success... What do you think @christian-kreuzberger-dtx ?

@christian-kreuzberger-dtx
Copy link
Contributor

christian-kreuzberger-dtx commented Apr 7, 2022

@pchila, I believe this is exactly how @Raffy23 implemented it! (Or did he force push some changes :D?) Relevant Code:

if container.status.Reason != "Completed" {

Copy link
Contributor

@christian-kreuzberger-dtx christian-kreuzberger-dtx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some deeper review, here are my change requests:

  • Do not output initcontainer logs UNLESS initcontainer or any other container fails (if initcontainer succeeds but the 2nd container fails, we need both log outputs)
  • Always output container logs of non-initcontainers
  • Ensure minimum interference with existing output, e.g.:
    previous output
Job job-executor-service-job-613f1259-f94f-4948-a3c0-7c97-1 finished successfully!

Logs:
...

  • Please keep the container name though, that's super helpful

@Raffy23 Raffy23 force-pushed the feat/improve-error-reporting branch from 1b9351d to 2d1f340 Compare April 22, 2022 08:49
@Raffy23 Raffy23 force-pushed the feat/improve-error-reporting branch 2 times, most recently from 76672ff to 8332598 Compare May 2, 2022 07:27
Signed-off-by: Raphael Ludwig <raphael.ludwig@dynatrace.com>
Signed-off-by: Raphael Ludwig <raphael.ludwig@dynatrace.com>
Signed-off-by: Raphael Ludwig <raphael.ludwig@dynatrace.com>
…account error

Signed-off-by: Raphael Ludwig <raphael.ludwig@dynatrace.com>
@Raffy23 Raffy23 force-pushed the feat/improve-error-reporting branch from 8332598 to 7f43233 Compare May 2, 2022 13:06
Signed-off-by: Raphael Ludwig <raphael.ludwig@dynatrace.com>
@Raffy23 Raffy23 force-pushed the feat/improve-error-reporting branch from d029802 to 5b376da Compare May 3, 2022 08:05
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2022

The following Docker Images have been built:

  • keptncontrib/job-executor-service:0.2.0-dev-pr-214,keptncontrib/job-executor-service:0.2.0-dev-pr-214.202205030805
  • keptncontrib/job-executor-service-initcontainer:0.2.0-dev-pr-214,keptncontrib/job-executor-service-initcontainer:0.2.0-dev-pr-214.202205030805

Copy link
Contributor

@pchila pchila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@christian-kreuzberger-dtx christian-kreuzberger-dtx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not solving #125 (still gives me Task 'Run helm' failed: max poll count reached for job job-executor-service-job-18a4f937-0918-412f-a38a-bdeb-1. Timing out after 5m1.048032241s), The reason behind is that the pod is not spawning at all :D
Other then that, it nicely solves #124 and it provides the logs that we needed in a readable and structured format.
LGTM

@christian-kreuzberger-dtx
Copy link
Contributor

Log Example:

Task 'Run helm' failed: job job-executor-service-job-cba897ca-b3d1-4840-8407-aa77-1 failed. Reason: BackoffLimitExceeded, Message: Job has reached the specified backoff limit

Logs: 
Container init-job-executor-service-job-cba897ca-b3d1-4840-8407-aa77-1 of pod job-executor-service-job-cba897ca-b3d1-4840-8407-aa77-1-fthxr:
2022/05/05 11:53:25 successfully moved file /charts/helloservice.tgz to /keptn/charts/helloservice.tgz


Container job-executor-service-job-cba897ca-b3d1-4840-8407-aa77-1 of pod job-executor-service-job-cba897ca-b3d1-4840-8407-aa77-1-fthxr terminated with an error (Reason: Error, ExitCode: 1):
Release "helloservice" does not exist. Installing it now.
Error: rendered manifests contain a resource that already exists. Unable to continue with install: Service "helloservice" in namespace "podtato-head-production" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-name" must equal "helloservice": current value is "podtato-head-production-helloservice"

@Raffy23 Raffy23 merged commit a58c2cb into main May 6, 2022
@Raffy23 Raffy23 deleted the feat/improve-error-reporting branch May 6, 2022 13:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail with a .finished event with proper message and status if initcontainer fails
3 participants