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

Refactor PodRunner and KubeTask to use PodController under the hood. #1986

Merged
merged 10 commits into from
May 23, 2023
Merged

Refactor PodRunner and KubeTask to use PodController under the hood. #1986

merged 10 commits into from
May 23, 2023

Conversation

e-sumin
Copy link
Contributor

@e-sumin e-sumin commented Apr 4, 2023

Change Overview

Here is a PodRunner refactoring using previously added PodController and PodCommandExecutor.

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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

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.

@infraq infraq added this to In Progress in Kanister Apr 4, 2023
@e-sumin e-sumin changed the title New pod runner Refactor PodRunner and dependent functions. Apr 4, 2023
@e-sumin e-sumin changed the base branch from pod_command_executor_exec_error to master April 4, 2023 22:43
@denisvmedia
Copy link
Contributor

@e-sumin could you please fix the linter issues?

@e-sumin e-sumin changed the base branch from master to pod_command_executor_exec_error May 11, 2023 09:05
denisvmedia
denisvmedia reviewed May 11, 2023
View reviewed changes
pkg/kube/pod_runner.go Outdated Show resolved Hide resolved
@e-sumin e-sumin force-pushed the pod_command_executor_exec_error branch from 539ea68 to 4b2e41d Compare May 12, 2023 09:24
Base automatically changed from pod_command_executor_exec_error to master May 15, 2023 23:41
@e-sumin e-sumin force-pushed the new-pod-runner branch 4 times, most recently from f391dab to da035a9 Compare May 18, 2023 17:57
@e-sumin e-sumin requested a review from denisvmedia May 19, 2023 08:11
@e-sumin e-sumin changed the title Refactor PodRunner and dependent functions. Refactor PodRunner and KubeTask to use PodController under the hood. May 19, 2023
denisvmedia
denisvmedia reviewed May 22, 2023
View reviewed changes
pkg/kube/pod_runner.go Outdated Show resolved Hide resolved
denisvmedia
denisvmedia reviewed May 22, 2023
View reviewed changes
pkg/kube/pod_runner.go Outdated
// PodRunner allows us to start / stop pod, write file to pod and execute command within it
type PodRunner interface {
Run(ctx context.Context, fn func(context.Context, *v1.Pod) (map[string]interface{}, error)) (map[string]interface{}, error)
RunEx(ctx context.Context, fn PodRunnerFunc) (PodOutputMap, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. I think we should either make named function types for both - Run and RunEx, or for none. The same is for the returned map type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a comment where on why we have to methods that are doing to the same things. Basically what you have explained in the comment below.
Also, I think RunEx is not very intuitive, do we have other alternate names :) ?

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've added the comment, but don't have better name. If you have something in mind, please suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denisvmedia I made returned type similar for both functions, but was unable to invent name for functor type, so I left it as is.

Copy link
Contributor

@denisvmedia denisvmedia May 23, 2023 •
edited

Choose a reason for hiding this comment

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

If no one has better names I suggest these:

  • PodAwareRunFunc
  • PodControllerAwareRunFunc

Also, as I previously said:

map[string]interface{} => PodOutputMap

Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming that we are going to get rid of Run method eventually. And then we can rename RunEx to Run.
I am ok with RunEx as well.

@e-sumin @denisvmedia
Better naming for internal context
9570306 
Co-authored-by: Denis Voytyuk <5462781+denisvmedia@users.noreply.github.com>
@viveksinghggits
Copy link
Contributor

@e-sumin,
There are some other kanister functions (for example prepare_data.go) that use podRunner to create and execute pods in them. Are we planning to handle those in other/separate PRs?

viveksinghggits
viveksinghggits reviewed May 22, 2023
View reviewed changes
Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

LGTM overall.

pkg/kube/pod_runner.go Outdated
// PodRunner allows us to start / stop pod, write file to pod and execute command within it
type PodRunner interface {
Run(ctx context.Context, fn func(context.Context, *v1.Pod) (map[string]interface{}, error)) (map[string]interface{}, error)
RunEx(ctx context.Context, fn PodRunnerFunc) (PodOutputMap, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a comment where on why we have to methods that are doing to the same things. Basically what you have explained in the comment below.
Also, I think RunEx is not very intuitive, do we have other alternate names :) ?

@e-sumin
Copy link
Contributor Author

e-sumin commented May 22, 2023

@viveksinghggits yep, there are several places where pod runner is used. I think it's better to refactor them in separate PRs, so that the reviewing process will be faster.

@e-sumin e-sumin added the kueue label May 23, 2023
@denisvmedia
Copy link
Contributor

@e-sumin please fix the signatures in the interface before merging. I put hold off merging to prevent the merge. You can remove it as soon as you sort out the issue.

viveksinghggits
viveksinghggits approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

Approving, we can merge after addressing Denis' interface signature comment.

Kanister automation moved this from In Progress to Reviewer approved May 23, 2023
@e-sumin e-sumin mentioned this pull request May 23, 2023
Closed
@mergify mergify bot merged commit 2babd1a into master May 23, 2023
Kanister automation moved this from Reviewer approved to Done May 23, 2023
@mergify mergify bot deleted the new-pod-runner branch May 23, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viveksinghggits viveksinghggits viveksinghggits approved these changes

@pavannd1 pavannd1 Awaiting requested review from pavannd1

@denisvmedia denisvmedia Awaiting requested review from denisvmedia

Assignees

@pavannd1 pavannd1

Labels
kueue
Projects
Kanister
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@e-sumin @denisvmedia @viveksinghggits @pavannd1