Skip to content

Commit

Permalink
Change CRD map entries to named object lists (#154)
Browse files Browse the repository at this point in the history
Co-authored-by: Paolo Ambrosio <paolo.ambrosio2@sky.uk>
Co-authored-by: EdwardCuiPeacock <Edward.Cui@nbcuni.com>
Co-authored-by: Alex Georgousis <alex.georgousis@outlook.com>
  • Loading branch information
4 people authored Aug 30, 2022
1 parent a5d98cf commit cfa724c
Show file tree
Hide file tree
Showing 132 changed files with 3,266 additions and 1,034 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ include go-get.mk
# Image URL to use all building/pushing image targets
IMG ?= kfp-operator-controller
# Produce CRDs that work back to Kubernetes 1.11 (no version conversion)
CRD_OPTIONS ?= "crd:trivialVersions=true,preserveUnknownFields=false"
CRD_OPTIONS ?= "crd:preserveUnknownFields=false"

# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
ifeq (,$(shell go env GOBIN))
Expand Down
25 changes: 17 additions & 8 deletions PROJECT
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,41 @@ resources:
domain: kubeflow.org
group: pipelines
kind: Pipeline
path: github.com/sky-uk/kfp-operator/apis/pipelines/v1
version: v1
path: github.com/sky-uk/kfp-operator/apis/pipelines/v1alpha3
version: v1alpha3
webhooks:
conversion: true
webhookVersion: v1
- api:
crdVersion: v1
namespaced: true
domain: kubeflow.org
group: config
kind: KfpControllerConfig
path: github.com/sky-uk/kfp-operator/apis/config/v1
version: v1
path: github.com/sky-uk/kfp-operator/apis/config/v1alpha3
version: v1alpha3
- api:
crdVersion: v1
namespaced: true
controller: true
domain: kubeflow.org
group: pipelines
kind: RunConfiguration
path: github.com/sky-uk/kfp-operator/apis/pipelines/v1
version: v1
path: github.com/sky-uk/kfp-operator/apis/pipelines/v1alpha3
version: v1alpha3
webhooks:
conversion: true
webhookVersion: v1
- api:
crdVersion: v1
namespaced: true
controller: true
domain: kubeflow.org
group: pipelines
kind: Experiment
path: github.com/sky-uk/kfp-operator/apis/pipelines/v1
version: v1
path: github.com/sky-uk/kfp-operator/apis/pipelines/v1alpha3
version: v1alpha3
webhooks:
conversion: true
webhookVersion: v1
version: "3"
13 changes: 11 additions & 2 deletions apis/pipelines/v1alpha2/common_types.go → apis/common_types.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package v1alpha2
package apis

import (
"context"
Expand All @@ -17,6 +17,15 @@ const (
Failed SynchronizationState = "Failed"
)

const Group = "pipelines.kubeflow.org"

//+kubebuilder:object:generate=true
type NamedValue struct {
Name string `json:"name,omitempty"`
Value string `json:"value,omitempty"`
}

//+kubebuilder:object:generate=true
type Status struct {
KfpId string `json:"kfpId,omitempty"`
SynchronizationState SynchronizationState `json:"synchronizationState,omitempty"`
Expand All @@ -27,7 +36,7 @@ type Status struct {
var Annotations = struct {
Debug string
}{
Debug: GroupVersion.Group + "/debug",
Debug: Group + "/debug",
}

type DebugOptions struct {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Package v2 contains API Schema definitions for the config v2 API group
//+kubebuilder:object:generate=true
//+groupName=config.kubeflow.org
package v1alpha2
package v1alpha3

import (
"k8s.io/apimachinery/pkg/runtime/schema"
Expand All @@ -10,7 +10,7 @@ import (

var (
// GroupVersion is group version used to register these objects
GroupVersion = schema.GroupVersion{Group: "config.kubeflow.org", Version: "v1alpha2"}
GroupVersion = schema.GroupVersion{Group: "config.kubeflow.org", Version: "v1alpha3"}

// SchemeBuilder is used to add go types to the GroupVersionKind scheme
SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package v1alpha2
package v1alpha3

import (
pipelinesv1 "github.com/sky-uk/kfp-operator/apis/pipelines/v1alpha2"
"github.com/sky-uk/kfp-operator/apis"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
cfg "sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
)
Expand All @@ -12,11 +12,13 @@ type Configuration struct {

WorkflowTemplatePrefix string `json:"workflowTemplatePrefix,omitempty"`

DefaultBeamArgs map[string]string `json:"defaultBeamArgs,omitempty"`
Multiversion bool `json:"multiversion,omitempty"`

DefaultBeamArgs []apis.NamedValue `json:"defaultBeamArgs,omitempty"`

DefaultExperiment string `json:"defaultExperiment,omitempty"`

Debug pipelinesv1.DebugOptions `json:"debug,omitempty"`
Debug apis.DebugOptions `json:"debug,omitempty"`
}

//+kubebuilder:object:root=true
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package objecthasher
package pipelines

import (
"crypto/sha1"
"github.com/sky-uk/kfp-operator/apis"
"hash"
"sort"
)
Expand All @@ -12,7 +13,7 @@ type ObjectHasher struct {
h hash.Hash
}

func New() ObjectHasher {
func NewObjectHasher() ObjectHasher {
return ObjectHasher{
sha1.New(),
}
Expand All @@ -38,6 +39,24 @@ func (oh ObjectHasher) WriteMapField(value map[string]string) {
oh.h.Write(hashFieldSeparator)
}

func (oh ObjectHasher) WriteNamedValueListField(namedValues []apis.NamedValue) {

sort.Slice(namedValues, func(i, j int) bool {
if namedValues[i].Name != namedValues[j].Name {
return namedValues[i].Name < namedValues[j].Name
} else {
return namedValues[i].Value < namedValues[j].Value
}
})

for _, k := range namedValues {
oh.WriteStringField(k.Name)
oh.WriteStringField(k.Value)
}

oh.h.Write(hashFieldSeparator)
}

func (oh ObjectHasher) Sum() []byte {
return oh.h.Sum(nil)
}
187 changes: 187 additions & 0 deletions apis/pipelines/object_hasher_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
//go:build unit
// +build unit

package pipelines

import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/sky-uk/kfp-operator/apis"
)

var _ = Context("ObjectHasher", func() {
var _ = Describe("WriteStringField", func() {
Specify("Adjacent string fields should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteStringField("ab")
oh1.WriteStringField("c")

oh2 := NewObjectHasher()
oh2.WriteStringField("a")
oh2.WriteStringField("bc")

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})
})

var _ = Describe("WriteNamedValueListField", func() {
Specify("Adjacent NamedValue list fields should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "1"},
{Name: "b", Value: "2"},
})
oh1.WriteNamedValueListField([]apis.NamedValue{
{Name: "c", Value: "3"},
})

oh2 := NewObjectHasher()
oh2.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "1"},
})
oh2.WriteNamedValueListField([]apis.NamedValue{
{Name: "b", Value: "2"},
{Name: "c", Value: "3"},
})

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})

Specify("NamedValue list key and values should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteNamedValueListField([]apis.NamedValue{
{Name: "ab", Value: "c"},
})

oh2 := NewObjectHasher()
oh2.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "bc"},
})

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})

