Skip to content

Commit

Permalink
Introduce Go std slices and maps lib
Browse files Browse the repository at this point in the history
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
  • Loading branch information
tenzen-y committed Nov 2, 2024
1 parent 8a59936 commit 4b1eddd
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 185 deletions.
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())
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

0 comments on commit 4b1eddd

Please sign in to comment.