Flyte CoPilot: Raw container support for all K8s containers #84

kumare3 · 2020-05-06T05:50:21Z

TL;DR

This PR implements FlyteCoPilot as an optional for all container executions. This is an Alpha feature

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Refer to Design Doc

Tracking Issue

flyteorg/flyte#297

Follow-up issue

flyteorg/flyte#316
flyteorg/flyte#317

codecov-commenter · 2020-06-05T04:50:52Z

Codecov Report

Merging #84 into master will increase coverage by 3.69%.
The diff coverage is 92.77%.

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
+ Coverage   56.03%   59.73%   +3.69%     
==========================================
  Files          93       95       +2     
  Lines        4806     5327     +521     
==========================================
+ Hits         2693     3182     +489     
- Misses       1830     1844      +14     
- Partials      283      301      +18

Impacted Files	Coverage Δ
...tasks/pluginmachinery/flytek8s/container_helper.go	`80.64% <0.00%> (-2.69%)`	⬇️
go/tasks/pluginmachinery/flytek8s/pod_helper.go	`59.37% <52.94%> (-3.96%)`	⬇️
go/tasks/pluginmachinery/flytek8s/copilot.go	`92.85% <92.85%> (ø)`
go/tasks/pluginmachinery/utils/literals.go	`95.04% <95.04%> (ø)`
go/tasks/pluginmachinery/flytek8s/config/config.go	`50.00% <100.00%> (+50.00%)`	⬆️
...machinery/flytek8s/config/k8spluginconfig_flags.go	`46.15% <100.00%> (+12.82%)`	⬆️
go/tasks/plugins/k8s/container/container.go	`80.48% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f6d312...d106008. Read the comment docs.

wild-endeavor

definitely need to spend more time reading through/using the code, but let me know if you want an approval just to move forward.

wild-endeavor · 2020-06-13T17:51:25Z

Makefile

+	cd copilot; go build -o ../artifacts/flyte-copilot .; cd -
+
+cross_compile:
+	@glide install


what good catch, how did that get in there?

pre go.mod go i think

wild-endeavor · 2020-06-13T18:16:37Z

copilot/README.md

+
+    Once we know the main container, waiting for it to exit is simple and implemented
+    Copying data is simple and implemented
+


It is not clear reading this which of the three you went with.

wild-endeavor · 2020-06-13T18:19:51Z

copilot/cmd/download.go

+
+func (d *DownloadOptions) Download(ctx context.Context) error {
+	if d.remoteOutputsPrefix == "" {
+		return fmt.Errorf("to-remoute-prefix is required")


wild-endeavor · 2020-06-13T18:22:40Z

copilot/cmd/download.go

+}
+
+func (d *DownloadOptions) Download(ctx context.Context) error {
+	if d.remoteOutputsPrefix == "" {


Should we add eventing to this to emit events to Admin?

Can we emit a timer?

yes metrics i want to add, but as a second pass

wild-endeavor · 2020-06-13T18:33:56Z

copilot/data/download.go

@@ -0,0 +1,365 @@
+package data


I imagine that the future flytectl will have all this code as well. Maybe this copilot will be a dependency. I can see users wanting to download inputs and outputs to past workflow and task executions to local.

You have absolutely hit a point that I was thinking as well. CoPilot should be part of flytectl?

wild-endeavor · 2020-06-13T18:53:24Z

go/tasks/pluginmachinery/utils/literals.go

@@ -0,0 +1,439 @@
+package utils


Maybe we can move this to flyteidl one day.

yes after this gets merged i was going to move this to there and move eventing etc into propeller

wild-endeavor · 2020-06-13T18:55:50Z

go/tasks/pluginmachinery/flytek8s/config/config.go

@@ -17,6 +25,21 @@ var (
 		DefaultAnnotations: map[string]string{
 			"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
 		},
+		CoPilot: FlyteCoPilotConfig{
+			NamePrefix:           "flyte-copilot-",
+			Image:                "docker.pkg.github.com/lyft/flyteplugins/operator:v0.4.0",


This will make it so that we have to make two PRs right for every change we want to publish right? The first to make the actual change and the second to update this version. Are we okay with this? I think it's fine, just thought I'd mention it.

no, its a config, so you can just change the config for the deployment?

wild-endeavor · 2020-06-13T19:00:32Z

go/tasks/pluginmachinery/flytek8s/copilot.go

+			if err != nil {
+				return err
+			}
+			coPilotPod.InitContainers = append(coPilotPod.InitContainers, downloader)


Appending will make it the last init container - should we make it the first instead?

cannot make the first like in case of IAM wait, but IAM wait is injected. Also, we do not really add the data volume to all init containers, thus not sure, if making it earlier has any uses?

copilot/cmd/root.go

wild-endeavor · 2020-06-13T19:18:36Z

copilot/cmd/sidecar.go

+	return nil
+}
+
+func NewUploadCommand(opts *RootOptions) *cobra.Command {


In general, I feel like the uploader, downloader, and sidecar watcher should be different cmds in flytectl. That could get messy too but maybe worth thinking about?

no this is not messy at all. I think I agree, I think they should be in flytectl, just that we need to move some code to flyteidl before we do that. What do you think we move ahead and then clean up after the fact. I can drop all the github workflows?

sure sure, sounds good.

wild-endeavor · 2020-06-17T20:18:44Z

just let me know when you want the final +1

Ketan Umare added 27 commits April 26, 2020 22:02

Work In progress

528e23f

updated

3f76571

Dockerfile, binary etc

7a947f3

Updated

7c8a254

Improved Flyte CoPilot Configuration

5e76ca7

fixed config

5df5c28

Working again

eb6d6dc

Updated cpu/mem and other things

fe9ad4b

updated default

0f6dfd6

Add support for flyteidl

7e2fa10

using custom

016de6e

More unit tests

56f98cc

unit tests

c0e649b

Merge branch 'master' into raw-container-plugin

f12e8ad

More unit tests and standardization

a55e859

Use default storage medium

e9ea6e8

Merge branch 'master' into raw-container-plugin

cdbf921

Improved Error messaging.

d47b3f8

any-extension support

da9a8d4

upload extension fix

7c8fa1e

Updated to use new idl

1b34fba

Better uploader and downloader

a8fffb6

Updated Flyteplugins

5a1f800

Pod should also add information

85098e3

Better code arragement

98f7a70

More unit tests

624aacb

More unit tests

dc4349c

kumare3 requested a review from EngHabu May 19, 2020 21:52

Ketan Umare added 2 commits May 19, 2020 16:24

Fixed linter errors

66d8bcc

More unit tests

4fd16f4

kumare3 changed the title ~~Raw container plugin~~ Flyte CoPilot: Raw container support for all K8s containers May 27, 2020

Ketan Umare added 6 commits May 28, 2020 21:47

Merge branch 'master' into raw-container-plugin

b9aee8d

Better logging for CoPilot

40fde05

Merge branch 'master' into raw-container-plugin

f217dcd

update flytestdlib

24c62d9

Update flytestdlib dependency

c079734

Merge branch 'master' into raw-container-plugin

b45f3cc

Ketan Umare added 6 commits June 4, 2020 22:15

refactored

829c995

wip

c7ce242

compiling, but tooling is broken

e213597

compiling

1bc3d9f

updated dependency and test

c02f480

test fix

b899660

wild-endeavor reviewed Jun 13, 2020

View reviewed changes

comments update

033c9f3

Ketan Umare added 4 commits June 21, 2020 22:19

flytestdlib updated

ea922f9

typo fix

0a3bea1

Make update

bdec44d

Merge branch 'master' into raw-container-plugin

b2eb94c

wild-endeavor previously approved these changes Jun 23, 2020

View reviewed changes

rename operator to copilot

3613bdf

kumare3 dismissed wild-endeavor’s stale review via 3613bdf June 23, 2020 16:20

wild-endeavor previously approved these changes Jun 23, 2020

View reviewed changes

upload image

d106008

kumare3 dismissed wild-endeavor’s stale review via d106008 June 23, 2020 16:57

wild-endeavor approved these changes Jun 23, 2020

View reviewed changes

kumare3 merged commit d39837a into master Jun 23, 2020

eapolinario pushed a commit that referenced this pull request Sep 6, 2023

Flyte CoPilot: Raw container support for all K8s containers (#84)

a7ece00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flyte CoPilot: Raw container support for all K8s containers #84

Flyte CoPilot: Raw container support for all K8s containers #84

kumare3 commented May 6, 2020 •

edited

Loading

codecov-commenter commented Jun 5, 2020 •

edited

Loading

wild-endeavor left a comment

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 13, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 22, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 13, 2020

kumare3 Jun 17, 2020

wild-endeavor Jun 17, 2020

wild-endeavor commented Jun 17, 2020


		Once we know the main container, waiting for it to exit is simple and implemented
		Copying data is simple and implemented

Flyte CoPilot: Raw container support for all K8s containers #84

Flyte CoPilot: Raw container support for all K8s containers #84

Conversation

kumare3 commented May 6, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov-commenter commented Jun 5, 2020 • edited Loading

Codecov Report

wild-endeavor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor commented Jun 17, 2020

kumare3 commented May 6, 2020 •

edited

Loading

codecov-commenter commented Jun 5, 2020 •

edited

Loading