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.

Sign up for GitHub

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 unit tests for raycluster_controller reconcilePods function #219

Merged
merged 9 commits into from
Apr 5, 2022
Merged

Add unit tests for raycluster_controller reconcilePods function #219

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
82c90ee
Write raycluster_controller reconcilePods unit tests
Apr 4, 2022
86258fe
Improve unit tests code
Waynegates Apr 4, 2022
b0c6de8
Fix lint issue and Improve unit tests code
Waynegates Apr 4, 2022
6e3db6c
Run goimports
Waynegates Apr 4, 2022
37fb342
Fix a bug of workersToDelete update and update unit tests
Waynegates Apr 4, 2022
be89699
Fix a bug of workersToDelete update and update unit tests
Waynegates Apr 4, 2022
f34632e
Update unit tests log
Waynegates Apr 4, 2022
84d5e09
Update workersToDelete local var to avoid unuseful kube api server de…
Waynegates Apr 5, 2022
0138e28
Remove code of updating instance spec workersToDelete
Waynegates Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ray-operator/controllers/raycluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,11 +265,12 @@ func (r *RayClusterReconciler) reconcilePods(instance *rayiov1alpha1.RayCluster)
}
log.Info("reconcilePods", "unable to delete worker ", pod.Name)
} else {
diff--
diff++
r.Recorder.Eventf(instance, v1.EventTypeNormal, "Deleted", "Deleted pod %s", pod.Name)
}
}
worker.ScaleStrategy.WorkersToDelete = []string{}
instance.Spec.WorkerGroupSpecs[index].ScaleStrategy.WorkersToDelete = []string{}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug fix to clear the instance.Spec.WorkerGroupSpecs[index].ScaleStrategy.WorkersToDelete

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that precisely what the line above is doing?

worker = instance.Spec.WorkerGroupSpecs[index]

as per the outer for loop :)

Copy link
Contributor Author

@Waynegates Waynegates Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually no. WorkerGroupSpecs []WorkerGroupSpec is not WorkerGroupSpecs []*WorkerGroupSpec. So update worker will not update instance.Spec.WorkerGroupSpecs[index].
This is found by the unit tests. So instance.Spec.WorkerGroupSpecs[index].ScaleStrategy.WorkersToDelete = []string{} is needed.
Check this for more info.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for catching this bug.

Could you replace line 239 with:

for index := range instance.Spec.WorkerGroupSpecs {
        worker = &instance.Spec.WorkerGroupSpecs[index]

then you would not require both lines 272 and 273

However, even after you do this, does the CR actually get updated? The only place I see this happening is within updateStatus() which has:

if err := r.Status().Update(context.Background(), instance); err != nil {

Doesn't this only update the Status and not the Spec? See https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client#StatusClient

So how does this clear WorkersToDelete in the CR?

Separately (strictly speaking) the operator should not be changing the Spec, it should only update the Status given the declarative nature of the k8s programming style. But in this case I would say it is OK given that ScaleStrategy is not really declarative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also would like to discuss the sync logic.

Copy link
Contributor Author

@Waynegates Waynegates Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right now, this

for index := range instance.Spec.WorkerGroupSpecs {
        worker = &instance.Spec.WorkerGroupSpecs[index]

has a conflict with diff < 0 (line 310) branch code.

}

// Once we remove the feature flag and commit to those changes, the code below can be cleaned up
Expand Down
Loading