Specify("NamedValue list fields should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "bc"},
{Name: "d", Value: "e"},
})

oh2 := NewObjectHasher()
oh2.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "b"},
{Name: "cd", Value: "e"},
})

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})

Specify("NamedValue list field hash should be consistent if the order of entries is changed", func() {
oh1 := NewObjectHasher()
oh1.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "1"},
{Name: "b", Value: "2"},
})

oh2 := NewObjectHasher()
oh2.WriteNamedValueListField([]apis.NamedValue{
{Name: "b", Value: "2"},
{Name: "a", Value: "1"},
})

Expect(oh1.Sum()).To(Equal(oh2.Sum()))
})

Specify("NamedValue list field hash should be consistent if the order of multi-entries is changed", func() {
oh1 := NewObjectHasher()
oh1.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "1"},
{Name: "a", Value: "2"},
})

oh2 := NewObjectHasher()
oh2.WriteNamedValueListField([]apis.NamedValue{
{Name: "a", Value: "2"},
{Name: "a", Value: "1"},
})

Expect(oh1.Sum()).To(Equal(oh2.Sum()))
})
})

var _ = Describe("WriteMapField", func() {

Specify("Adjacent map fields should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteMapField(map[string]string{
"a": "1",
"b": "2",
})
oh1.WriteMapField(map[string]string{
"c": "3",
})

oh2 := NewObjectHasher()
oh2.WriteMapField(map[string]string{
"a": "1",
})
oh2.WriteMapField(map[string]string{
"b": "2",
"c": "3",
})

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})

Specify("Map key and values should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteMapField(map[string]string{
"ab": "c",
})

oh2 := NewObjectHasher()
oh2.WriteMapField(map[string]string{
"a": "bc",
})

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})

Specify("Map fields should be considered separate", func() {
oh1 := NewObjectHasher()
oh1.WriteMapField(map[string]string{
"a": "bc",
"d": "e",
})

oh2 := NewObjectHasher()
oh2.WriteMapField(map[string]string{
"a": "b",
"cd": "e",
})

Expect(oh1.Sum()).NotTo(Equal(oh2.Sum()))
})

Specify("Map field hash should be consistent", func() {
// Map iterators start from a random point. The chance of
// a false positive is {map len}^-{iterations}
iterations := 10
sameMap := map[string]string{
"a": "1",
"b": "2",
}

for i := 0; i < iterations; i++ {
oh1 := NewObjectHasher()
oh1.WriteMapField(sameMap)

oh2 := NewObjectHasher()
oh2.WriteMapField(sameMap)

Expect(oh1.Sum()).To(Equal(oh2.Sum()))
}
})
})
})
Loading

0 comments on commit cfa724c

Please sign in to comment.