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

Add notifications, schedules to new flytekit #280

Merged
merged 9 commits into from
Dec 17, 2020
Merged

Conversation

katrogan
Copy link
Contributor

No description provided.

@wild-endeavor
Copy link
Contributor

Can we broaden this PR a bit? Instead of just adding notifications, can we also add schedules, auth (probably as two fields - iam role or service account), and rawoutputdataconfig?

Also why do we need to create a new Notification class? Can we use flytekit.models.common.Notification, flytekit.models.schedule.Schedule, flytekit.models.common.AuthRole, and flytekit.models.common.RawOutputDataConfig

Also, where did we land on stuff having both iam role and service account? @anandswaminathan is that a thing we decided to do?

@EngHabu
Copy link
Collaborator

EngHabu commented Dec 16, 2020

@anandswaminathan has a doc on deprecating KIAM... I'm not sure if it's public (I think it's ok to make public... if so, please do @anandswaminathan)... I do NOT recall that we actually decided this is the way to go... here is my recollection:

  • First iteration: Let user repos provision these service accounts into flyte clusters.. CON: we keep fiddling with these clusters all the time, what happens if we move them to different clusters, or we add new clusters... etc. etc.
  • Second iteration: Admin will provision these service accounts before it launches a launchplan.. CON: additional steps, more prune to errors

Honorable mentioned:

  • Admin provisions these service accounts along with cluster resource manager templates. CON: Launchplans can define their own IAM roles that where never previously provisioned with admin and we like to keep this flexibility.

Orthogonal to all of this: We continue to need IAM roles for AWS Batch plugin.

@anandswaminathan ?

@katrogan
Copy link
Contributor Author

katrogan commented Dec 16, 2020

@wild-endeavor schedules, roles, etc is all in the pipeline :) i'm still not very familiar with new flytekit so i prefer a piecemeal approach for PRs so i can get feedback before doing a lot more work

Also why do we need to create a new Notification class?

For validation that notification phases are only terminal and to simplify the oneof structure for notification types that is specific to flyteidl but not necessarily simpler for users to grok

@katrogan katrogan changed the title Add notifications to new flytekit Add notifications, schedules to new flytekit Dec 16, 2020
@katrogan
Copy link
Contributor Author

katrogan commented Dec 16, 2020

@wild-endeavor mind taking another look? the lp auth role is already implemented https://github.com/lyft/flytekit/blob/annotations/flytekit/annotated/launch_plan.py#L189,L194

my understanding was that we would change the auth role to optionally include both an assumable iam role AND a k8s service account instead of the current one of structure. but nothing has changed in idl so i think we can mirror the existing behavior for now

from flytekit.models.core import execution as _execution_model


class Notification(_common_model.Notification):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment that this is duplicate because we don't want to deal with metaclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

flytekit/annotated/schedule.py Show resolved Hide resolved
flytekit/models/schedule.py Show resolved Hide resolved
@katrogan
Copy link
Contributor Author

@honnix

@katrogan katrogan merged commit ca8ef05 into annotations Dec 17, 2020
@honnix
Copy link
Member

honnix commented Dec 17, 2020

Thanks for implementing this! Regarding auth role, how would a user pass in when creating a launch plan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants