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

Jump to bottom

[Chore] Fix lint errors caused by casting int to int32 #2368

Merged
merged 4 commits into from
Sep 10, 2024
Merged

[Chore] Fix lint errors caused by casting int to int32 #2368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
409c289
update
kevin85421 Sep 10, 2024
97c1f87
update
kevin85421 Sep 10, 2024
9228afe
update
kevin85421 Sep 10, 2024
e70b277
update
kevin85421 Sep 10, 2024
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
12 changes: 6 additions & 6 deletions ray-operator/controllers/ray/common/pod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,14 +264,14 @@ func initLivenessAndReadinessProbe(rayContainer *corev1.Container, rayNodeType r
}

if rayContainer.LivenessProbe == nil {
probeTimeout := utils.DefaultLivenessProbeTimeoutSeconds
probeTimeout := int32(utils.DefaultLivenessProbeTimeoutSeconds)
if rayNodeType == rayv1.HeadNode {
probeTimeout = utils.DefaultHeadLivenessProbeTimeoutSeconds
probeTimeout = int32(utils.DefaultHeadLivenessProbeTimeoutSeconds)
}

rayContainer.LivenessProbe = &corev1.Probe{
InitialDelaySeconds: utils.DefaultLivenessProbeInitialDelaySeconds,
TimeoutSeconds: int32(probeTimeout),
Copy link
Member Author

Choose a reason for hiding this comment

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

The linter is not smart enough to determine whether casting probeTimeout (int) to int32 will cause an overflow, but it can determine if an overflow will occur when casting utils.DefaultLivenessProbeTimeoutSeconds or utils.DefaultHeadLivenessProbeTimeoutSeconds.

Copy link
Member

@MortalHappiness MortalHappiness Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is not the fault of the linter. Because utils.DefaultLivenessProbeTimeoutSeconds is a constant so the linter can easily find it will not cause error if casted to int32. But for the int32(probeTimeout) case, the linter cannot determine if it will cause an overflow because it does not know whether probeTimeout is assigned with some other values in between.

Copy link
Member Author

@kevin85421 kevin85421 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You can check securego/gosec#1212. The gosec community seems to consider it as a false positive because probeTimeout should be either utils.DefaultLivenessProbeTimeoutSeconds or utils.DefaultHeadLivenessProbeTimeoutSeconds which is smaller than MaxInt32 and should not cause overflow when casting.

TimeoutSeconds: probeTimeout,
PeriodSeconds: utils.DefaultLivenessProbePeriodSeconds,
SuccessThreshold: utils.DefaultLivenessProbeSuccessThreshold,
FailureThreshold: utils.DefaultLivenessProbeFailureThreshold,
Expand All @@ -280,13 +280,13 @@ func initLivenessAndReadinessProbe(rayContainer *corev1.Container, rayNodeType r
}

if rayContainer.ReadinessProbe == nil {
probeTimeout := utils.DefaultReadinessProbeTimeoutSeconds
probeTimeout := int32(utils.DefaultReadinessProbeTimeoutSeconds)
if rayNodeType == rayv1.HeadNode {
probeTimeout = utils.DefaultHeadReadinessProbeTimeoutSeconds
probeTimeout = int32(utils.DefaultHeadReadinessProbeTimeoutSeconds)
}
rayContainer.ReadinessProbe = &corev1.Probe{
InitialDelaySeconds: utils.DefaultReadinessProbeInitialDelaySeconds,
TimeoutSeconds: int32(probeTimeout),
TimeoutSeconds: probeTimeout,
PeriodSeconds: utils.DefaultReadinessProbePeriodSeconds,
SuccessThreshold: utils.DefaultReadinessProbeSuccessThreshold,
FailureThreshold: utils.DefaultReadinessProbeFailureThreshold,
Expand Down
14 changes: 4 additions & 10 deletions ray-operator/controllers/ray/raycluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
errstd "errors"
"fmt"
"math"
"os"
"reflect"
"runtime"
Expand Down Expand Up @@ -769,21 +768,16 @@ func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv
if worker.NumOfHosts <= 0 {
worker.NumOfHosts = 1
}
numExpectedPods := workerReplicas * worker.NumOfHosts

if len(runningPods.Items) > math.MaxInt32 {
return errstd.New("len(runningPods.Items) exceeds math.MaxInt32")
}
diff := numExpectedPods - int32(len(runningPods.Items)) //nolint:gosec // Already checked in the previous line.
numExpectedPods := int(workerReplicas * worker.NumOfHosts)
Copy link
Member Author

@kevin85421 kevin85421 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Converting int32 to int will not cause overflow. Therefore, I decide to convert int32 to int instead of converting int to int32.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code is doing the opposite, it converts to int instead of int32?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, it is a typo in my comment.

diff := numExpectedPods - len(runningPods.Items)

logger.Info("reconcilePods", "workerReplicas", workerReplicas, "NumOfHosts", worker.NumOfHosts, "runningPods", len(runningPods.Items), "diff", diff)

if diff > 0 {
// pods need to be added
logger.Info("reconcilePods", "Number workers to add", diff, "Worker group", worker.GroupName)
// create all workers of this group
var i int32
for i = 0; i < diff; i++ {
for i := 0; i < diff; i++ {
logger.Info("reconcilePods", "creating worker for group", worker.GroupName, fmt.Sprintf("index %d", i), fmt.Sprintf("in total %d", diff))
if err := r.createWorkerPod(ctx, *instance, *worker.DeepCopy()); err != nil {
return errstd.Join(utils.ErrFailedCreateWorkerPod, err)
Expand Down Expand Up @@ -814,7 +808,7 @@ func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv
// diff < 0 means that we need to delete some Pods to meet the desired number of replicas.
randomlyRemovedWorkers := -diff
logger.Info("reconcilePods", "Number workers to delete randomly", randomlyRemovedWorkers, "Worker group", worker.GroupName)
for i := 0; i < int(randomlyRemovedWorkers); i++ {
for i := 0; i < randomlyRemovedWorkers; i++ {
randomPodToDelete := runningPods.Items[i]
logger.Info("Randomly deleting Pod", "progress", fmt.Sprintf("%d / %d", i+1, randomlyRemovedWorkers), "with name", randomPodToDelete.Name)
if err := r.Delete(ctx, &randomPodToDelete); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion ray-operator/controllers/ray/rayservice_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (r *RayServiceReconciler) calculateStatus(ctx context.Context, rayServiceIn
if numServeEndpoints > math.MaxInt32 {
return errstd.New("numServeEndpoints exceeds math.MaxInt32")
}
rayServiceInstance.Status.NumServeEndpoints = int32(numServeEndpoints) //nolint:gosec // Already checked in the previous line.
rayServiceInstance.Status.NumServeEndpoints = int32(numServeEndpoints) //nolint:gosec // This is a false positive from gosec. See https://github.com/securego/gosec/issues/1212 for more details.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not a false positive. The type of numServeEndPonits is int, so it may be 32 or 64 bits depending on the system. If it is 64 bits, then casting it to int32 will cause overflow. So this comment should not be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

numServeEndpoints should be between 0 and MaxInt32, so converting it to int32 should not cause an overflow. Based on securego/gosec#1212, the gosec community considers it as a false positive.

return nil
}

Expand Down
Loading