-
Notifications
You must be signed in to change notification settings - Fork 805
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 Schedule #1713
Add Schedule #1713
Conversation
Signed-off-by: Yang Keao <keao.yang@yahoo.com>
Signed-off-by: Yang Keao <keao.yang@yahoo.com>
Signed-off-by: Yang Keao <keao.yang@yahoo.com>
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
return ctrl.Result{}, nil | ||
} | ||
|
||
kind, ok := v1alpha1.AllKinds()[string(schedule.Spec.Type)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1alpha1.AllKinds()
seems to contains only chaos type only, workflow type is not supported yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the support for Workflow
, I need another discussion with @STRRL (as it needs the finished status to GC the history). I prefer to include it in another PR as this one is too big.
workers: 1 | ||
load: 100 | ||
options: ["--cpu 2", "--timeout 600", "--hdd 1"] | ||
duration: "30s" | ||
scheduler: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we would not test scheduler seperately in every chaos. Instead we could test it once and for all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But we nearly didn't test scheduler in previous tests 🤦 .
I tried to write an integration test for the scheduler, through the sigs.k8s.io/controller-runtime/pkg/envtest
. Would you like to help and make it easy to define and add case ? (And I think it could be added in another PR, as this one is big enough 🤦 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
umm, I'm a bit coinfused. Does that mean we would use envtest
to create a virtual cluster, register crd and controllers, and run tests on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Some tests are already using it, e.g. api/v1alpha1/suite_test.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that suite_test.go
is good to use on crds, but it has no controllers. Could we just reuse that?
Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: YangKeao <keao.yang@yahoo.com>
Signed-off-by: YangKeao <keao.yang@yahoo.com>
It seems no one want to review it yet. But we have to review it in the final PR about merging nirvana into the master 😛 Merge it to not block the development roadmap. |
What problem does this PR solve?
Fix #1693
TODO
ConcurrencyPolicy