K8 array resource manager #120

anandswaminathan · 2020-09-15T09:01:58Z

Continued from #71 - Unable to push to mtoledo/k8-array-resource-manager

New changes:

Support for remote endpoint
Unit tests for resource manager and k8s array.
Few minor fixes and updates.
Remove owner refs for remote execution.
Removed project/namespace limit for number of pods.

cc @EngHabu @migueltol22

anandswaminathan · 2020-09-17T22:41:35Z

PTAL @EngHabu

anandswaminathan · 2020-10-06T22:39:32Z

PTAL @katrogan @kumare3 @migueltol22

katrogan · 2020-10-06T23:05:25Z

go/tasks/plugins/array/core/state.go

-	if totalRunning+totalSuccesses < minSuccesses {
-		logger.Infof(ctx, "Array failed early because totalRunning[%v] + totalSuccesses[%v] < minSuccesses[%v]",
-			totalRunning, totalSuccesses, minSuccesses)
+	if totalRunning+totalSuccesses+totalWaitingForResources < minSuccesses {


wow how was this working correctly before?

Because we never have totalWaitingForResources ? Is that your question?

oh I didn't realize that phase was introduced here. yes, thanks for explaining

katrogan · 2020-10-06T23:09:47Z

go/tasks/plugins/array/k8s/config.go

+	return string(token), nil
+}
+
+// Reads secret values from paths specified in the config to initialize a Kubernetes rest client Config.


we should move this to flytestdlib since it also exists in flyteadmin

Yup. Will add TODO.

@katrogan / @anandswaminathan I prefer we dont do this at all, I know you want to do this to test, but keep it for shorter time. My preference is that we run the entire workflow in a separate cluster if need be

katrogan · 2020-10-06T23:56:00Z

go/tasks/plugins/array/k8s/monitor.go

+			// Continue with execution if successful
+		case LaunchError:
+			return currentState, logLinks, err
+		// If Resource manager is enabled and there are currently not enough resources we can skip this round


is this the only case when we get to LaunchWaiting?

Yes. Only resourcemanager has introduced LaunchWaiting.

katrogan · 2020-10-06T23:56:59Z

go/tasks/plugins/array/k8s/task.go

+func (t Task) Launch(ctx context.Context, tCtx core.TaskExecutionContext, kubeClient core.KubeClient) (LaunchResult, error) {
+	podTemplate, _, err := FlyteArrayJobToK8sPodTemplate(ctx, tCtx)
+	// Do not owner references for remote cluster execution
+	if t.Config.RemoteClusterConfig.Enabled {


nit: move this check after if err != nil { below? or is it necessary to clear regardless?

Aah good catch

katrogan · 2020-10-07T00:34:24Z

go/tasks/plugins/array/k8s/task.go

+		return MonitorError, errors2.Wrapf(ErrCheckPodStatus, err, "Failed to check pod status.")
+	}
+
+	logger.Debugf(ctx, "Monitor - phase info %v", phaseInfo)


nit: add task index to this?

Removed. For debug. Not useful.

katrogan · 2020-10-07T00:38:31Z

go/tasks/plugins/array/k8s/task.go

+
+			return nil
+		}
+		return err


should this be wrapped flytestdlib.error?

Not sure. Any reason to?

we have been wrapping it in stdlib error if we want to achieve some specific behavior. But for plugins we want to make it simpler

👍 got it

katrogan · 2020-10-07T00:41:35Z

go/tasks/plugins/array/k8s/task.go

+
+	resourceNamespace := core.ResourceNamespace(config.ResourceConfig.PrimaryLabel)
+	resourceConstraintSpec := core.ResourceConstraintsSpec{
+		ProjectScopeResourceConstraint:   nil,


why are these nil?

Because we do not want any Project/Namespace based restriction today. We can add later if needed. I think it will not be used at all as resource quota will get a lot sooner. On top there is global limit as well.

I'm confused though, don't we need to specify some constraint in order to leverage resource manager?

or is the resourceNamespace that constraint and sufficient?

codecov-io · 2020-10-07T08:21:46Z

Codecov Report

Merging #120 into master will decrease coverage by 0.64%.
The diff coverage is 38.46%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
- Coverage   61.84%   61.20%   -0.65%     
==========================================
  Files         104      105       +1     
  Lines        5797     5941     +144     
==========================================
+ Hits         3585     3636      +51     
- Misses       1845     1930      +85     
- Partials      367      375       +8

Impacted Files	Coverage Δ
go/tasks/aws/config.go	`50.00% <ø> (ø)`
go/tasks/pluginmachinery/core/phase.go	`28.88% <0.00%> (-0.66%)`	⬇️
go/tasks/plugins/array/core/state.go	`37.70% <0.00%> (-1.96%)`	⬇️
go/tasks/plugins/array/k8s/launcher.go	`32.00% <0.00%> (-12.16%)`	⬇️
go/tasks/plugins/array/k8s/config.go	`10.71% <7.40%> (-89.29%)`	⬇️
go/tasks/plugins/array/k8s/executor.go	`38.14% <15.38%> (-8.61%)`	⬇️
go/tasks/plugins/array/k8s/task.go	`48.24% <48.24%> (ø)`
go/tasks/plugins/array/k8s/monitor.go	`55.23% <53.84%> (-0.06%)`	⬇️
go/tasks/plugins/array/k8s/config_flags.go	`61.29% <100.00%> (+13.46%)`	⬆️

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 0d28888...fac4413. Read the comment docs.

kumare3 · 2020-10-07T17:17:09Z

@anandswaminathan can you please associate this with the issue?

kumare3 · 2020-10-07T17:18:27Z

go/tasks/pluginmachinery/core/mocks/resource_negotiator.go

-import core "github.com/lyft/flyteplugins/go/tasks/pluginmachinery/core"
-import mock "github.com/stretchr/testify/mock"
+import (
+	context "context"


too many blank lines?

kumare3 · 2020-10-07T17:19:40Z

go/tasks/plugins/array/k8s/config.go

+	Limit        int    `json:"limit" pflag:",Resource quota (in the number of outstanding requests) for the cluster"`
+}
+
+type ClusterConfig struct {


can we put some comments here, explaining why we have a cluster config and why we need to do this? This seems a little complicated IMO

kumare3 · 2020-10-07T17:34:16Z

go/tasks/plugins/array/core/state.go

-		logger.Infof(ctx, "Array failed early because totalRunning[%v] + totalSuccesses[%v] < minSuccesses[%v]",
-			totalRunning, totalSuccesses, minSuccesses)
+	if totalRunning+totalSuccesses+totalWaitingForResources < minSuccesses {
+		logger.Infof(ctx, "Array failed early because totalRunning[%v] + totalSuccesses[%v] + totalWaitingForResource[%v] < minSuccesses[%v]",


Array failed early because number of failures are > minimum success requirements. Snapshot: TotalRunning[%d].....

do we have concurrency limits?

kumare3 · 2020-10-07T17:37:33Z

go/tasks/plugins/array/k8s/task.go

+			if strings.Contains(err.Error(), "exceeded quota") {
+				// TODO: Quota errors are retried forever, it would be good to have support for backoff strategy.
+				logger.Infof(ctx, "Failed to launch  job, resource quota exceeded. Err: %v", err)
+				t.State = t.State.SetPhase(arrayCore.PhaseWaitingForResources, 0).SetReason("Not enough resources to launch job")


is this the same as plugin manager?

As in ? yes. If you are referring to same phase? then yes

kumare3 · 2020-10-07T17:39:55Z

go/tasks/plugins/array/k8s/task.go

+	podName := formatSubTaskName(ctx, tCtx.TaskExecutionMetadata().GetTaskExecutionID().GetGeneratedName(), indexStr)
+	resourceNamespace := core.ResourceNamespace(config.ResourceConfig.PrimaryLabel)
+
+	err := tCtx.ResourceManager().ReleaseResource(ctx, resourceNamespace, podName)


is not found handled here?

Yup. I checked and confirmed logs. Will confirm with resource manager API as well.

anandswaminathan · 2020-10-08T00:14:07Z

PTAL @kumare3 @katrogan Addressed comments

migueltol22 · 2020-10-08T16:57:19Z

go/tasks/plugins/array/k8s/config.go

+
+// TODO: Move logic to flytestdlib
+// Reads secret values from paths specified in the config to initialize a Kubernetes rest client Config.
+func RemoteClusterConfig(host string, auth Auth) (*restclient.Config, error) {


What is the purpose of remoteCluster?

This is our V0 milestone. We do not have a way to prioritize regular tasks over array tasks in the same namespace. Array task will end up utilizing all the resource quotas, that will prevent all the other tasks from progressing.

katrogan · 2020-10-08T17:36:31Z

go/tasks/plugins/array/k8s/monitor.go

 		}

-		if phaseInfo.Info() != nil {
-			logLinks = append(logLinks, phaseInfo.Info().Logs...)
+		// The first time we enter this state we will launch every subtask. On subsequent rounds, the pod


which state? i don't see this wrapped in a check for a specific phase

the entire function - LaunchAndCheckSubTasksState . The specific phase is the caller - PhaseCheckingSubTaskExecutions

so we call Launch everytime we're in PhaseCheckingSubTaskExecutions?

is launch idempotent?

Yup. And we handle K8s already exists.

Not just launch, but also resource allocation.

katrogan

lgtm, but not sure if @kumare3 has more comments

* Support for remote endpoint * Unit tests for resource manager and k8s array. * Few minor fixes and updates. * Remove owner refs for remote execution. * Removed project/namespace limit for number of pods. * abort then finalize * rm unnecessary code & update state transition * Adding unit tests, remote endpoint and fixes Co-authored-by: Miguel Toledo <[email protected]>

migueltol22 and others added 30 commits March 11, 2020 15:54

add resource manager

d4efa26

launchandmonitor func

74523da

check allocation status

e2c3aae

fmt

e12e287

fix build

e7c226d

errors2

8ed48d4

update state machine

6090a7b

upd state machine

3bd1ef4

remove phaseLaunchAndMonitor state

b913806

generate

61f4751

initialize array if empty

d39b457

merge conflicts

78ee66d

create resource contraint spec

e371f27

add json doc for pflag

f2a16bf

use primary label

ae4f146

upd config and task launch

286425c

switch to enum return status

9cd4e4e

impl Monitor and Finalize in task

1a5c62c

use task in finalize

a72846d

upd comment

94f695d

upd Skip to Waiting

98409ba

lint and err

08b75e7

upd based on pr feedback

ba2a74d

upd allocate to return status, throw error on no containers

dd5db90

merge with master and remove launchSubTasks

438b0fd

lint

edd233e

upd pflag, launch and monitor status

5f5d1a9

abort then finalize

ad4ce20

rm unnecessary code & update state transition

050e8b2

Adding unit tests, remote endpoint and fixes

f6c4d72

MErge and fix

089f9ba

anandswaminathan added 3 commits September 28, 2020 17:34

update propeller

0b5580a

Fix config

e54c081

Fix resource manager and debug

50ccc6b

anandswaminathan force-pushed the k8-array-resource-manager branch from f616f34 to 50ccc6b Compare October 2, 2020 00:22

anandswaminathan added 2 commits October 5, 2020 15:37

Fix owner refs

68912a6

Fix owner refs

0d1961c

katrogan reviewed Oct 7, 2020

View reviewed changes

Fix and revert

cf9c388

anandswaminathan force-pushed the k8-array-resource-manager branch from ce45427 to cf9c388 Compare October 7, 2020 17:12

kumare3 reviewed Oct 7, 2020

View reviewed changes

comments

24c18e0

migueltol22 reviewed Oct 8, 2020

View reviewed changes

katrogan reviewed Oct 8, 2020

View reviewed changes

katrogan previously approved these changes Oct 12, 2020

View reviewed changes

anandswaminathan added 2 commits October 12, 2020 23:15

Merge branch 'new-master' into res-plug

85455ba

go mod

fac4413

anandswaminathan dismissed katrogan’s stale review via fac4413 October 13, 2020 06:16

katrogan approved these changes Oct 13, 2020

View reviewed changes

anandswaminathan merged commit f6148f4 into master Oct 13, 2020

K8 array resource manager #120

K8 array resource manager #120

Conversation

anandswaminathan commented Sep 15, 2020 • edited Loading

anandswaminathan commented Sep 17, 2020

anandswaminathan commented Oct 6, 2020

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

codecov-io commented Oct 7, 2020 • edited Loading

Codecov Report

kumare3 commented Oct 7, 2020

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

anandswaminathan commented Oct 8, 2020

Choose a reason for hiding this comment

anandswaminathan Oct 8, 2020 • edited Loading

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

anandswaminathan Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

katrogan left a comment

Choose a reason for hiding this comment

anandswaminathan commented Sep 15, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

anandswaminathan Oct 8, 2020 •

edited

Loading

anandswaminathan Oct 9, 2020 •

edited

Loading