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.

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

Handle errors when waiting for pod status change fails #1922

Merged

Conversation

e-sumin
Copy link
Contributor

@e-sumin e-sumin commented Feb 21, 2023

Change Overview

When executing kubeTask it may happen that pod won't get into desired state (ready or succeeded). In such case we might lost information about failure reason. This PR introduces simple pod log tail capturing mechanism.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@e-sumin e-sumin changed the base branch from master to keu/e-sumin/extend-pod-runner February 21, 2023 11:00
@github-actions
Copy link
Contributor

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

@e-sumin e-sumin changed the base branch from keu/e-sumin/extend-pod-runner to refactor-pod-runner February 21, 2023 11:01
@e-sumin e-sumin force-pushed the keu/e-sumin/handle-errors-when-waiting-for-status-fails branch from fa4e669 to 89f8741 Compare February 21, 2023 12:25
e-sumin and others added 5 commits February 21, 2023 15:44
* Introduce interfaces for extended POD manipulation

* Update copyright year

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>

* Fix spelling

* Describe purpose of interface

---------

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
* Implement PodController interface

* Add `corev1` alias

* Fix variable name.

* Fix naming

* Rename errors and minor rewording.

* Update copyright year and spelling.

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>

* Address review note

---------

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
* Implement PodFileWriter interface

* Fix naming

* Rename `pfwp` property to `fileWriterProcessor` to make it more understandable.

* Update copyright year

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>

---------

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
* Implement PodController interface

* Implement PodCommandExecutor interface

* Update copyright year

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>

---------

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
* Introduce interfaces for extended POD manipulation

* Refactor PodRunner, use PodController under the hood
Base automatically changed from refactor-pod-runner to keu/e-sumin/extend-pod-runner February 21, 2023 15:38
@e-sumin e-sumin force-pushed the keu/e-sumin/handle-errors-when-waiting-for-status-fails branch from 89f8741 to 21d5442 Compare February 22, 2023 07:34
Comment on lines 25 to 26
Write(p []byte) (int, error) // Write log line(s) to circular buffer
ToString() string // Join stored log lines to one string separated by newline
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have the comments above the functions so that they build up a proper godoc format.

defer r.Close()

// Grab last log lines and put them to an error
lt := NewLogTail(10)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are chances that the actual error message is not included in the last 10 lines right? Or we are assuming that it will always be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that it should be there, but of course there is a chance that after logging an error, some other information will be logged, but frankly, I've never seen such scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I am not sure what should be done here. How to figure out how many lines to tail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. Keeping this solution since we don't have better options here.

Base automatically changed from keu/e-sumin/extend-pod-runner to master February 23, 2023 02:14
pkg/kube/log_tail.go Outdated Show resolved Hide resolved
pkg/kube/pod.go Outdated Show resolved Hide resolved
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
@e-sumin
Copy link
Contributor Author

e-sumin commented Feb 27, 2023

Since there are no more objections, merging.

@e-sumin e-sumin added the kueue label Feb 27, 2023
@mergify mergify bot merged commit ec63b99 into master Feb 27, 2023
@mergify mergify bot deleted the keu/e-sumin/handle-errors-when-waiting-for-status-fails branch February 27, 2023 17:14
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.

4 participants