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

CRI containerd job environment #105

Merged
merged 7 commits into from
Mar 26, 2024
Merged

Conversation

koct9i
Copy link
Collaborator

@koct9i koct9i commented Jan 17, 2024

  • Cleanup handling legacy config layout
  • Cleanup building spec for exec nodes
  • Add jobResources and jobEnvironment into exec node spec
  • Ignore typed nil in Fetch And Sync
  • Add CRI job environment
  • Add EntrypointWrapper into InstanceSpec

Copy link

robot-magpie bot commented Jan 17, 2024

✅ All contributors are covered under a CLA with Yandex

See CONTRIBUTING.md for more info about Yandex Contributor License Agreement.


The following contributors were found:
3e765ff Author: @koct9i (k***9i@gmail.com)

(Only the first commit for a unique contributor is listed)

@koct9i koct9i marked this pull request as ready for review January 17, 2024 19:50
Copy link
Collaborator

@l0kix2 l0kix2 left a comment

Choose a reason for hiding this comment

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

I can't say much about containerd stuff here, suppose it's ok, but I suggest to add some testing (either e2e or envtest-based without real cluster — I hope some helpers for second option will be merged in main soon) for this new functionality.

Also I suppose we better test some existing exec nodes code current behaviour, it is not obviously to me we don't break anything from diff.

pkg/components/exec_node.go Outdated Show resolved Hide resolved
pkg/ytconfig/node.go Outdated Show resolved Hide resolved
pkg/ytconfig/cri.go Show resolved Hide resolved
@koct9i koct9i added the assigned Issue has an assignee label Mar 20, 2024
go.mod Show resolved Hide resolved
pkg/components/exec_node_base.go Outdated Show resolved Hide resolved
pkg/components/exec_node_base.go Outdated Show resolved Hide resolved
pkg/components/exec_node_base.go Outdated Show resolved Hide resolved
pkg/components/exec_node_base.go Outdated Show resolved Hide resolved
pkg/ytconfig/cri.go Show resolved Hide resolved
@koct9i koct9i force-pushed the cri-job-environment branch 3 times, most recently from e229cfc to 3966964 Compare March 21, 2024 14:28
api/v1/ytsaurus_types.go Outdated Show resolved Hide resolved
api/v1/ytsaurus_types.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@l0kix2 l0kix2 left a comment

Choose a reason for hiding this comment

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

lgtm, though suppose e2e which would successfully run job, relying on some specific image env would help not to break this code in the future.

Copy link
Collaborator

@l0kix2 l0kix2 left a comment

Choose a reason for hiding this comment

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

Tests are failing though

@l0kix2 l0kix2 merged commit 800ea8e into ytsaurus:main Mar 26, 2024
1 check passed
@koct9i koct9i deleted the cri-job-environment branch March 26, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned Issue has an assignee
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

4 participants