Add AWS kfctl(golang) support and 0.6 support #3695

Jeffwan · 2019-07-19T19:39:32Z

Part of the work of #3151

This change is

Jeffwan · 2019-07-19T19:40:07Z

jlewi · 2019-07-22T00:42:28Z

HI @Jeffwan moving forward I'd like to try to get away from adding more command line flags. The desired experience will be closer to something like

kfctl init --config=<path to config file>

So rather than define new config flags; you should define an AWSPlugin

kubeflow/bootstrap/pkg/kfapp/gcp/gcp_plugin.go

Line 8 in e6944f3

type GcpPluginSpec struct {

And then any AWS fields (e.g. region) should be provided as part of that plugin and set in the KFDef file.
In kfctl you could add logic to try to intelligently set those defaults based on the user's locally configured preferences.

jlewi · 2019-07-22T00:45:18Z

bootstrap/config/kfctl_aws.yaml

+  namespace: kubeflow
+spec:
+  platform: aws
+  componentParams:


The fields packages, components, componentParams are the deprecated fields that were there for ksonnet.

If you want you can still use these fields but since its a new config I might suggest using the new field

Spec.applications

Each application is just a pointer to a kustomize package.

kfctl will automatically update kfdef.spec.applications based on packages, components,componentParams

so if you run kfctl and then look at app.yaml you can copy the value of KfDef.Spec.Applications from there.

Sure. I will make some clean ups as you suggest.

Define a plugin to avoid bring in more options

Use Applications instead.

jlewi

Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @Jeffwan, @kkasravi, @kunmingg, and @mameshini)

bootstrap/cmd/kfctl/cmd/apply.go, line 19 at r2 (raw file):

