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

Add two feature gates : Image based deployment and additional runtimeClasses #394

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions controllers/openshift_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,13 @@ func (r *KataConfigOpenShiftReconciler) processKataConfigDeleteRequest() (ctrl.R
return ctrl.Result{Requeue: true}, nil
}

// Revert the feature gate status configmap to default
err = r.FeatureGates.RevertFeatureGateStatusConfigMapToDefault(context.TODO())
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it would be cleaner, and simpler too, to just remove the cm here. As I said elsewhere, I'm far from convinced that we should insist on this configmap's existence in the first place, but it definitely serves no purpose when there's even no KataConfig instance, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm also not sure why do we need to update the user's CM? This might look strange from the user's PoV having some settings changed. We should not mix default values and user's values. Whoever create it shouldn't be the one responsible for deleting?

Not a k8s expert, and I was not following all the discussions here closely, so I'm probably missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a very good point, we should avoid muddying the notion of resource ownership to avoid problems like we had a while ago with KataConfig.spec.kataMonitorImage which was basically an OSC-owned field within a user-owner KataConfig instance.

However, my takeaway from the latest round of discussions is that we're now leaning towards the controller not requiring the existence of any of the feature-gating configmaps. This solves the ownership issue quite neatly by putting the configmap resources wholly under user control.

if err != nil {
r.Log.Info("Error reverting feature gate status configmap to default", "err", err)
return ctrl.Result{Requeue: true}, err
}

r.Log.Info("Uninstallation completed. Proceeding with the KataConfig deletion")
if err = r.removeFinalizer(); err != nil {
return ctrl.Result{Requeue: true}, nil
Expand Down
27 changes: 27 additions & 0 deletions internal/featuregates/featuregates.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package featuregates

import (
"context"
"fmt"

"github.com/go-logr/logr"
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -104,3 +106,28 @@ func (fg *FeatureGates) IsFeatureConfigMapPresent(ctx context.Context, feature s
}
return true
}

// Method to revert the feature gate status configmap to default values
func (fg *FeatureGates) RevertFeatureGateStatusConfigMapToDefault(ctx context.Context) error {
data := make(map[string]string)
for key, value := range DefaultFeatureGatesStatus {
data[key] = fmt.Sprintf("%t", value)
}

configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: fg.ConfigMapName,
Namespace: fg.Namespace,
},

Data: data,
}

err := fg.Client.Update(ctx, configMap)
if err != nil {
fgLogger.Info("Error reverting feature gate status configmap", "err", err)
return err
}
fgLogger.Info("Feature gate status configmap reverted to default values")
return nil
}