Skip to content

Commit

Permalink
WIP: raise an alert when the descheduler is not correctly configured
Browse files Browse the repository at this point in the history 
Descheduler is an optional operator and it's
not installed by default nor as a dependency of HCO.
When installed it can work on a cluster with
KubeVirt only if configured enabling
devEnableEvictionsInBackground profileCustomization
that is disabled by default.
HCO will check if the descheduler is there,
and if so it will check its configuration.
If the descheduler is misconfigured for
the KubeVirt use case, HCO will trigger
an alert making the cluster admin aware.
HCO is not directly amending the descheduler
configuration since it's an external independent
operator and directly controlling it is not a safe
practice (it could bring to infinite loops fighting
with other operators and so on).

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
  • Loading branch information
@tiraboschi
tiraboschi committed Sep 18, 2024
1 parent 077d5d1 commit cc2820d
Show file tree
Hide file tree
Showing 25 changed files with 1,506 additions and 14 deletions.
17 changes: 13 additions & 4 deletions cmd/hyperconverged-cluster-operator/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
imagev1 "github.com/openshift/api/image/v1"
operatorv1 "github.com/openshift/api/operator/v1"
openshiftroutev1 "github.com/openshift/api/route/v1"
deschedulerv1 "github.com/openshift/cluster-kube-descheduler-operator/pkg/apis/descheduler/v1"
csvv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
operatorsapiv2 "github.com/operator-framework/api/pkg/operators/v2"
monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
Expand Down Expand Up @@ -86,6 +87,7 @@ var (
operatorsapiv2.AddToScheme,
imagev1.Install,
aaqv1alpha1.AddToScheme,
deschedulerv1.AddToScheme,
}
)

