Skip to content

fix: log duplication when using dependencies#8042

Merged
aaron-prindle merged 1 commit intoGoogleContainerTools:mainfrom
ericzzzzzzz:fix#8023-log-duplication-when-using-dependencies
Nov 7, 2022
Merged

fix: log duplication when using dependencies#8042
aaron-prindle merged 1 commit intoGoogleContainerTools:mainfrom
ericzzzzzzz:fix#8023-log-duplication-when-using-dependencies

Conversation

@ericzzzzzzz
Copy link
Copy Markdown
Contributor

@ericzzzzzzz ericzzzzzzz commented Nov 4, 2022

Fixes: #8023
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

  • The cause was that each deployer was adding all builds to podSelectors, the logger of a deployer will stream log from all pods deployed with runner built artifacts.
  • Ideally, each deployer should only stream from pods that it deploys artifacts to.
  • The workable implementation in v1 only added shared artifacts between deployerArtifacts set and runner builds artifacts set to podSelector for streaming logs, port-forwarding, etc. With artifact image name as identifier for an artifact to filter common elements, but due to the difference between v1 and v2 we cannot use imageName as the key for the map to filter those, the reason was added in the comment, let me know if you have any concern for using artifact.tag as key to filter. Alternatively, we can just track deployed artifacts regardless they're built from runner or not. This should fix log duplication issue, but it will also stream logs for remote artifacts.

Manual Testing

Screen Shot 2022-11-07 at 10 17 16 AM

Follow-up Work (remove if N/A)

@ericzzzzzzz ericzzzzzzz requested a review from a team as a code owner November 4, 2022 19:36
@ericzzzzzzz ericzzzzzzz force-pushed the fix#8023-log-duplication-when-using-dependencies branch from ce8577a to 0773d04 Compare November 4, 2022 19:38
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 4, 2022

Codecov Report

Merging #8042 (0773d04) into main (290280e) will decrease coverage by 3.91%.
The diff coverage is 54.09%.

@@            Coverage Diff             @@
##             main    #8042      +/-   ##
==========================================
- Coverage   70.48%   66.56%   -3.92%     
==========================================
  Files         515      598      +83     
  Lines       23150    29039    +5889     
==========================================
+ Hits        16317    19331    +3014     
- Misses       5776     8280    +2504     
- Partials     1057     1428     +371     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/test.go 44.44% <0.00%> (ø)
cmd/skaffold/app/exitcode.go 100.00% <ø> (+6.66%) ⬆️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/render.go 35.48% <18.18%> (-5.90%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/run.go 64.28% <33.33%> (-9.63%) ⬇️
cmd/skaffold/app/cmd/fix.go 56.41% <37.50%> (-20.07%) ⬇️
... and 392 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ericzzzzzzz ericzzzzzzz marked this pull request as draft November 4, 2022 20:33
@ericzzzzzzz ericzzzzzzz marked this pull request as ready for review November 7, 2022 15:18
Copy link
Copy Markdown
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM

@aaron-prindle aaron-prindle merged commit 0a9f57e into GoogleContainerTools:main Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log duplication when using dependencies

2 participants