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.

Sign up for GitHub

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

Jump to bottom

Added flyte scheduler in helm #1374

Merged
merged 26 commits into from
Sep 25, 2021
Merged

Added flyte scheduler in helm #1374

merged 26 commits into from
Sep 25, 2021

Conversation

yindia
Copy link
Contributor

@yindia yindia commented Aug 25, 2021 •
edited
Loading

Test:
Override helm values

workflow_scheduler:
  enabled: true
  type: native
 
flytescheduler:
  # -- Replicas count for Flyteadmin deployment
  replicaCount: 1
  image:
    # -- Docker image for Flyteadmin deployment
    repository: evalsocket/flytescheduler
    # -- Docker image tag
    tag: latest # FLYTEADMIN_TAG
    # -- Docker image pull policy
    pullPolicy: IfNotPresent

Install Flyte

helm install -n flyte -f values-sandbox.yaml --create-namespace flyte .

Test:

  • if flytescheduler pre-check will wait for flyteadmin in the init container...It will only start after flyteadmin is healthy
  • If flyteadmin didn't start successfully then flytescheduler will retry and after retry exhaustion it will restart the pod
  • If flyteadmin goes down after flytescheduler init container success then pre-check will not responsible, it will be handle by scheduler retry

Not Tested:

  • Pre check with auth

Signed-off-by: Yuvraj [email protected]

yindia added 2 commits August 25, 2021 20:41
@yindia
make helm
1e82019 
Signed-off-by: Yuvraj <[email protected]>
@yindia
Added common secret
1aa1f25 
Signed-off-by: Yuvraj <[email protected]>
@yindia yindia changed the title [WIP] Added flyte scheduler in helm Added flyte scheduler in helm Aug 25, 2021
@yindia yindia marked this pull request as ready for review August 25, 2021 15:52
@yindia yindia requested a review from sbrunk August 25, 2021 15:56
EngHabu
EngHabu reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

We shouldn't merge this until we merge flytescheduler PR... otherwise, looks good!

charts/flyte/templates/common/secret-auth.yaml Outdated Show resolved Hide resolved
yindia added 4 commits August 31, 2021 11:36
@yindia
Added scheduler in flyte-core
fded008 
Signed-off-by: Yuvraj <[email protected]>
@yindia
small fix
9461c0b 
Signed-off-by: Yuvraj <[email protected]>
@yindia
more changes
a74ab71 
Signed-off-by: Yuvraj <[email protected]>
@yindia
make helm
3b90008 
Signed-off-by: Yuvraj <[email protected]>
@pmahindrakar-oss
Copy link
Contributor

We need to be able to scale admin and scheduler services separately.
The current changes will cause multiple scheduler instances if admin replicas are increased which we dont want

@pmahindrakar-oss pmahindrakar-oss mentioned this pull request Sep 6, 2021
Closed
@yindia
Rebase master
719a578 
Signed-off-by: Yuvraj <[email protected]>
EngHabu
EngHabu reviewed Sep 16, 2021
View reviewed changes
charts/flyte-core/templates/flytescheduler/configmap.yaml Outdated Show resolved Hide resolved
charts/flyte-core/templates/flytescheduler/deployment.yaml Show resolved Hide resolved
charts/flyte-core/templates/propeller/deployment.yaml Outdated
@@ -48,7 +48,7 @@ spec:
name: config-volume
- name: auth
secret:
secretName: flyte-propeller-auth
secretName: flyte-common-auth
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no i moved it in a commen secret

charts/flyte-core/values-eks.yaml Show resolved Hide resolved
charts/flyte-core/values.yaml Outdated Show resolved Hide resolved
charts/flyte/templates/common/secret-auth.yaml Outdated Show resolved Hide resolved
@yindia
drop changes
c065d5f 
Signed-off-by: Yuvraj <[email protected]>
@yindia
rebase
677a062 
Signed-off-by: Yuvraj <[email protected]>
@yindia
drop common changes
ac4492d 
Signed-off-by: Yuvraj <[email protected]>
@yindia yindia requested a review from EngHabu September 18, 2021 07:12
EngHabu
EngHabu reviewed Sep 19, 2021
View reviewed changes
charts/flyte-core/templates/flytescheduler/configmap.yaml Outdated Show resolved Hide resolved
charts/flyte/templates/flytescheduler/configmap.yaml Outdated Show resolved Hide resolved
charts/flyte/values-eks.yaml Outdated Show resolved Hide resolved
charts/flyte/values.yaml Show resolved Hide resolved
@yindia
More changes
67ffdd7 
Signed-off-by: Yuvraj <[email protected]>
@yindia yindia requested a review from EngHabu September 21, 2021 16:02
EngHabu
EngHabu reviewed Sep 21, 2021
View reviewed changes
charts/flyte-core/values.yaml Outdated Show resolved Hide resolved
charts/flyte/values.yaml Show resolved Hide resolved
charts/flyte/values.yaml Outdated Show resolved Hide resolved
deployment/eks/flyte_helm_generated.yaml Outdated Show resolved Hide resolved
deployment/sandbox/flyte_helm_generated.yaml Outdated Show resolved Hide resolved
deployment/sandbox/flyte_helm_generated.yaml Outdated Show resolved Hide resolved
@yindia
more changes
6627d6e 
Signed-off-by: Yuvraj <[email protected]>
@yindia
rebase master
3c48500 
Signed-off-by: Yuvraj <[email protected]>
@yindia
small config fixes
bba16d3 
Signed-off-by: Yuvraj <[email protected]>
EngHabu
EngHabu previously approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Just a couple of nits...

charts/flyte-core/values.yaml Outdated Show resolved Hide resolved
charts/flyte/values.yaml Outdated Show resolved Hide resolved
deployment/gcp/flyte_helm_generated.yaml
app.kubernetes.io/managed-by: Helm
spec:
initContainers:
- command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need to run migrations here too? this can be added in the future...

Copy link
Contributor Author

@yindia yindia Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No, currently flyteadmin will handle the migration...When we have a separate DB for flytescheduler then we can run migration here
CC: @pmahindrakar-oss

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @EngHabu , we are relying on the admin running the migration script for now but it can easily be decoupled to run as a separate init container for scheduler specific tables in the future

@yindia
more changes
639b6d2 
Signed-off-by: Yuvraj <[email protected]>
@pmahindrakar-oss pmahindrakar-oss merged commit e3f9211 into master Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmahindrakar-oss pmahindrakar-oss pmahindrakar-oss approved these changes

@pradithya pradithya pradithya left review comments

@EngHabu EngHabu EngHabu left review comments

@kumare3 kumare3 Awaiting requested review from kumare3

@katrogan katrogan Awaiting requested review from katrogan

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor

@sbrunk sbrunk Awaiting requested review from sbrunk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yindia @pmahindrakar-oss @kumare3 @EngHabu @pradithya