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

Fix scheduler in helm #1896

Merged
merged 12 commits into from
Dec 7, 2021
Merged

Fix scheduler in helm #1896

merged 12 commits into from
Dec 7, 2021

Conversation

yindia
Copy link
Contributor

@yindia yindia commented Dec 3, 2021

Enable/disable scheduler with workflow_scheduler.enabled, If use want to use aws scheduler then he needs to define workflow_scheduler.type: aws or else helm will install the default scheduler.

Sandbox: Sandbox will use native as default scheduler

workflow_scheduler:
  enabled: true
  type: native

GCP: GCP will use native as default scheduler

workflow_scheduler:
  enabled: true
  type: native

Eks: Eks will deploy use native as default scheduler

workflow_scheduler:
  enabled: true
  type: native

#To enable aws specific scheduler then

workflow_scheduler:
 enabled: true
  type: aws
  config:
    scheduler:
      # -- This is configured to use Cloudwatch schedules as explained [here](https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/Create-CloudWatch-Events-Scheduled-Rule.html)
      eventScheduler:
        scheme: aws
        region: "{{ .Values.userSettings.accountRegion }}"
        scheduleRole: "arn:aws:iam::{{ .Values.userSettings.accountNumber }}:role/flyte_cron_scheduler_role"
        targetName: "arn:aws:sqs:{{ .Values.userSettings.accountRegion }}:{{ .Values.userSettings.accountNumber }}:flyte-helm-test-cron-scheduler-queue"
        scheduleNamePrefix: flyte
      workflowExecutor:
        scheme: aws
        region: "{{ .Values.userSettings.accountRegion }}"
        scheduleQueueName: flyte-helm-test-cron-scheduler-queue
        accountId: "{{ .Values.userSettings.accountNumber }}"
        reconnectAttempts: 10
        reconnectDelaySeconds: 30

Test:

  • Tested on sandbox & eks.

Fix: #1891

Signed-off-by: Yuvraj <code@evalsocket.dev>
@pmahindrakar-oss
Copy link
Contributor

Having both workflow_scheduler and flytescheduler in helm causes confusion.
Can we juts keep one and have native or aws type and keep default as native in all deployments.

The additional config required for AWS based scheduler should be put only if aws scheduler is enabled and can be enabled through conditional template. And for native scheduler it has its own config
@EngHabu @evalsocket let me know what you all think

@yindia
Copy link
Contributor Author

yindia commented Dec 4, 2021

@pmahindrakar-oss Made changes as you suggested

Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
@yindia
Copy link
Contributor Author

yindia commented Dec 6, 2021

@pmahindrakar-oss It's ready for review

@pmahindrakar-oss
Copy link
Contributor

We seem to have removed the AWS scheduler config. Can we templatize and import it when aws type is enabled.

@yindia
Copy link
Contributor Author

yindia commented Dec 6, 2021

@pmahindrakar-oss yeah that is done, it's not there because i enable flytescheduler as default

@pmahindrakar-oss
Copy link
Contributor

I understand that we have flytescheduler as default which is basically enabling native type , but what happens if we use aws type . How does the user get that config. Does he have to type in this config?

#  config:
#    scheduler:
#      # -- This is configured to use Cloudwatch schedules as explained [here](https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/Create-CloudWatch-Events-Scheduled-Rule.html)
#      eventScheduler:
#        scheme: aws
#        region: "{{ .Values.userSettings.accountRegion }}"
#        scheduleRole: "arn:aws:iam::{{ .Values.userSettings.accountNumber }}:role/flyte_cron_scheduler_role"
#        targetName: "arn:aws:sqs:{{ .Values.userSettings.accountRegion }}:{{ .Values.userSettings.accountNumber }}:flyte-helm-test-cron-scheduler-queue"
#        scheduleNamePrefix: flyte
#      workflowExecutor:
#        scheme: aws
#        region: "{{ .Values.userSettings.accountRegion }}"
#        scheduleQueueName: flyte-helm-test-cron-scheduler-queue
#        accountId: "{{ .Values.userSettings.accountNumber }}"
#        reconnectAttempts: 10
#        reconnectDelaySeconds: 30

EngHabu
EngHabu previously approved these changes Dec 6, 2021
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.

Looks good to me but please wait for Praful to sign off

Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
Signed-off-by: Yuvraj <code@evalsocket.dev>
script/generate_helm.sh Outdated Show resolved Hide resolved
charts/flyte-core/values-eks-override.yaml Outdated Show resolved Hide resolved
Signed-off-by: Yuvraj <code@evalsocket.dev>
@yindia yindia merged commit b4d66e2 into master Dec 7, 2021
@yindia yindia deleted the feature/scheduler-check branch December 7, 2021 12:51
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.

[BUG] .Values.flytescheduler.enabled is not implemented
3 participants