Skip to content

Commit

Permalink
fix: duplicate pools (#167)
Browse files Browse the repository at this point in the history 
Fixes a bug, where two pools crs could reference two differently named
image crs with the same image tag specified. Both pools would get the
same ID, as garm would only create one pool internally which caused an
unexpected behaviour of the whole pool reconciliation logic / spinning
of runners.

NOTE:
This should be absolote for garm v0.1.5 as garm allows multiple pools
with same image, github-scope, flavor and provider
  • Loading branch information
@rafalgalaw
rafalgalaw authored Sep 24, 2024
1 parent 208a6d0 commit a59562d
Show file tree
Hide file tree
Showing 29 changed files with 504 additions and 167 deletions.
2 changes: 1 addition & 1 deletion Tiltfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
load('ext://cert_manager', 'deploy_cert_manager')

# garm-operator is the name of the kind-cluster and therefore usable as k8s context
allow_k8s_contexts('garm-operator')
allow_k8s_contexts('kind-garm-operator')

# we use the `cert_manager` extension to deploy cert-manager into the kind-cluster
# as the plugin has already well written readiness checks we can use it to wait for
Expand Down
2 changes: 1 addition & 1 deletion api/v1alpha1/enterprise_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ package v1alpha1
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/mercedes-benz/garm-operator/pkg/util/conditions"
"github.com/mercedes-benz/garm-operator/pkg/conditions"
)

// EnterpriseSpec defines the desired state of Enterprise
Expand Down
8 changes: 8 additions & 0 deletions api/v1alpha1/image_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ package v1alpha1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/mercedes-benz/garm-operator/pkg/filter"
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
Expand Down Expand Up @@ -50,6 +52,12 @@ type ImageList struct {
Items []Image `json:"items"`
}

func MatchesTag(tag string) filter.Predicate[Image] {
return func(i Image) bool {
return i.Spec.Tag == tag
}
}

func init() {
SchemeBuilder.Register(&Image{}, &ImageList{})
}
2 changes: 1 addition & 1 deletion api/v1alpha1/organization_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ package v1alpha1
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/mercedes-benz/garm-operator/pkg/util/conditions"
"github.com/mercedes-benz/garm-operator/pkg/conditions"
)

// OrganizationSpec defines the desired state of Organization
Expand Down
86 changes: 86 additions & 0 deletions api/v1alpha1/pool_helper.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// SPDX-License-Identifier: MIT

package v1alpha1

import (
"context"
"fmt"

"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/mercedes-benz/garm-operator/pkg/filter"
)

func (p *Pool) CheckDuplicate(ctx context.Context, client client.Client, poolImage *Image) (bool, string, error) {
poolList := &PoolList{}
err := client.List(ctx, poolList)
if err != nil {
return false, "", err
}

// only get other pools
filteredPoolList := filter.Match(poolList.Items,
MatchesFlavor(p.Spec.Flavor),
MatchesProvider(p.Spec.ProviderName),
MatchesGitHubScope(p.Spec.GitHubScopeRef.Name, p.Spec.GitHubScopeRef.Kind),
NotMatchingName(p.Name),
)

for _, p := range filteredPoolList {
image, err := p.GetImageCR(ctx, client)
if err != nil {
continue
}
if image.Spec.Tag == poolImage.Spec.Tag {
return true, fmt.Sprintf("%s/%s", p.Namespace, p.Name), nil
}
}
return false, "", nil
}

func (p *Pool) GetImageCR(ctx context.Context, client client.Client) (*Image, error) {
image := &Image{}
if p.Spec.ImageName != "" {
if err := client.Get(ctx, types.NamespacedName{Name: p.Spec.ImageName, Namespace: p.Namespace}, image); err != nil {
return nil, err
}
}
return image, nil
}

func MatchesImage(image string) filter.Predicate[Pool] {
return func(p Pool) bool {
return p.Spec.ImageName == image
}
}

func MatchesFlavor(flavor string) filter.Predicate[Pool] {
return func(p Pool) bool {
return p.Spec.Flavor == flavor
}
}

func MatchesProvider(provider string) filter.Predicate[Pool] {
return func(p Pool) bool {
return p.Spec.ProviderName == provider
}
}

func MatchesGitHubScope(name, kind string) filter.Predicate[Pool] {
return func(p Pool) bool {
return p.Spec.GitHubScopeRef.Name == name && p.Spec.GitHubScopeRef.Kind == kind
}
}

func MatchesID(id string) filter.Predicate[Pool] {
return func(p Pool) bool {
return p.Status.ID == id
}
}

