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

Vsphere enable autoscaling from/to zero #839

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions cmd/vsphere/main.go
Expand Up @@ -11,13 +11,16 @@ import (
vsphereapis "github.com/openshift/machine-api-operator/pkg/apis/vsphereprovider"
capimachine "github.com/openshift/machine-api-operator/pkg/controller/machine"
machine "github.com/openshift/machine-api-operator/pkg/controller/vsphere"
machinesetcontroller "github.com/openshift/machine-api-operator/pkg/controller/vsphere/machineset"
"github.com/openshift/machine-api-operator/pkg/metrics"
"github.com/openshift/machine-api-operator/pkg/version"
"k8s.io/klog/v2"
"k8s.io/klog/v2/klogr"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client/config"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/manager/signals"
)

// The default durations for the leader electrion operations.
Expand Down Expand Up @@ -128,6 +131,16 @@ func main() {

capimachine.AddWithActuator(mgr, machineActuator)

ctrl.SetLogger(klogr.New())
setupLog := ctrl.Log.WithName("setup")
if err = (&machinesetcontroller.Reconciler{
Client: mgr.GetClient(),
Log: ctrl.Log.WithName("controllers").WithName("MachineSet"),
}).SetupWithManager(mgr, controller.Options{}); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "MachineSet")
os.Exit(1)
}

if err := mgr.AddReadyzCheck("ping", healthz.Ping); err != nil {
klog.Fatal(err)
}
Expand All @@ -136,7 +149,7 @@ func main() {
klog.Fatal(err)
}

if err := mgr.Start(signals.SetupSignalHandler()); err != nil {
if err = mgr.Start(ctrl.SetupSignalHandler()); err != nil {
klog.Fatalf("Failed to run manager: %v", err)
}
}
1 change: 1 addition & 0 deletions go.mod
Expand Up @@ -4,6 +4,7 @@ go 1.13

require (
github.com/blang/semver v3.5.1+incompatible
github.com/go-logr/logr v0.3.0
Copy link
Contributor

@lobziik lobziik Apr 7, 2021

Choose a reason for hiding this comment

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

Do we really need this? Can we use klog as we do everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

explanation by @JoelSpeed: "This interface is what controller runtime uses, so if we just use klog directly, we lose the logging that controller runtime would emit, so it's better to use the logger interface so we are sharing what controller runtime is doing"

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 already a dependency, though just be an indirect dependency up to this point, looks like we have already run make vendor on this PR right? As there are no changes to go.sum this shows it's not a new depdencency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, seems thats the case.

github.com/google/gofuzz v1.1.0
github.com/google/uuid v1.1.2
github.com/onsi/ginkgo v1.14.1
Expand Down
122 changes: 122 additions & 0 deletions pkg/controller/vsphere/machineset/controller.go
@@ -0,0 +1,122 @@
package machineset

import (
"context"
"fmt"
"strconv"

"github.com/go-logr/logr"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
providerconfigv1 "github.com/openshift/machine-api-operator/pkg/apis/vsphereprovider/v1beta1"
mapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
)

const (
// This exposes compute information based on the providerSpec input.
// This is needed by the autoscaler to foresee upcoming capacity when scaling from zero.
// https://github.com/openshift/enhancements/pull/186
cpuKey = "machine.openshift.io/vCPU"
memoryKey = "machine.openshift.io/memoryMb"
)

// Reconciler reconciles machineSets.
type Reconciler struct {
Client client.Client
Log logr.Logger

recorder record.EventRecorder
scheme *runtime.Scheme
}

// SetupWithManager creates a new controller for a manager.
func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
_, err := ctrl.NewControllerManagedBy(mgr).
For(&machinev1.MachineSet{}).
WithOptions(options).
Build(r)

if err != nil {
return fmt.Errorf("failed setting up with a controller manager: %w", err)
}

r.recorder = mgr.GetEventRecorderFor("machineset-controller")
r.scheme = mgr.GetScheme()
return nil
}

