Added SetDefault() for each Reconcile function #464
Conversation
| @@ -76,6 +76,7 @@ func NewController(kubeClient client.Client, coreV1Client corev1.CoreV1Interface | |||
| // Reconcile executes an allocation control loop for the resource | |||
| func (c *Controller) Reconcile(ctx context.Context, object client.Object) (reconcile.Result, error) { | |||
| provisioner := object.(*v1alpha1.Provisioner) | |||
| provisioner.SetDefaults(ctx) | |||
There was a problem hiding this comment.
Can you use the defaultable interface and put this in the generic controller?
pkg/controllers/controller.go
Outdated
| if _, err := c.Controller.Reconcile(ctx, resource); err != nil { | ||
| zap.S().Errorf("Controller failed to reconcile kind %s, %s", | ||
| resource.GetObjectKind().GroupVersionKind().Kind, err.Error()) | ||
| return reconcile.Result{Requeue: true}, nil | ||
| } | ||
| // 5. Update Status using a merge patch | ||
| // 6. Update Status using a merge patch | ||
| if !reflect.DeepEqual(resource, persisted) { |
There was a problem hiding this comment.
Unrelated to your PR, but I realized the other day that we should be comparing statuses and patching statuses, or comparing objects and patching objects. This is currently mismatched: we compare the objects, but only patch the statuses. If resource has a changed spec, we'll send an empty patch.
There was a problem hiding this comment.
Additionally, if we only update the status, this actually won't call the patch. I think until we have the statuses working, we should delete this line.
There was a problem hiding this comment.
SGTM -- can you own this?
pkg/controllers/controller.go
Outdated
| if _, ok := resource.(apis.Defaultable); ok { | ||
| resource.(apis.Defaultable).SetDefaults(ctx) | ||
| } | ||
| // 4. Set to true to remove race condition | ||
| // TODO: remove status conditions on provisioners | ||
| if provisioner, ok := resource.(*v1alpha1.Provisioner); ok { |
There was a problem hiding this comment.
Similarly, can you use the condition manager interface while you're in here? This would remove any special provisioner logic in this file.
| @@ -72,7 +71,6 @@ var _ = Describe("Reallocation", func() { | |||
| }, | |||
| Spec: v1alpha1.ProvisionerSpec{ | |||
| Cluster: &v1alpha1.ClusterSpec{Name: "test-cluster", Endpoint: "http://test-cluster", CABundle: "dGVzdC1jbHVzdGVyCg=="}, | |||
| TTLSeconds: ptr.Int32(300), | |||
There was a problem hiding this comment.
Does your test rely on the TTL behavior at all? Will it change tests to have nodes instantly deleted after underutilized?
There was a problem hiding this comment.
At the end of the first test in this suite, it checks to see if the TTL annotation is part of the node. Therefore, if our default logic fails here, the test will fail.
pkg/controllers/controller.go
Outdated
| if provisioner, ok := resource.(*v1alpha1.Provisioner); ok { | ||
| provisioner.StatusConditions().MarkTrue(v1alpha1.Active) | ||
| if _, ok := resource.(apis.ConditionManager); ok { | ||
| resource.(apis.ConditionManager).SetCondition(apis.Condition{ |
There was a problem hiding this comment.
I think you can do
if conditionManager, ok := resource.(apis.ConditionManager); ok {
conditionsManager.StatusConditions().MarkTrue(v1alpha1.Active)
}
There was a problem hiding this comment.
the knative apis.ConditionManager interface does not have a StatusConditions() method.
pkg/controllers/controller.go
Outdated
| @@ -49,22 +49,24 @@ func (c *GenericController) Reconcile(ctx context.Context, req reconcile.Request | |||
| } | |||
| // 2. Copy object for merge patch base | |||
| persisted := resource.DeepCopyObject() | |||
| // 3. Set to true to remove race condition | |||
| // 3. Set Defaults | |||
There was a problem hiding this comment.
Would be nice to explain why here - I could see eventually someone removes this code because they think it's redundant
pkg/controllers/controller.go
Outdated
| if _, ok := resource.(apis.Defaultable); ok { | ||
| resource.(apis.Defaultable).SetDefaults(ctx) | ||
| } | ||
| // 4. Set to true to remove race condition |
There was a problem hiding this comment.
This comment is confusing - what race condition?
There was a problem hiding this comment.
If we have multiple controllers reconciling a provisioner, they can set the status of the provisioner to different statuses, where last write wins. We're setting this to true for now while we change what resources the controllers are reconciling. I'll add this in too to make it clearer.
* reorder drift * tests
Issue #, if available:
#463
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.