Add options structs and timeouts to orch package #100
Conversation
codeblooded
added
feature
This change adds an options struct for the Controller and Watcher types, allowing them to be configured. Previously, they used magic constants.
The Stop method of the orch.Controller type accepted a time.Duration. It would wait for a graceful shutdown for this amount of time. Then, it would return an error if the graceful shutdown did not occur. This commit replaces the time.Duration with a context.Context, permitting more flexibility.
codeblooded
force-pushed
the
feature/controller-options
branch
from
c6600db to
c3458e1
Compare
codeblooded
changed the title
codeblooded
force-pushed
the
feature/controller-options
branch
3 times, most recently
from
4f323e1 to
d45f281
Compare
| // The options value allows the controller to be customized. Specifying nil will | ||
| // configure the controller to sane defaults described in the ControllerOptions | ||
| // documentation. | ||
| func NewController(clientset kubernetes.Interface, store store.Store, options *ControllerOptions) (*Controller, error) { |
There was a problem hiding this comment.
I'm not sure if it is best to make the ControllerOptions parameter a pointer. It allows someone to specify nil and inherit defaults, but it decreases the readability of function calls. It may be better to have callers specify ControllerOptions{} to inherit the defaults. I believe I have seen both with Kubernetes. Would love input here.
There was a problem hiding this comment.
I don't think it is too important. If it is not a pointer you would write ControllerOptions{} where you now write nil, and it avoids some logic inside the controller, so it may be a reasonable thing to do.
| func TestNewController(t *testing.T) { | ||
| t.Run("nil clientset returns error", func(t *testing.T) { | ||
| controller, err := NewController(nil, nil) | ||
| controller, err := NewController(nil, nil, nil) |
There was a problem hiding this comment.
All tests currently use the default options. In the future, we may want to add tests to experiment with multiple combinations. The defaults are very sane, so I doubt we will deviate much.
There was a problem hiding this comment.
I assume that means testing scheduling with two executors or with a timeout or with different watcher options. It may be ok to defer these, although eventually it would be good to test for instance that a second test can be scheduled when you have two executors, and so on.
codeblooded
force-pushed
the
feature/controller-options
branch
from
fa9bd72 to
65465d2
Compare
This change implements timeouts on orchestration events, preventing rouge operations from blocking indefinitely. Specifically, it: - Switches the Executor.Execute method to accept a context.Context - Modifies (*kubeExecutor).provision and (*kubeExecutor).monitor to use a context.Context. They now return an error when the context is cancelled before completion. - Adds a TestTimeout field to ControllerOptions. This sets the timeout on the Executor's context. It also performs a handful of clean-up tasks, including: - Using UUIDs to name executors - Making TestKubeExecutorProvision a table-driven test
codeblooded
force-pushed
the
feature/controller-options
branch
2 times, most recently
from
7ca9edc to
679826d
Compare
The flag -testTimeout specifies the maximum duration a session can take to provision resources and run a test. If this duration is exceeded, the session is killed and an error is returned.
Previously, the ControllerOptions struct copied a field from the WatcherOptions struct, leading to duplicated logic. This change adds a pointer to WatcherOptions in the ControllerOptions. This allows the watcher's options to be handled in one place.
codeblooded
force-pushed
the
feature/controller-options
branch
from
679826d to
31e741a
Compare
The TestExecutorProvision function includes a test case to ensure an error is returned when the context is cancelled. The case lacked a driver and client. The server was in an unhealthy status. In a future version, the provision function may have returned errors for these issues, not the cancelled context. This change adds all necessary components with a healthy status, ensuring the error is due to the cancelled context.
codeblooded
added
bug
| // The options value allows the controller to be customized. Specifying nil will | ||
| // configure the controller to sane defaults described in the ControllerOptions | ||
| // documentation. | ||
| func NewController(clientset kubernetes.Interface, store store.Store, options *ControllerOptions) (*Controller, error) { |
There was a problem hiding this comment.
I don't think it is too important. If it is not a pointer you would write ControllerOptions{} where you now write nil, and it avoids some logic inside the controller, so it may be a reasonable thing to do.
| func TestNewController(t *testing.T) { | ||
| t.Run("nil clientset returns error", func(t *testing.T) { | ||
| controller, err := NewController(nil, nil) | ||
| controller, err := NewController(nil, nil, nil) |
There was a problem hiding this comment.
I assume that means testing scheduling with two executors or with a timeout or with different watcher options. It may be ok to defer these, although eventually it would be good to test for instance that a second test can be scheduled when you have two executors, and so on.
This change adds an options struct for the Controller and Watcher types, allowing them to be configured. Previously, they used magic constants.