Skip to content
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

fix: for pod that's been GC'ed we need to get the log from the artifact #9540

Merged
merged 1 commit into from
Sep 8, 2022
Merged

fix: for pod that's been GC'ed we need to get the log from the artifact #9540

merged 1 commit into from
Sep 8, 2022

Conversation

juliev0
Copy link
Contributor

@juliev0 juliev0 commented Sep 7, 2022

Signed-off-by: Julie Vogelman julie_vogelman@intuit.com

Fixes #9434

This fixes the use case that a Pod has been GC'ed and enables the UI to get the log from the artifact in this case.

This reverts a change made to this function here.

@juliev0
fix: for pod that's been GC'ed we need to get the log from the artifact
c5394ec 
Signed-off-by: Julie Vogelman <julie_vogelman@intuit.com>
@terrytangyuan terrytangyuan enabled auto-merge (squash) September 7, 2022 19:51
@juliev0
Copy link
Contributor Author

juliev0 commented Sep 7, 2022

Note there's a very rare case in which isPendingOrRunning=true and the Pod gets deleted immediately after, such that by the time we make the call on line 216 it fails because the Pod's been deleted. Seems rare enough that I'm not sure we want to worry about it, though.

@juliev0 juliev0 marked this pull request as draft September 7, 2022 20:02
auto-merge was automatically disabled September 7, 2022 20:02

Pull request was converted to draft

@juliev0
Copy link
Contributor Author

juliev0 commented Sep 7, 2022

converting to draft for the moment just so I can do testing with different types of Workflows just to make sure no issues

@juliev0 juliev0 marked this pull request as ready for review September 7, 2022 20:56
@juliev0
Copy link
Contributor Author

juliev0 commented Sep 7, 2022

converting to draft for the moment just so I can do testing with different types of Workflows just to make sure no issues

done. tested under various different circumstances different workflows

@juliev0 juliev0 requested a review from alexec September 7, 2022 20:56
@juliev0
Copy link
Contributor Author

juliev0 commented Sep 8, 2022

@terrytangyuan Sorry, I know you enabled auto-merge, but I accidentally un-did that when I converted to draft and then back to "ready for review". Would you mind merging this?

@terrytangyuan terrytangyuan merged commit 9d66b69 into argoproj:master Sep 8, 2022
@juliev0 juliev0 deleted the 9434-ui-logs branch September 8, 2022 16:24
@juliev0
Copy link
Contributor Author

juliev0 commented Sep 8, 2022

@terrytangyuan Thank you for being so on top of these PRs!

@terrytangyuan
Copy link
Member

@terrytangyuan Thank you for being so on top of these PRs!

Sure. Thanks for your fixes!

juchaosong pushed a commit to juchaosong/argo-workflows that referenced this pull request Nov 3, 2022
@juliev0 @juchaosong
fix: for pod that's been GC'ed we need to get the log from the artifa…
701b67b 
…ct (argoproj#9540)

Signed-off-by: juchao <juchao@coscene.io>
@sarabala1979 sarabala1979 mentioned this pull request Nov 8, 2022
Closed
@agilgur5 agilgur5 added area/ui area/archive-logs Archive Logs feature labels Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@alexec alexec Awaiting requested review from alexec

Assignees
No one assigned
Labels
area/archive-logs Archive Logs feature area/ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3.4-rc2 - Workflows UI can no longer get logs (s3)
3 participants
@juliev0 @terrytangyuan @agilgur5