Expand Down Expand Up @@ -121,7 +123,7 @@ func main() {
needLeaderElection := !ci.IsRunningLocally()

// Create a new Cmd to provide shared dependencies and start components
mgr, err := manager.New(cfg, getManagerOptions(operatorNamespace, needLeaderElection, ci.IsMonitoringAvailable(), ci.IsOpenshift(), scheme))
mgr, err := manager.New(cfg, getManagerOptions(operatorNamespace, needLeaderElection, ci.IsMonitoringAvailable(), ci.IsDeschedulerAvailable(), ci.IsOpenshift(), scheme))
cmdHelper.ExitOnError(err, "can't initiate manager")

// register pprof instrumentation if HCO_PPROF_ADDR is set
Expand Down Expand Up @@ -203,7 +205,7 @@ func main() {

// Restricts the cache's ListWatch to specific fields/labels per GVK at the specified object to control the memory impact
// this is used to completely overwrite the NewCache function so all the interesting objects should be explicitly listed here
func getCacheOption(operatorNamespace string, isMonitoringAvailable, isOpenshift bool) cache.Options {
func getCacheOption(operatorNamespace string, isMonitoringAvailable, isDeschedulerAvailable, isOpenshift bool) cache.Options {
namespaceSelector := fields.Set{"metadata.namespace": operatorNamespace}.AsSelector()
labelSelector := labels.Set{hcoutil.AppLabel: hcoutil.HyperConvergedName}.AsSelector()
labelSelectorForNamespace := labels.Set{hcoutil.KubernetesMetadataName: operatorNamespace}.AsSelector()
Expand Down Expand Up @@ -257,6 +259,10 @@ func getCacheOption(operatorNamespace string, isMonitoringAvailable, isOpenshift
},
}

cacheOptionsByObjectForDescheduler := map[client.Object]cache.ByObject{
&deschedulerv1.KubeDescheduler{}: {},
}

cacheOptionsByObjectForOpenshift := map[client.Object]cache.ByObject{
&openshiftroutev1.Route{}: {
Namespaces: map[string]cache.Config{
Expand All @@ -282,6 +288,9 @@ func getCacheOption(operatorNamespace string, isMonitoringAvailable, isOpenshift
if isMonitoringAvailable {
maps.Copy(cacheOptions.ByObject, cacheOptionsByObjectForMonitoring)
}
if isDeschedulerAvailable {
maps.Copy(cacheOptions.ByObject, cacheOptionsByObjectForDescheduler)
}
if isOpenshift {
maps.Copy(cacheOptions.ByObject, cacheOptionsByObjectForOpenshift)
}
Expand All @@ -290,7 +299,7 @@ func getCacheOption(operatorNamespace string, isMonitoringAvailable, isOpenshift

}

func getManagerOptions(operatorNamespace string, needLeaderElection, isMonitoringAvailable, isOpenshift bool, scheme *apiruntime.Scheme) manager.Options {
func getManagerOptions(operatorNamespace string, needLeaderElection, isMonitoringAvailable, isDeschedulerAvailable, isOpenshift bool, scheme *apiruntime.Scheme) manager.Options {
return manager.Options{
Metrics: server.Options{
BindAddress: fmt.Sprintf("%s:%d", hcoutil.MetricsHost, hcoutil.MetricsPort),
Expand All @@ -305,7 +314,7 @@ func getManagerOptions(operatorNamespace string, needLeaderElection, isMonitorin
// "configmapsleases". Therefore, having only "leases" should be safe now.
LeaderElectionResourceLock: resourcelock.LeasesResourceLock,
LeaderElectionID: "hyperconverged-cluster-operator-lock",
Cache: getCacheOption(operatorNamespace, isMonitoringAvailable, isOpenshift),
Cache: getCacheOption(operatorNamespace, isMonitoringAvailable, isDeschedulerAvailable, isOpenshift),
Scheme: scheme,
}
}
Expand Down
40 changes: 39 additions & 1 deletion controllers/commontestutils/testUtils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
imagev1 "github.com/openshift/api/image/v1"
operatorv1 "github.com/openshift/api/operator/v1"
routev1 "github.com/openshift/api/route/v1"
deschedulerv1 "github.com/openshift/cluster-kube-descheduler-operator/pkg/apis/descheduler/v1"
csvv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -164,6 +165,7 @@ func GetScheme() *runtime.Scheme {
openshiftconfigv1.Install,
csvv1alpha1.AddToScheme,
aaqv1alpha1.AddToScheme,
deschedulerv1.AddToScheme,
} {
Expect(f(testScheme)).ToNot(HaveOccurred())
}
Expand Down Expand Up @@ -307,6 +309,15 @@ func (c ClusterInfoMock) IsConsolePluginImageProvided() bool {
func (c ClusterInfoMock) IsMonitoringAvailable() bool {
return true
}
func (c ClusterInfoMock) IsDeschedulerAvailable() bool {
return true
}
func (c ClusterInfoMock) IsDeschedulerCRDDeployed(_ context.Context, _ client.Client) bool {
return true
}
func (c ClusterInfoMock) IsDeschedulerMisconfigured() bool {
return false
}
func (c ClusterInfoMock) IsSingleStackIPv6() bool {
return true
}
Expand All @@ -330,6 +341,9 @@ func (ClusterInfoMock) GetTLSSecurityProfile(_ *openshiftconfigv1.TLSSecurityPro
func (ClusterInfoMock) RefreshAPIServerCR(_ context.Context, _ client.Client) error {
return nil
}
func (ClusterInfoMock) RefreshDeschedulerCR(_ context.Context, _ client.Client) error {
return nil
}

// ClusterInfoSNOMock mocks Openshift SNO
type ClusterInfoSNOMock struct{}
Expand Down Expand Up @@ -378,12 +392,24 @@ func (ClusterInfoSNOMock) GetTLSSecurityProfile(_ *openshiftconfigv1.TLSSecurity
func (ClusterInfoSNOMock) RefreshAPIServerCR(_ context.Context, _ client.Client) error {
return nil
}
func (ClusterInfoSNOMock) RefreshDeschedulerCR(_ context.Context, _ client.Client) error {
return nil
}
func (ClusterInfoSNOMock) IsConsolePluginImageProvided() bool {
return true
}
func (c ClusterInfoSNOMock) IsMonitoringAvailable() bool {
return true
}
func (c ClusterInfoSNOMock) IsDeschedulerAvailable() bool {
return true
}
func (c ClusterInfoSNOMock) IsDeschedulerCRDDeployed(_ context.Context, _ client.Client) bool {
return true
}
func (c ClusterInfoSNOMock) IsDeschedulerMisconfigured() bool {
return false
}
func (c ClusterInfoSNOMock) IsSingleStackIPv6() bool {
return true
}
Expand Down Expand Up @@ -432,7 +458,16 @@ func (ClusterInfoSRCPHAIMock) IsConsolePluginImageProvided() bool {
func (ClusterInfoSRCPHAIMock) IsMonitoringAvailable() bool {
return true
}
func (m ClusterInfoSRCPHAIMock) IsSingleStackIPv6() bool {
func (ClusterInfoSRCPHAIMock) IsDeschedulerAvailable() bool {
return true
}
func (ClusterInfoSRCPHAIMock) IsDeschedulerCRDDeployed(_ context.Context, _ client.Client) bool {
return true
}
func (ClusterInfoSRCPHAIMock) IsDeschedulerMisconfigured() bool {
return false
}
func (ClusterInfoSRCPHAIMock) IsSingleStackIPv6() bool {
return true
}
func (ClusterInfoSRCPHAIMock) GetTLSSecurityProfile(_ *openshiftconfigv1.TLSSecurityProfile) *openshiftconfigv1.TLSSecurityProfile {
Expand All @@ -444,6 +479,9 @@ func (ClusterInfoSRCPHAIMock) GetTLSSecurityProfile(_ *openshiftconfigv1.TLSSecu
func (ClusterInfoSRCPHAIMock) RefreshAPIServerCR(_ context.Context, _ client.Client) error {
return nil
}
func (ClusterInfoSRCPHAIMock) RefreshDeschedulerCR(_ context.Context, _ client.Client) error {
return nil
}

func KeysFromSSMap(ssmap map[string]string) gstruct.Keys {
keys := gstruct.Keys{}
Expand Down
95 changes: 90 additions & 5 deletions controllers/hyperconverged/hyperconverged_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
consolev1 "github.com/openshift/api/console/v1"
imagev1 "github.com/openshift/api/image/v1"
routev1 "github.com/openshift/api/route/v1"
deschedulerv1 "github.com/openshift/cluster-kube-descheduler-operator/pkg/apis/descheduler/v1"
operatorhandler "github.com/operator-framework/operator-lib/handler"
"github.com/pkg/errors"
monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
Expand Down Expand Up @@ -82,9 +83,10 @@ const (
systemHealthStatusWarning = "warning"
systemHealthStatusError = "error"

hcoVersionName = "operator"
secondaryCRPrefix = "hco-controlled-cr-"
apiServerCRPrefix = "api-server-cr-"
hcoVersionName = "operator"
secondaryCRPrefix = "hco-controlled-cr-"
apiServerCRPrefix = "api-server-cr-"
deschedulerCRPrefix = "descheduler-cr-"

// These group are no longer supported. Use these constants to remove unused resources
v2vGroup = "v2v.kubevirt.io"
Expand Down Expand Up @@ -233,6 +235,11 @@ func add(mgr manager.Manager, r reconcile.Reconciler, ci hcoutil.ClusterInfo) er
return err
}

deschedulerCRPlaceholder, err := getDeschedulerCRPlaceholder()
if err != nil {
return err
}

if ci.IsOpenshift() {
// Watch openshiftconfigv1.APIServer separately
msg := "Reconciling for openshiftconfigv1.APIServer"
Expand All @@ -255,6 +262,30 @@ func add(mgr manager.Manager, r reconcile.Reconciler, ci hcoutil.ClusterInfo) er
return err
}
}

if ci.IsDeschedulerAvailable() {
// Watch KubeDescheduler separately
msg := "Reconciling for KubeDescheduler"

err = c.Watch(
source.Kind(
mgr.GetCache(),
client.Object(&deschedulerv1.KubeDescheduler{}),
handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request {
// enqueue using a placeholder to signal that the change is not
// directly on HCO CR but on the Descheduler CR that we want to reload
// only if really changed
log.Info(msg)
return []reconcile.Request{
{NamespacedName: deschedulerCRPlaceholder},
}
}),
))
if err != nil {
return err
}
}

return nil
}

Expand Down Expand Up @@ -345,13 +376,25 @@ func (r *ReconcileHyperConverged) Reconcile(ctx context.Context, request reconci
result.Requeue = true
}

// the controller-runtime caching client can only handle kinds
// that were already defined when the client cache got initialized.
// If a new relevant kind got deployed at runtime,
// the operator should restart to be able to read it.
// See: https://github.com/kubernetes-sigs/controller-runtime/issues/2456
if !hcoutil.GetClusterInfo().IsDeschedulerAvailable() {
if hcoutil.GetClusterInfo().IsDeschedulerCRDDeployed(ctx, r.client) {
logger.Info("Failed validating upgrade patches file")
r.eventEmitter.EmitEvent(hcoRequest.Instance, corev1.EventTypeNormal, "KubeDescheduler CRD got deployed, restarting the operator to reconfigure the operator for the new kind", "Restarting the operator to be able to read KubeDescheduler CRs ")
os.Exit(0)
}
}

return result, err
}

// resolveReconcileRequest returns a reconcile.Request to be used throughout the reconciliation cycle,
// regardless of which resource has triggered it.
func (r *ReconcileHyperConverged) resolveReconcileRequest(ctx context.Context, logger logr.Logger, originalRequest reconcile.Request) (reconcile.Request, bool, error) {

hcoTriggered, err := isTriggeredByHyperConverged(originalRequest)
if err != nil {
return reconcile.Request{}, hcoTriggered, err
Expand Down Expand Up @@ -384,6 +427,21 @@ func (r *ReconcileHyperConverged) resolveReconcileRequest(ctx context.Context, l
return resolvedRequest, hcoTriggered, nil
}

deschedulerCRTriggered, err := isTriggeredByDeschedulerCR(originalRequest)
if err != nil {
return reconcile.Request{}, hcoTriggered, err
}
if deschedulerCRTriggered {
logger.Info("Triggered by Descheduler CR, refreshing it")
err = hcoutil.GetClusterInfo().RefreshDeschedulerCR(ctx, r.client)
if err != nil {
return reconcile.Request{}, hcoTriggered, err
}
metrics.SetHCOMetricMisconfiguredDescheduler(hcoutil.GetClusterInfo().IsDeschedulerMisconfigured())
hcoTriggered = false
return resolvedRequest, hcoTriggered, nil
}

logger.Info("The reconciliation got triggered by a secondary CR object")
return resolvedRequest, hcoTriggered, nil
}
Expand All @@ -397,8 +455,12 @@ func isTriggeredByHyperConverged(request reconcile.Request) (bool, error) {
if err != nil {
return false, err
}
deschedulerPlaceholder, err := getDeschedulerCRPlaceholder()
if err != nil {
return false, err
}

isHyperConverged := request.NamespacedName != placeholder && request.NamespacedName != apiServerPlaceholder
isHyperConverged := request.NamespacedName != placeholder && request.NamespacedName != apiServerPlaceholder && request.NamespacedName != deschedulerPlaceholder
return isHyperConverged, nil
}

Expand All @@ -411,6 +473,15 @@ func isTriggeredByAPIServerCR(request reconcile.Request) (bool, error) {
return request.NamespacedName == placeholder, nil
}

func isTriggeredByDeschedulerCR(request reconcile.Request) (bool, error) {
placeholder, err := getDeschedulerCRPlaceholder()
if err != nil {
return false, err
}

return request.NamespacedName == placeholder, nil
}

func (r *ReconcileHyperConverged) doReconcile(req *common.HcoRequest) (reconcile.Result, error) {

valid, err := r.validateNamespace(req)
Expand Down Expand Up @@ -1451,6 +1522,20 @@ func getAPIServerCRPlaceholder() (types.NamespacedName, error) {
return fakeHco, nil
}

func getDeschedulerCRPlaceholder() (types.NamespacedName, error) {
fakeHco := types.NamespacedName{
Name: deschedulerCRPrefix + randomConstSuffix,
}

namespace, err := hcoutil.GetOperatorNamespaceFromEnv()
if err != nil {
return fakeHco, err
}
fakeHco.Namespace = namespace

return fakeHco, nil
}

func drop(slice []string, s string) ([]string, bool) {
var newSlice []string
dropped := false
Expand Down
Loading

0 comments on commit cc2820d

Please sign in to comment.