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.

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

Introduce Go std slices and maps lib #696

Merged
merged 2 commits into from
Nov 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ require (
github.com/open-policy-agent/cert-controller v0.11.0
github.com/prometheus/client_golang v1.20.5
github.com/stretchr/testify v1.9.0
golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56
k8s.io/api v0.31.2
k8s.io/apimachinery v0.31.2
k8s.io/apiserver v0.31.2
Expand Down Expand Up @@ -66,6 +65,7 @@ require (
go.uber.org/atomic v1.11.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.26.0 // indirect
golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect
golang.org/x/mod v0.20.0 // indirect
golang.org/x/net v0.28.0 // indirect
golang.org/x/oauth2 v0.21.0 // indirect
Expand Down
18 changes: 13 additions & 5 deletions pkg/controllers/jobset_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"encoding/hex"
"errors"
"fmt"
"maps"
"slices"
"sort"
"strconv"
"sync"
Expand Down Expand Up @@ -659,8 +661,8 @@ func constructJobsFromTemplate(js *jobset.JobSet, rjob *jobset.ReplicatedJob, ow
func constructJob(js *jobset.JobSet, rjob *jobset.ReplicatedJob, jobIdx int) *batchv1.Job {
job := &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{
Labels: collections.CloneMap(rjob.Template.Labels),
Annotations: collections.CloneMap(rjob.Template.Annotations),
Labels: maps.Clone(rjob.Template.Labels),
Annotations: maps.Clone(rjob.Template.Annotations),
Name: placement.GenJobName(js.Name, rjob.Name, jobIdx),
Namespace: js.Namespace,
},
Expand Down Expand Up @@ -708,7 +710,7 @@ func shouldCreateJob(jobName string, ownedJobs *childJobs) bool {
// TODO: maybe we can use a job map here so we can do O(1) lookups
// to check if the job already exists, rather than a linear scan
// through all the jobs owned by the jobset.
for _, job := range collections.Concat(ownedJobs.active, ownedJobs.successful, ownedJobs.failed, ownedJobs.previous) {
for _, job := range slices.Concat(ownedJobs.active, ownedJobs.successful, ownedJobs.failed, ownedJobs.previous) {
if jobName == job.Name {
return false
}
Expand All @@ -731,7 +733,10 @@ func labelAndAnnotateObject(obj metav1.Object, js *jobset.JobSet, rjob *jobset.R
jobName := placement.GenJobName(js.Name, rjob.Name, jobIdx)

// Set labels on the object.
labels := collections.CloneMap(obj.GetLabels())
labels := maps.Clone(obj.GetLabels())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to clone the map here? similar question to the annotations clone below

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good question.
TBH, I have the same question since we already set the constructed labels and annotations to object the below:

obj.SetLabels(labels)
obj.SetAnnotations(annotations)

I thought this redundant clone came from historical reasons.
Anyway, let me remove these clones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.
If you want to keep these clones, let me know.

if labels == nil {
labels = make(map[string]string)
}
labels[jobset.JobSetNameKey] = js.Name
labels[jobset.ReplicatedJobNameKey] = rjob.Name
labels[constants.RestartsKey] = strconv.Itoa(int(js.Status.Restarts))
Expand All @@ -742,7 +747,10 @@ func labelAndAnnotateObject(obj metav1.Object, js *jobset.JobSet, rjob *jobset.R
labels[jobset.JobGlobalIndexKey] = globalJobIndex(js, rjob.Name, jobIdx)

// Set annotations on the object.
annotations := collections.CloneMap(obj.GetAnnotations())
annotations := maps.Clone(obj.GetAnnotations())
if annotations == nil {
annotations = make(map[string]string)
}
annotations[jobset.JobSetNameKey] = js.Name
annotations[jobset.ReplicatedJobNameKey] = rjob.Name
annotations[constants.RestartsKey] = strconv.Itoa(int(js.Status.Restarts))
Expand Down
8 changes: 4 additions & 4 deletions pkg/controllers/success_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,23 @@ limitations under the License.
package controllers

import (
"slices"

batchv1 "k8s.io/api/batch/v1"

jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2"

"sigs.k8s.io/jobset/pkg/util/collections"
)

// jobMatchesSuccessPolicy returns a boolean value indicating if the Job is part of a
// ReplicatedJob that matches the JobSet's success policy.
func jobMatchesSuccessPolicy(js *jobset.JobSet, job *batchv1.Job) bool {
return len(js.Spec.SuccessPolicy.TargetReplicatedJobs) == 0 || collections.Contains(js.Spec.SuccessPolicy.TargetReplicatedJobs, job.ObjectMeta.Labels[jobset.ReplicatedJobNameKey])
return len(js.Spec.SuccessPolicy.TargetReplicatedJobs) == 0 || slices.Contains(js.Spec.SuccessPolicy.TargetReplicatedJobs, job.ObjectMeta.Labels[jobset.ReplicatedJobNameKey])
}

// replicatedJobMatchesSuccessPolicy returns a boolean value indicating if the ReplicatedJob
// matches the JobSet's success policy.
func replicatedJobMatchesSuccessPolicy(js *jobset.JobSet, rjob *jobset.ReplicatedJob) bool {
return len(js.Spec.SuccessPolicy.TargetReplicatedJobs) == 0 || collections.Contains(js.Spec.SuccessPolicy.TargetReplicatedJobs, rjob.Name)
return len(js.Spec.SuccessPolicy.TargetReplicatedJobs) == 0 || slices.Contains(js.Spec.SuccessPolicy.TargetReplicatedJobs, rjob.Name)
}

// replicatedJobMatchesSuccessPolicy returns the number of jobs in the given slice `jobs`
Expand Down
35 changes: 1 addition & 34 deletions pkg/util/collections/collections.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,41 +12,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package collections

func Concat[T any](slices ...[]T) []T {
var result = make([]T, 0)
for _, slice := range slices {
result = append(result, slice...)
}
return result
}

func CloneMap[K, V comparable](m map[K]V) map[K]V {
copy := make(map[K]V)
for k, v := range m {
copy[k] = v
}
return copy
}

func Contains[T comparable](slice []T, element T) bool {
for _, item := range slice {
if item == element {
return true
}
}
return false
}

func IndexOf[T comparable](slice []T, item T) int {
for i, v := range slice {
if v == item {
return i
}
}
return -1
}
package collections

// MergeMaps will merge the `old` and `new` maps and return the
// merged map. If a key appears in both maps, the key-value pair
Expand Down
135 changes: 1 addition & 134 deletions pkg/util/collections/collections_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,144 +15,11 @@ limitations under the License.
package collections

import (
"fmt"
"reflect"
"slices"
"testing"

"github.com/google/go-cmp/cmp"
"golang.org/x/exp/slices"
)

func TestConcat(t *testing.T) {
type testCase struct {
name string
slices [][]int
want []int
}

testCases := []testCase{
{
name: "Two empty slices",
slices: [][]int{{}, {}},
want: []int{},
},
{
name: "One empty slice, one non-empty slice",
slices: [][]int{{1, 2, 3}, {}},
want: []int{1, 2, 3},
},
{
name: "Two non-empty slices",
slices: [][]int{{1, 2, 3}, {4, 5, 6}},
want: []int{1, 2, 3, 4, 5, 6},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
got := Concat(tc.slices...)
if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("unexpected diff (-want/+got): %s", diff)
}
})
}
}

func TestCloneMap(t *testing.T) {
type testCase struct {
name string
input map[string]string
want map[string]string
}

testCases := []testCase{
{
name: "Empty map",
input: map[string]string{},
want: map[string]string{},
},
{
name: "Non-empty map",
input: map[string]string{"foo": "bar", "baz": "qux"},
want: map[string]string{"foo": "bar", "baz": "qux"},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
got := CloneMap(tc.input)
if !reflect.DeepEqual(tc.want, got) {
t.Errorf("cloned map values do not match input map. input: %s, output: %s", fmt.Sprintf("%v", tc.input), fmt.Sprintf("%v", got))
}
// Confirm these maps have different pointers.
if &tc.want == &got {
t.Errorf("input map reference and output map reference point to the same object")
}
})
}
}

func TestContains(t *testing.T) {
type testCase struct {
name string
slice []string
element string
want bool
}

testCases := []testCase{
{
name: "Empty slice",
slice: []string{},
element: "foo",
want: false,
},
{
name: "Slice with one element, contains element",
slice: []string{"foo"},
element: "foo",
want: true,
},
{
name: "Slice with one element, does not element",
slice: []string{"foo"},
element: "bar",
want: false,
},
{
name: "Slice with two elements, contains element",
slice: []string{"foo", "bar"},
element: "bar",
want: true,
},
{
name: "Slice with two elements, does not contain element",
slice: []string{"foo", "bar"},
element: "baz",
want: false,
},
{
name: "Slice with three elements, contains element",
slice: []string{"foo", "bar", "baz"},
element: "bar",
want: true,
},
{
name: "Slice with two elements, does not contain element",
slice: []string{"foo", "bar", "baz"},
element: "missing",
want: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
if got := Contains(tc.slice, tc.element); got != tc.want {
t.Errorf("got: %t, want %t", got, tc.want)
}
})
}
}

func TestMergeMaps(t *testing.T) {
testCases := []struct {
name string
Expand Down
8 changes: 4 additions & 4 deletions pkg/webhooks/jobset_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"math"
"regexp"
"slices"
"strconv"
"strings"

Expand All @@ -35,7 +36,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"sigs.k8s.io/jobset/pkg/util/collections"
"sigs.k8s.io/jobset/pkg/util/placement"

jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2"
Expand Down Expand Up @@ -228,7 +228,7 @@ func (j *jobSetWebhook) ValidateCreate(ctx context.Context, obj runtime.Object)

// Validate the success policy's target replicated jobs are valid.
for _, rjobName := range js.Spec.SuccessPolicy.TargetReplicatedJobs {
if !collections.Contains(validReplicatedJobs, rjobName) {
if !slices.Contains(validReplicatedJobs, rjobName) {
allErrs = append(allErrs, fmt.Errorf("invalid replicatedJob name '%s' does not appear in .spec.ReplicatedJobs", rjobName))
}
}
Expand Down Expand Up @@ -320,14 +320,14 @@ func validateFailurePolicy(failurePolicy *jobset.FailurePolicy, validReplicatedJ

// Validate the rules target replicated jobs are valid
for _, rjobName := range rule.TargetReplicatedJobs {
if !collections.Contains(validReplicatedJobs, rjobName) {
if !slices.Contains(validReplicatedJobs, rjobName) {
allErrs = append(allErrs, fmt.Errorf("invalid replicatedJob name '%s' in failure policy does not appear in .spec.ReplicatedJobs", rjobName))
}
}

// Validate the rules on job failure reasons are valid
for _, failureReason := range rule.OnJobFailureReasons {
if !collections.Contains(validOnJobFailureReasons, failureReason) {
if !slices.Contains(validOnJobFailureReasons, failureReason) {
allErrs = append(allErrs, fmt.Errorf("invalid job failure reason '%s' in failure policy is not a recognized job failure reason", failureReason))
}
}
Expand Down
6 changes: 3 additions & 3 deletions test/integration/controller/jobset_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllertest
import (
"context"
"fmt"
"slices"
"sort"
"strconv"
"time"
Expand All @@ -36,7 +37,6 @@ import (
jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2"
"sigs.k8s.io/jobset/pkg/constants"
"sigs.k8s.io/jobset/pkg/controllers"
"sigs.k8s.io/jobset/pkg/util/collections"
"sigs.k8s.io/jobset/pkg/util/testing"
testutil "sigs.k8s.io/jobset/test/util"
)
Expand Down Expand Up @@ -1944,7 +1944,7 @@ func removeForegroundDeletionFinalizers(js *jobset.JobSet, expectedFinalizers in
gomega.Eventually(k8sClient.List(ctx, &jobList, client.InNamespace(js.Namespace))).Should(gomega.Succeed())

for _, job := range jobList.Items {
idx := collections.IndexOf(job.Finalizers, metav1.FinalizerDeleteDependents)
idx := slices.Index(job.Finalizers, metav1.FinalizerDeleteDependents)
if idx != -1 {
job.Finalizers = append(job.Finalizers[:idx], job.Finalizers[idx+1:]...)
if err := k8sClient.Update(ctx, &job); err != nil {
Expand Down Expand Up @@ -2126,7 +2126,7 @@ func checkPodTemplateUpdates(js *jobset.JobSet, podTemplateUpdates *updatePodTem

// Check tolerations were updated.
for _, toleration := range podTemplateUpdates.tolerations {
if !collections.Contains(job.Spec.Template.Spec.Tolerations, toleration) {
if !slices.Contains(job.Spec.Template.Spec.Tolerations, toleration) {
return false, fmt.Errorf("missing toleration %v", toleration)
}
}
Expand Down