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

🌱 PartitionSet e2e #2642

Merged
merged 4 commits into from
Mar 1, 2023
Merged

🌱 PartitionSet e2e #2642

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
2a68f30
Fix issue when GenerateName size limit is reached
fgiloux Feb 8, 2023
ea2a0ac
Fix issue when duplicate dimensions are provided
fgiloux Feb 8, 2023
46c00df
Prevent APIBinding test to fail when non schedulable shards are added…
fgiloux Feb 23, 2023
d6bdad4
Add end-to-end tests for PartitionSets
fgiloux Jan 18, 2023
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
10 changes: 10 additions & 0 deletions pkg/reconciler/topology/partitionset/partitioning_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/stretchr/testify/require"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation"

corev1alpha1 "github.com/kcp-dev/kcp/pkg/apis/core/v1alpha1"
)
Expand Down Expand Up @@ -134,3 +135,12 @@ func TestPartition(t *testing.T) {
require.Equal(t, "prod", v["environment"], "Expected that all partitions have a label selector for environment = prod")
}
}

func TestGeneratePartitionName(t *testing.T) {
name := generatePartitionName(
"partitionset",
map[string]string{"region": "europe", "cloud": "EKS", "verylong": "123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890"},
[]string{"region", "verylong"},
)
require.Equal(t, "partitionset-europe-123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890"[:validation.DNS1123SubdomainMaxLength-1], name)
}
9 changes: 6 additions & 3 deletions pkg/reconciler/topology/partitionset/partitionset_reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/klog/v2"
"k8s.io/kube-openapi/pkg/util/sets"

corev1alpha1 "github.com/kcp-dev/kcp/pkg/apis/core/v1alpha1"
conditionsv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/third_party/conditions/apis/conditions/v1alpha1"
Expand Down Expand Up @@ -89,10 +90,12 @@ func (c *controller) reconcile(ctx context.Context, partitionSet *topologyv1alph
}

