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

Deploy with new scheduler #729

Merged
merged 14 commits into from
Sep 26, 2022
Merged

Deploy with new scheduler #729

merged 14 commits into from
Sep 26, 2022

Conversation

hunhoffe
Copy link
Contributor

@hunhoffe hunhoffe commented May 6, 2022

What this PR contains:

  • Deploy etcd and scheduler components
  • Allow option to enable or disable the scheduler (similar to ansible)
  • Akka cluster config setup for replicating the scheduler
  • Tested with and without scheduler enabled on 3-node Kubernetes cluster with CouchDB activation store

Some changes are needed to the scheduler to support the akka cluster discovery mechanism. The scheduler also needs to be integrated into the build process. These changes are in PR#5278 of apache/openwhisk.

Tasks:

  • Merge/finalize PR for apache/openwhisk with Akka cluster management changes for scheduler, controller & updates to scripts that create docker images, etc.
  • Update docker image tags once images are generated
  • Update documentation

Future Work:

  • etcd will currently always start fresh as a deployment specifically for an openwhisk deployment; future work could be to add persistence and also to support external etcd. I added in documentation that persistence to etcd was not currently supported and in a comment that the external etcd was not supported.
  • Does not support dedicated invoker feature, e.g., Dedicated Invokers openwhisk#5292

@hunhoffe hunhoffe changed the title Proof-of-concept for running new scheduler Deploy with new scheduler Jul 12, 2022
@hunhoffe
Copy link
Contributor Author

hunhoffe commented Jul 13, 2022

Having issues with inProgressJobRetention parsing in new scheduler image, see: apache/openwhisk#5281

@hunhoffe hunhoffe marked this pull request as ready for review September 14, 2022 02:34
helm/openwhisk/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

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

I am a bit outdated regarding this repo as I am using a bit different chart and scripts to deploy ow on k8s in my downstream.
But anyway I think at least the followings are required.

tools/travis/deploy-chart.sh Outdated Show resolved Hide resolved
helm/openwhisk/values.yaml Outdated Show resolved Hide resolved
docs/configurationChoices.md Show resolved Hide resolved
helm/openwhisk/templates/scheduler-pod.yaml Outdated Show resolved Hide resolved
helm/openwhisk/templates/scheduler-pod.yaml Show resolved Hide resolved
helm/openwhisk/templates/scheduler-pod.yaml Outdated Show resolved Hide resolved
helm/openwhisk/templates/_helpers.tpl Show resolved Hide resolved
@style95
Copy link
Member

style95 commented Sep 15, 2022

@hunhoffe
This is great!
Thank you for your contribution.

@hunhoffe
Copy link
Contributor Author

hunhoffe commented Sep 21, 2022

I believe I've addressed all comments except the external etcd feature. I wrote a comment that it is not yet supported in the values.yaml file, and I'm hoping that it is a feature that could be added later; I could create an issue for it so it is not forgotten if that's acceptable.

@style95
Copy link
Member

style95 commented Sep 22, 2022

I could create an issue for it so it is not forgotten if that's acceptable.

I am ok with this.

@hunhoffe
Copy link
Contributor Author

I created an issue for the external etcd support and fixed the timestamp (#751). Pending any further feedback, I believe this is ready for merge. Thank you for your time in reviewing @style95 and @dgrove-oss!

@hunhoffe hunhoffe removed the request for review from style95 September 22, 2022 15:53
@hunhoffe hunhoffe requested review from dgrove-oss and style95 and removed request for dgrove-oss September 22, 2022 15:53
Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dgrove-oss dgrove-oss left a comment

Choose a reason for hiding this comment

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

This is great! Sorry to be slow reviewing...time for away from me last week.

@dgrove-oss dgrove-oss merged commit ba72e9a into apache:master Sep 26, 2022
@hunhoffe hunhoffe deleted the newscheduler branch September 26, 2022 18:35
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.

3 participants