import (
	"fmt"

nit: can we revert this file just to get rid of spurious changes?

bootstrap/config/overlays/aws/kfctl_default.yaml, line 1 at r2 (raw file):

apiVersion: kfdef.apps.kubeflow.org/v1alpha1

I don't think you need these files.

kfctl used to be using kustomize to generate a KFDef in a YAML file
but now

kfctl takes a single parameter --config=

where the YAML file should fully specify KFDef

So if you want to have multiple configurations you what have multiple independent YAML files.

If you wanted to use kustomize to reduce boilerplate across those configs you could but the pattern would be to run kustomize and then check in the results.

bootstrap/pkg/apis/apps/kfdef/v1alpha1/application_types.go, line 37 at r2 (raw file):

	SkipInitProject        bool   `json:"skipInitProject,omitempty"`
	UseIstio               bool   `json:"useIstio"`
	ServerVersion          string `json:"serverVersion,omitempty"`

Did you mean to make this change? Maybe you just need to sync and pull in the latest changes?

bootstrap/pkg/kfapp/aws/aws.go, line 82 at r2 (raw file):

	// use aws to call sts get-caller-identity to verify aws credential works.
	if err := utils.CheckAwsStsCallerIdentity(sess); err != nil {

I think you would want to move this code to Generate and/or Apply

kubeflow/bootstrap/pkg/kfapp/gcp/gcp.go

Line 782 in ed40c4f

if err := gcp.initGcpClient(); err != nil {

kkasravi

Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @Jeffwan, @jlewi, @kkasravi, @kunmingg, and @mameshini)

bootstrap/config/overlays/aws/kfctl_default.yaml, line 1 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

I don't think you need these files.

kfctl used to be using kustomize to generate a KFDef in a YAML file
but now

kfctl takes a single parameter --config=

where the YAML file should fully specify KFDef

So if you want to have multiple configurations you what have multiple independent YAML files.

If you wanted to use kustomize to reduce boilerplate across those configs you could but the pattern would be to run kustomize and then check in the results.

I think keeping them would be ok since bootstrap/config/{base,overlays} will be moved into manifests and a tool can generate the file used for kfctl init --config=...

Jeffwan · 2019-07-24T20:07:51Z

/hold

Jeffwan · 2019-07-24T20:08:34Z

Changes are not ready yet. Still working on it. I will address above comments.

Jeffwan

Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @Jeffwan, @jlewi, @kunmingg, and @mameshini)

bootstrap/cmd/kfctl/cmd/apply.go, line 19 at r2 (raw file):

nit: can we revert this file just to get rid of spurious changes?
Looks like my editor auto format this fine and add one empty line. Revert it.

bootstrap/config/overlays/aws/kfctl_default.yaml, line 1 at r2 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

I think keeping them would be ok since bootstrap/config/{base,overlays} will be moved into manifests and a tool can generate the file used for kfctl init --config=...

Notice we need kfctl init {app_name} --config=.. since init.go check args.

bootstrap/config/{base,overlays} will give some template like this, if we want to use --config, where should we keep the preconfigured templates?

bootstrap/pkg/apis/apps/kfdef/v1alpha1/application_types.go, line 37 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Did you mean to make this change? Maybe you just need to sync and pull in the latest changes?

I will have a check. I may carelessly delete it in rebase.

bootstrap/pkg/kfapp/aws/aws.go, line 82 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

I think you would want to move this code to Generate and/or Apply

kubeflow/bootstrap/pkg/kfapp/gcp/gcp.go

Line 782 in ed40c4f

if err := gcp.initGcpClient(); err != nil {

Move to Generate/Apply/Delete

Jeffwan · 2019-07-25T19:43:56Z

/hold cancel

Jeffwan · 2019-07-25T19:47:52Z

@jlewi @kkasravi Changes are ready and I address the feedbacks you left and please have a check. Only confusion part is what's the best practice if we have different config patterns? Should we create different kfctl_aws_**.yaml template for users?

Use kustomize manner in config file. User should install kubeflow using command like this `kfctl init ${clusterName} --config=.
Extract aws options to aws plugin.
Revert some of unrelated changes.

kkasravi · 2019-07-26T05:21:45Z

Thanks @Jeffwan

@jlewi @kkasravi Changes are ready and I address the feedbacks you left and please have a check. Only confusion part is what's the best practice if we have different config patterns? Should we create different kfctl_aws_**.yaml template for users?

Could you explain a bit more on the different config patterns or point me to the relevanant part(s), or paste several kfctl_aws_*.yaml's you had in mind?

jlewi

Reviewed 2 of 18 files at r1, 4 of 12 files at r2, 9 of 9 files at r3.
Reviewable status: 14 of 16 files reviewed, 4 unresolved discussions (waiting on @Jeffwan, @jlewi, @kkasravi, @kunmingg, and @mameshini)

bootstrap/config/kfctl_aws.yaml, line 1 at r3 (raw file):

apiVersion: kfdef.apps.kubeflow.org/v1alpha1

Maybe add a comment explaining what this config is for?

bootstrap/config/kfctl_aws_cognito.yaml, line 1 at r3 (raw file):

apiVersion: kfdef.apps.kubeflow.org/v1alpha1

Maybe add a comment at the top of this file explaining what its for?

bootstrap/config/overlays/aws/kfctl_default.yaml, line 1 at r2 (raw file):

Previously, Jeffwan (Jiaxin Shan) wrote…

Notice we need kfctl init {app_name} --config=.. since init.go check args.

bootstrap/config/{base,overlays} will give some template like this, if we want to use --config, where should we keep the preconfigured templates?

I think the current location is fine. Lets follow up in
kubeflow/manifests#241

Did you mean to retain this now that you have kfctl_aws.yaml?

Looks like this is still using old style components, packages, componentParams

bootstrap/pkg/kfapp/aws/aws.go, line 82 at r2 (raw file):

Previously, Jeffwan (Jiaxin Shan) wrote…

Move to Generate/Apply/Delete

It looks like you are still initiating the clients and session here?

jlewi · 2019-07-26T17:57:20Z

/lgtm
/approve

@Jeffwan I left a couple comments but in the interest of trying to get this into the next RC I'm LGTM.

k8s-ci-robot · 2019-07-26T17:57:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~bootstrap/OWNERS~~ [jlewi]
~~deployment/aws/OWNERS~~ [jlewi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Jeffwan

Reviewable status: 14 of 16 files reviewed, 4 unresolved discussions (waiting on @Jeffwan, @jlewi, @kunmingg, and @mameshini)

bootstrap/config/overlays/aws/kfctl_default.yaml, line 1 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

I think the current location is fine. Lets follow up in
kubeflow/manifests#241

Did you mean to retain this now that you have kfctl_aws.yaml?

Looks like this is still using old style components, packages, componentParams

Yes. I used it in overlay kfctl_default.yaml. After I generate config and kustomize config are backfilled, I delete all components and packages in final template. Technically, this one is unnecessary now. I will check #241 for updates.

bootstrap/pkg/kfapp/aws/aws.go, line 82 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

It looks like you are still initiating the clients and session here?

Will move entire client initiation to later steps in next revision.

Jeffwan · 2019-07-26T19:00:24Z

Thanks @Jeffwan

@jlewi @kkasravi Changes are ready and I address the feedbacks you left and please have a check. Only confusion part is what's the best practice if we have different config patterns? Should we create different kfctl_aws_**.yaml template for users?

Could you explain a bit more on the different config patterns or point me to the relevanant part(s), or paste several kfctl_aws_*.yaml's you had in mind?

I image that user want simple setups can use bootstrap/config/kfctl_aws.yaml and users want entire aws service integrations like auth, file storage should use bootstrap/config/kfctl_aws_coginito.yaml. Should I keep two config there? Or just use one kfctl_aws.yaml with basic setups and ask users to follow documentation to change yaml file like add different fields by themselves?

* Add AWS parameters in kfctl API * Add aws provision implementation * Add aws parameters in coordinator * Clean up logs and comments * Add config kustomization files for AWS * Update kfctl default components for aws * Update aws infra configs * Update kfctl aws settings * Clean up aws configs and boostrap process * Refactor aws.go and add aws plugin * Address feedbacks and refactor Generate/Apply * Update manifest files * Update aws kfctl config files

fabboe · 2019-07-30T13:51:13Z

deployment/aws/infra_configs/cluster_features.sh

@@ -1,14 +0,0 @@
-# privateAccess enable private access for your Amazon EKS cluster's Kubernetes API server endpoint


Hi @Jeffwan - thanks for your work on this. Did you delete this intentionally? For v0.6.1 this broke https://github.com/kubeflow/kubeflow/blob/master/scripts/aws/util.sh - which relies on cluster_features.sh

Following https://www.kubeflow.org/docs/aws/deploy/install-kubeflow/ with export KUBEFLOW_TAG=v0.6.1

Oh. I see. Thanks for letting me know. This will be only used in 0.5.1 and 0.6 has few changes for this. I will update docs this week to give latest guidance.

Cool - thanks!

* Add AWS parameters in kfctl API * Add aws provision implementation * Add aws parameters in coordinator * Clean up logs and comments * Add config kustomization files for AWS * Update kfctl default components for aws * Update aws infra configs * Update kfctl aws settings * Clean up aws configs and boostrap process * Refactor aws.go and add aws plugin * Address feedbacks and refactor Generate/Apply * Update manifest files * Update aws kfctl config files

googlebot added the cla: yes label Jul 19, 2019

k8s-ci-robot added the size/XXL label Jul 19, 2019

k8s-ci-robot requested review from kunmingg and mameshini July 19, 2019 19:39

k8s-ci-robot requested a review from kkasravi July 19, 2019 19:40

k8s-ci-robot assigned jlewi Jul 22, 2019

jlewi reviewed Jul 22, 2019

View reviewed changes

Jeffwan force-pushed the aws-golang-kfctl branch from 33de57c to e5f3960 Compare July 24, 2019 00:54

jlewi suggested changes Jul 24, 2019

View reviewed changes

kkasravi reviewed Jul 24, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold label Jul 24, 2019

Jeffwan changed the title ~~Add AWS kfctl(golang) support and 0.6 support~~ [WIP] Add AWS kfctl(golang) support and 0.6 support Jul 24, 2019

k8s-ci-robot added the do-not-merge/work-in-progress label Jul 24, 2019

Jeffwan added 11 commits July 24, 2019 13:43

Add AWS parameters in kfctl API

b46c16d

Add aws provision implementation

6cd919a

Add aws parameters in coordinator

2c466b1

Clean up logs and comments

8468420

Add config kustomization files for AWS

bad4abf

Update kfctl default components for aws

b3bbe26

Update aws infra configs

cd0754c

Update kfctl aws settings

9c9dde9

Clean up aws configs and boostrap process

50cb4ac

Refactor aws.go and add aws plugin

1620b23

Address feedbacks and refactor Generate/Apply

37ca547

Jeffwan force-pushed the aws-golang-kfctl branch from e5f3960 to 37ca547 Compare July 25, 2019 17:41

Jeffwan commented Jul 25, 2019

View reviewed changes

Update manifest files

433b3a4

Jeffwan changed the title ~~[WIP] Add AWS kfctl(golang) support and 0.6 support~~ Add AWS kfctl(golang) support and 0.6 support Jul 25, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Jul 25, 2019

Update aws kfctl config files

f13869b

k8s-ci-robot removed the do-not-merge/hold label Jul 25, 2019

jlewi suggested changes Jul 26, 2019

View reviewed changes

k8s-ci-robot added the lgtm label Jul 26, 2019

k8s-ci-robot added the approved label Jul 26, 2019

k8s-ci-robot merged commit 32b7443 into kubeflow:master Jul 26, 2019

Jeffwan commented Jul 26, 2019

View reviewed changes

Jeffwan deleted the aws-golang-kfctl branch July 26, 2019 19:00

jlewi mentioned this pull request Jul 26, 2019

Update AWS deploy to use ISTIO and Kustomize for 0.6 #3151

Closed

fabboe reviewed Jul 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AWS kfctl(golang) support and 0.6 support #3695

Add AWS kfctl(golang) support and 0.6 support #3695

Jeffwan commented Jul 19, 2019 •

edited by jlewi

Loading

Jeffwan commented Jul 19, 2019

jlewi commented Jul 22, 2019

jlewi Jul 22, 2019

Jeffwan Jul 22, 2019

jlewi Jul 22, 2019

jlewi left a comment

kkasravi left a comment

Jeffwan commented Jul 24, 2019

Jeffwan commented Jul 24, 2019

Jeffwan left a comment

Jeffwan commented Jul 25, 2019

Jeffwan commented Jul 25, 2019 •

edited

Loading

kkasravi commented Jul 26, 2019

jlewi left a comment

jlewi commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

Jeffwan left a comment

Jeffwan commented Jul 26, 2019

fabboe Jul 30, 2019

Jeffwan Jul 30, 2019

fabboe Jul 31, 2019

		@@ -1,14 +0,0 @@
		# privateAccess enable private access for your Amazon EKS cluster's Kubernetes API server endpoint

Add AWS kfctl(golang) support and 0.6 support #3695

Add AWS kfctl(golang) support and 0.6 support #3695

Conversation

Jeffwan commented Jul 19, 2019 • edited by jlewi Loading

Jeffwan commented Jul 19, 2019

jlewi commented Jul 22, 2019

jlewi Jul 22, 2019

Choose a reason for hiding this comment

Jeffwan Jul 22, 2019

Choose a reason for hiding this comment

jlewi Jul 22, 2019

Choose a reason for hiding this comment

jlewi left a comment

Choose a reason for hiding this comment

kkasravi left a comment

Choose a reason for hiding this comment

Jeffwan commented Jul 24, 2019

Jeffwan commented Jul 24, 2019

Jeffwan left a comment

Choose a reason for hiding this comment

Jeffwan commented Jul 25, 2019

Jeffwan commented Jul 25, 2019 • edited Loading

kkasravi commented Jul 26, 2019

jlewi left a comment

Choose a reason for hiding this comment

jlewi commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

Jeffwan left a comment

Choose a reason for hiding this comment

Jeffwan commented Jul 26, 2019

fabboe Jul 30, 2019

Choose a reason for hiding this comment

Jeffwan Jul 30, 2019

Choose a reason for hiding this comment

fabboe Jul 31, 2019

Choose a reason for hiding this comment

Jeffwan commented Jul 19, 2019 •

edited by jlewi

Loading

Jeffwan commented Jul 25, 2019 •

edited

Loading