// Reconcile implements controller runtime Reconciler interface.
func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
logger := r.Log.WithValues("machineset", req.Name, "namespace", req.Namespace)
logger.V(3).Info("Reconciling")

machineSet := &machinev1.MachineSet{}
if err := r.Client.Get(ctx, req.NamespacedName, machineSet); err != nil {
if apierrors.IsNotFound(err) {
// Object not found, return. Created objects are automatically garbage collected.
// For additional cleanup logic use finalizers.
return ctrl.Result{}, nil
}
// Error reading the object - requeue the request.
return ctrl.Result{}, err
}

// Ignore deleted MachineSets, this can happen when foregroundDeletion
// is enabled
if !machineSet.DeletionTimestamp.IsZero() {
return ctrl.Result{}, nil
}
originalMachineSetToPatch := client.MergeFrom(machineSet.DeepCopy())

result, err := reconcile(machineSet)
if err != nil {
logger.Error(err, "Failed to reconcile MachineSet")
r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "ReconcileError", "%v", err)
// we don't return here so we want to attempt to patch the machine regardless of an error.
}

if err := r.Client.Patch(ctx, machineSet, originalMachineSetToPatch); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to patch machineSet: %v", err)
}

if isInvalidConfigurationError(err) {
// For situations where requeuing won't help we don't return error.
// https://github.com/kubernetes-sigs/controller-runtime/issues/617
return result, nil
}

return result, err
}

func isInvalidConfigurationError(err error) bool {
switch t := err.(type) {
case *mapierrors.MachineError:
if t.Reason == machinev1.InvalidConfigurationMachineError {
return true
}
}
return false
Comment on lines +98 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use the errors package you can use errors.Is instead of this function, it detects wrapped errors and unwraps them until it finds the error you wanted (if it was ever in the chain)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further talk with @JoelSpeed we decided the change is not actually worth here.

}

func reconcile(machineSet *machinev1.MachineSet) (ctrl.Result, error) {
providerConfig, err := providerconfigv1.ProviderSpecFromRawExtension(machineSet.Spec.Template.Spec.ProviderSpec.Value)
if err != nil {
return ctrl.Result{}, mapierrors.InvalidMachineConfiguration("failed to get providerConfig: %v", err)
}

if machineSet.Annotations == nil {
machineSet.Annotations = make(map[string]string)
}

// TODO: get annotations keys from machine API
machineSet.Annotations[cpuKey] = strconv.FormatInt(int64(providerConfig.NumCPUs), 10)
machineSet.Annotations[memoryKey] = strconv.FormatInt(providerConfig.MemoryMiB, 10)

return ctrl.Result{}, nil
}
79 changes: 79 additions & 0 deletions pkg/controller/vsphere/machineset/controller_suite_test.go
@@ -0,0 +1,79 @@
/*
Copyright The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package machineset

import (
"context"
"path/filepath"
"testing"
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
"sigs.k8s.io/controller-runtime/pkg/manager"
)

const (
timeout = 10 * time.Second
)

var (
cfg *rest.Config
testEnv *envtest.Environment

ctx = context.Background()
)

func TestReconciler(t *testing.T) {
RegisterFailHandler(Fail)

RunSpecsWithDefaultAndCustomReporters(t,
"Controller Suite",
[]Reporter{printer.NewlineReporter{}})
}

var _ = BeforeSuite(func() {
testEnv = &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "install")},
}
machinev1.AddToScheme(scheme.Scheme)

var err error
cfg, err = testEnv.Start()
Expect(err).ToNot(HaveOccurred())
Expect(cfg).ToNot(BeNil())
})

var _ = AfterSuite(func() {
Expect(testEnv.Stop()).To(Succeed())
})

// StartTestManager adds recFn
func StartTestManager(mgr manager.Manager) context.CancelFunc {
mgrCtx, cancel := context.WithCancel(ctx)
go func() {
defer GinkgoRecover()

Expect(mgr.Start(mgrCtx)).To(Succeed())
}()
return cancel
}