func NotMatchingName(name string) filter.Predicate[Pool] {
return func(p Pool) bool {
return p.Name != name
}
}
52 changes: 0 additions & 52 deletions api/v1alpha1/pool_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,55 +98,3 @@ type PoolList struct {
func init() {
SchemeBuilder.Register(&Pool{}, &PoolList{})
}

// +k8s:deepcopy-gen=false
type Predicate func(p Pool) bool

func MatchesImage(image string) Predicate {
return func(p Pool) bool {
return p.Spec.ImageName == image
}
}

func MatchesFlavor(flavor string) Predicate {
return func(p Pool) bool {
return p.Spec.Flavor == flavor
}
}

func MatchesProvider(provider string) Predicate {
return func(p Pool) bool {
return p.Spec.ProviderName == provider
}
}

func MatchesGitHubScope(name, kind string) Predicate {
return func(p Pool) bool {
return p.Spec.GitHubScopeRef.Name == name && p.Spec.GitHubScopeRef.Kind == kind
}
}

func MatchesID(id string) Predicate {
return func(p Pool) bool {
return p.Status.ID == id
}
}

func (p *PoolList) FilterByFields(predicates ...Predicate) {
var filteredItems []Pool

for _, pool := range p.Items {
match := true
for _, predicate := range predicates {
if !predicate(pool) {
match = false
break
}
}
if match {
filteredItems = append(filteredItems, pool)
}
}

p.Items = filteredItems
}
113 changes: 107 additions & 6 deletions api/v1alpha1/pool_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"

"github.com/mercedes-benz/garm-operator/pkg/filter"
)

func TestPoolList_FilterByFields(t *testing.T) {
Expand All @@ -17,7 +19,7 @@ func TestPoolList_FilterByFields(t *testing.T) {
Items []Pool
}
type args struct {
predicates []Predicate
predicates []filter.Predicate[Pool]
}
tests := []struct {
name string
Expand Down Expand Up @@ -68,7 +70,7 @@ func TestPoolList_FilterByFields(t *testing.T) {
},
},
args: args{
predicates: []Predicate{
predicates: []filter.Predicate[Pool]{
MatchesImage("ubuntu-2204"),
MatchesFlavor("large"),
MatchesProvider("openstack"),
Expand Down Expand Up @@ -120,7 +122,7 @@ func TestPoolList_FilterByFields(t *testing.T) {
},
},
args: args{
predicates: []Predicate{
predicates: []filter.Predicate[Pool]{
MatchesImage("ubuntu-2404"),
MatchesFlavor("large"),
MatchesProvider("openstack"),
Expand All @@ -129,6 +131,105 @@ func TestPoolList_FilterByFields(t *testing.T) {
},
length: 0,
},
{
name: "no predicates provided",
fields: fields{
TypeMeta: metav1.TypeMeta{},
ListMeta: metav1.ListMeta{},
Items: []Pool{
{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Name: "ubuntu-2004-large",
Namespace: "test",
},
Spec: PoolSpec{
ImageName: "ubuntu-2004",
Flavor: "large",
ProviderName: "openstack",
GitHubScopeRef: corev1.TypedLocalObjectReference{
Name: "test",
Kind: "Enterprise",
APIGroup: ptr.To[string]("github.com"),
},
},
},
{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Name: "ubuntu-2204-large",
Namespace: "test",
},
Spec: PoolSpec{
ImageName: "ubuntu-2204",
Flavor: "large",
ProviderName: "openstack",
GitHubScopeRef: corev1.TypedLocalObjectReference{
Name: "test",
Kind: "Enterprise",
APIGroup: ptr.To[string]("github.com"),
},
},
},
},
},
args: args{
predicates: []filter.Predicate[Pool]{},
},
length: 2,
},
{
name: "duplicate pool but different name",
fields: fields{
TypeMeta: metav1.TypeMeta{},
ListMeta: metav1.ListMeta{},
Items: []Pool{
{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Name: "ubuntu-2004-large",
Namespace: "test",
},
Spec: PoolSpec{
ImageName: "ubuntu-2004",
Flavor: "large",
ProviderName: "openstack",
GitHubScopeRef: corev1.TypedLocalObjectReference{
Name: "test",
Kind: "Enterprise",
APIGroup: ptr.To[string]("github.com"),
},
},
},
{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Name: "ubuntu-2204-large",
Namespace: "test",
},
Spec: PoolSpec{
ImageName: "ubuntu-2204",
Flavor: "large",
ProviderName: "openstack",
GitHubScopeRef: corev1.TypedLocalObjectReference{
Name: "test",
Kind: "Enterprise",
APIGroup: ptr.To[string]("github.com"),
},
},
},
},
},
args: args{
predicates: []filter.Predicate[Pool]{
MatchesFlavor("large"),
MatchesProvider("openstack"),
MatchesGitHubScope("test", "Enterprise"),
NotMatchingName("ubuntu-2004-large"),
},
},
length: 1,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -138,10 +239,10 @@ func TestPoolList_FilterByFields(t *testing.T) {
Items: tt.fields.Items,
}

p.FilterByFields(tt.args.predicates...)
filteredPools := filter.Match(p.Items, tt.args.predicates...)

if len(p.Items) != tt.length {
t.Errorf("FilterByFields() = %v, want %v", len(p.Items), tt.length)
if len(filteredPools) != tt.length {
t.Errorf("FilterByFields() = %v, want %v", len(filteredPools), tt.length)
}
})
}
Expand Down
Loading

0 comments on commit a59562d

Please sign in to comment.