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

controllers: replicate odf tolerations to child subscriptions #368

Merged
merged 1 commit into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
68 changes: 64 additions & 4 deletions controllers/subscriptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,39 +42,99 @@ import (
// because fake.Client does not support them.
func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operatorv1alpha1.Subscription) (*operatorv1alpha1.Subscription, error) {

odfSub, err := GetOdfSubscription(cli)
if err != nil {
return nil, err
}

if odfSub.Spec.Config == nil {
odfSub.Spec.Config = &operatorv1alpha1.SubscriptionConfig{}
}
Comment on lines +50 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed. Do not update your own subscription.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not gonna update. It is just to save from the panic in the below code.


subsList := &operatorv1alpha1.SubscriptionList{}
err := cli.List(context.TODO(), subsList, &client.ListOptions{Namespace: desiredSubscription.Namespace})
err = cli.List(context.TODO(), subsList, &client.ListOptions{Namespace: desiredSubscription.Namespace})
if err != nil {
return nil, err
}

var subExsist bool
var actualSub *operatorv1alpha1.Subscription
pkgName := desiredSubscription.Spec.Package
for i, sub := range subsList.Items {
if sub.Spec.Package == pkgName {
subExsist = true
if actualSub != nil {
foundSubs := []string{actualSub.Name, sub.Name}
return nil, fmt.Errorf("multiple Subscriptions found for package '%s': %v", pkgName, foundSubs)
}
actualSub = &subsList.Items[i]
actualSub.Spec.Channel = desiredSubscription.Spec.Channel

// If the config is not set, only then set it to the desired value => allow user to override
if actualSub.Spec.Config == nil {
if actualSub.Spec.Config == nil && desiredSubscription.Spec.Config == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just update

func GetStorageClusterSubscriptions() []*operatorv1alpha1.Subscription {
to inject tolerations instead of all these checks?

Add tolerations to desired subscription, and during reconcile merge actual and desired subs. Wouldn't that be simpler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that will require extensive changes in the code, we will have to change the signature of multiple funcs.

actualSub.Spec.Config = &operatorv1alpha1.SubscriptionConfig{}
actualSub.Spec.Config.Tolerations = odfSub.Spec.Config.Tolerations
} else if actualSub.Spec.Config == nil && desiredSubscription.Spec.Config != nil {
actualSub.Spec.Config = desiredSubscription.Spec.Config
actualSub.Spec.Config.Tolerations = getMergedTolerations(odfSub.Spec.Config.Tolerations, desiredSubscription.Spec.Config.Tolerations)
} else if actualSub.Spec.Config != nil && desiredSubscription.Spec.Config == nil {
actualSub.Spec.Config.Tolerations = odfSub.Spec.Config.Tolerations
} else if actualSub.Spec.Config != nil && desiredSubscription.Spec.Config != nil {
// Combines the environment variables from both subscriptions.
// If actualSub already contains an environment variable, its value will be updated with the value from desiredSubscription.
actualSub.Spec.Config.Env = getMergedEnvVars(actualSub.Spec.Config.Env, desiredSubscription.Spec.Config.Env)
// Combines the Tolerations from odf sub and desired sub.
actualSub.Spec.Config.Tolerations = getMergedTolerations(odfSub.Spec.Config.Tolerations, desiredSubscription.Spec.Config.Tolerations)
}

desiredSubscription = actualSub
}
}

if !subExsist {
if desiredSubscription.Spec.Config == nil {
desiredSubscription.Spec.Config = &operatorv1alpha1.SubscriptionConfig{
Tolerations: odfSub.Spec.Config.Tolerations,
}
} else {
desiredSubscription.Spec.Config.Tolerations = getMergedTolerations(odfSub.Spec.Config.Tolerations, desiredSubscription.Spec.Config.Tolerations)
}
}

return desiredSubscription, nil
}

func getMergedTolerations(tol1, tol2 []corev1.Toleration) []corev1.Toleration {

if len(tol1) == 0 {
return append([]corev1.Toleration{}, tol2...)
} else if len(tol2) == 0 {
return append([]corev1.Toleration{}, tol1...)
}

mergedTolerations := append([]corev1.Toleration{}, tol1...)

for _, t2 := range tol2 {
found := false
for i, t1 := range tol1 {
if t1.Key == t2.Key && t1.Operator == t2.Operator && t1.Value == t2.Value {
found = true
break
}
// If the toleration with the same key but different values is found,
// update the existing toleration in tol1 with the new toleration from tol2.
if t1.Key == t2.Key && t1.Operator == t2.Operator {
tol1[i] = t2
found = true
break
}
}
if !found {
mergedTolerations = append(mergedTolerations, t2)
}
}

return mergedTolerations
}

// getMergedEnvVars updates the value of env variables in the envList1 with that of envList2 and
// returns the updated list of env variables.
func getMergedEnvVars(envList1, envList2 []corev1.EnvVar) []corev1.EnvVar {
Expand Down
20 changes: 20 additions & 0 deletions controllers/subscriptions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

Expand Down Expand Up @@ -72,6 +73,16 @@ func TestEnsureSubscription(t *testing.T) {

Spec: &operatorv1alpha1.SubscriptionSpec{
Package: OdfSubscriptionPackage,
Config: &operatorv1alpha1.SubscriptionConfig{
Tolerations: []corev1.Toleration{
{
Key: "node.odf.openshift.io/storage",
Operator: "Equal",
Value: "true",
Effect: "NoSchedule",
},
},
},
},
}
err = fakeReconciler.Client.Create(context.TODO(), odfSub)
Expand All @@ -81,9 +92,18 @@ func TestEnsureSubscription(t *testing.T) {
assert.NoError(t, err)

for _, expectedSubscription := range subs {
if expectedSubscription.Spec.Config == nil {
expectedSubscription.Spec.Config = &operatorv1alpha1.SubscriptionConfig{
Tolerations: odfSub.Spec.Config.Tolerations,
}
} else {
expectedSubscription.Spec.Config.Tolerations = getMergedTolerations(odfSub.Spec.Config.Tolerations, expectedSubscription.Spec.Config.Tolerations)
}

actualSubscription := &operatorv1alpha1.Subscription{}
err = fakeReconciler.Client.Get(context.TODO(), types.NamespacedName{Name: expectedSubscription.Name, Namespace: expectedSubscription.Namespace}, actualSubscription)
assert.NoError(t, err)

assert.Equal(t, expectedSubscription.Spec, actualSubscription.Spec)
}
}
Expand Down
Loading