var matchLabelsMap map[string]map[string]string
// remove duplicates
dimensions := sets.NewString(partitionSet.Spec.Dimensions...).List()
if partitionSet.Spec.ShardSelector != nil {
matchLabelsMap = partition(shards, partitionSet.Spec.Dimensions, partitionSet.Spec.ShardSelector.MatchLabels)
matchLabelsMap = partition(shards, dimensions, partitionSet.Spec.ShardSelector.MatchLabels)
} else {
matchLabelsMap = partition(shards, partitionSet.Spec.Dimensions, nil)
matchLabelsMap = partition(shards, dimensions, nil)
}
partitionSet.Status.Count = uint16(len(matchLabelsMap))
existingMatches := map[string]struct{}{}
Expand Down Expand Up @@ -162,7 +165,7 @@ func (c *controller) reconcile(ctx context.Context, partitionSet *topologyv1alph
// Create partitions when no existing partition for the set has the same selector.
for key, matchLabels := range matchLabelsMap {
if _, ok := existingMatches[key]; !ok {
partition := generatePartition(partitionSet.Name, newMatchExpressions, matchLabels, partitionSet.Spec.Dimensions)
partition := generatePartition(partitionSet.Name, newMatchExpressions, matchLabels, dimensions)
partition.OwnerReferences = []metav1.OwnerReference{
*metav1.NewControllerRef(partitionSet, topologyv1alpha1.SchemeGroupVersion.WithKind("PartitionSet")),
}
Expand Down
31 changes: 22 additions & 9 deletions pkg/reconciler/topology/partitionset/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,18 @@ import (
"strings"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation"

topologyv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/topology/v1alpha1"
)

// generatePartition generates the Partition specifications based on
// the provided matchExpressions and matchLabels.
func generatePartition(name string, matchExpressions []metav1.LabelSelectorRequirement, matchLabels map[string]string, dimensions []string) *topologyv1alpha1.Partition {
pname := name
labels := make([]string, len(dimensions))
copy(labels, dimensions)
sort.Strings(labels)
for _, label := range labels {
pname = pname + "-" + strings.ToLower(matchLabels[label])
}

name = generatePartitionName(name, matchLabels, dimensions)
return &topologyv1alpha1.Partition{
ObjectMeta: metav1.ObjectMeta{
GenerateName: pname + "-",
GenerateName: name + "-",
},
Spec: topologyv1alpha1.PartitionSpec{
Selector: &metav1.LabelSelector{
Expand All @@ -48,3 +42,22 @@ func generatePartition(name string, matchExpressions []metav1.LabelSelectorRequi
},
}
}

// generatePartitionName creates a name based on the dimension values.
func generatePartitionName(name string, matchLabels map[string]string, dimensions []string) string {
labels := make([]string, len(dimensions))
copy(labels, dimensions)
sort.Strings(labels)
for _, label := range labels {
name = name + "-" + strings.ToLower(matchLabels[label])
}
name = name[:min(validation.DNS1123SubdomainMaxLength-1, len(name))]
return name
}

func min(a, b int) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we don't need to accumulate this sort of logic. Just inline it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inlining is done by the compiler. I find this way more readable and it seems that it is not an uncommon practice.

if a < b {
return a
}
return b
}
22 changes: 16 additions & 6 deletions test/e2e/apibinding/apibinding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
"github.com/kcp-dev/kcp/config/helpers"
apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
"github.com/kcp-dev/kcp/pkg/apis/core"
corev1alpha1 "github.com/kcp-dev/kcp/pkg/apis/core/v1alpha1"
"github.com/kcp-dev/kcp/pkg/apis/third_party/conditions/util/conditions"
kcpclientset "github.com/kcp-dev/kcp/pkg/client/clientset/versioned/cluster"
"github.com/kcp-dev/kcp/test/e2e/fixtures/wildwest/apis/wildwest"
Expand Down Expand Up @@ -167,13 +168,22 @@ func TestAPIBinding(t *testing.T) {
t.Logf("Getting a list of VirtualWorkspaceURLs assigned to Shards")
shards, err := kcpClusterClient.Cluster(core.RootCluster.Path()).CoreV1alpha1().Shards().List(ctx, metav1.ListOptions{})
require.NoError(t, err)
// Filtering out shards that are not schedulable
var shardItems []corev1alpha1.Shard
for _, s := range shards.Items {
if _, ok := s.Annotations["experimental.core.kcp.io/unschedulable"]; !ok {
shardItems = append(shardItems, s)
}
}
require.Eventually(t, func() bool {
for _, s := range shards.Items {
if len(s.Spec.VirtualWorkspaceURL) == 0 {
t.Logf("%q shard hasn't had assigned a virtual workspace URL", s.Name)
return false
for _, s := range shardItems {
if _, ok := s.Annotations["experimental.core.kcp.io/unschedulable"]; !ok {
if len(s.Spec.VirtualWorkspaceURL) == 0 {
t.Logf("%q shard hasn't had assigned a virtual workspace URL", s.Name)
return false
}
shardVirtualWorkspaceURLs.Insert(s.Spec.VirtualWorkspaceURL)
}
shardVirtualWorkspaceURLs.Insert(s.Spec.VirtualWorkspaceURL)
}
return true
}, wait.ForeverTestTimeout, 100*time.Millisecond, "expected all Shards to have a VirtualWorkspaceURL assigned")
Expand Down Expand Up @@ -356,7 +366,7 @@ func TestAPIBinding(t *testing.T) {
gvrWithIdentity := wildwestv1alpha1.SchemeGroupVersion.WithResource("cowboys:" + identity)

var names []string
for _, shard := range shards.Items {
for _, shard := range shardItems {
t.Logf("Doing a wildcard identity list for %v against %s workspace on shard %s", gvrWithIdentity, consumerWorkspace, shard.Name)
shardDynamicClusterClients, err := kcpdynamic.NewForConfig(server.ShardSystemMasterBaseConfig(t, shard.Name))
require.NoError(t, err)
Expand Down
Loading