Skip to content

Commit

Permalink
[release/v1.54] Allow external CCMs to handle node objects before MC (#…
Browse files Browse the repository at this point in the history 
…1654)

* Run VMs with run strategy 'Once'

Signed-off-by: Marcin Franczyk <[email protected]>

* Allow external CCMs to handle node objects before MC.

Otherwise we have a race condition between MC and CCM.
Both try to check status of instances at cloud provider.
If MC reconciles instances first then kubelet will reuse the old node object
which is problematic in case of IP change.

Signed-off-by: Marcin Franczyk <[email protected]>

* Update doc of handleNodeFailuresWithExternalCCM func

Signed-off-by: Marcin Franczyk <[email protected]>

* Use ceph instead of portworx for KubeVirt tests

Signed-off-by: Marcin Franczyk <[email protected]>

---------

Signed-off-by: Marcin Franczyk <[email protected]>
  • Loading branch information
@mfranczy
mfranczy authored Jun 6, 2023
1 parent e94cc19 commit a0af4c0
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 34 deletions.
5 changes: 3 additions & 2 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ require (
golang.org/x/oauth2 v0.0.0-20220608161450-d0670ef3b1eb
gomodules.xyz/jsonpatch/v2 v2.2.0
google.golang.org/api v0.74.0
google.golang.org/grpc v1.45.0
google.golang.org/grpc v1.47.0
gopkg.in/gcfg.v1 v1.2.3
gopkg.in/yaml.v3 v3.0.1
// Please ensure that you update the image tags in `examples/operating-system-manager.yaml` as well.
Expand All @@ -49,6 +49,7 @@ require (
k8s.io/apiextensions-apiserver v0.24.2
k8s.io/apimachinery v0.24.2
k8s.io/client-go v12.0.0+incompatible
k8s.io/cloud-provider v0.24.2
k8s.io/klog v1.0.0
k8s.io/kubelet v0.24.2
k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
Expand Down Expand Up @@ -143,7 +144,7 @@ require (
golang.org/x/text v0.4.0 // indirect
golang.org/x/time v0.0.0-20220609170525-579cf78fd858 // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/genproto v0.0.0-20220413183235-5e96e2839df9 // indirect
google.golang.org/genproto v0.0.0-20220502173005-c8bf987b8c21 // indirect
google.golang.org/protobuf v1.28.0 // indirect
gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
Expand Down
13 changes: 11 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ github.com/cncf/udpa/go v0.0.0-20210930031921-04548b0d99d4/go.mod h1:6pvJx4me5XP
github.com/cncf/xds/go v0.0.0-20210312221358-fbca930ec8ed/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cncf/xds/go v0.0.0-20210805033703-aa0b78936158/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cncf/xds/go v0.0.0-20210922020428-25de7278fc84/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cncf/xds/go v0.0.0-20211001041855-01bcc9b48dfe/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8=
github.com/cockroachdb/datadriven v0.0.0-20200714090401-bf6692d28da5/go.mod h1:h6jFvWxBdQXxjopDMZyH2UVceIRfR84bdzbkoKrsWNo=
Expand Down Expand Up @@ -255,6 +256,7 @@ github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.m
github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk=
github.com/envoyproxy/go-control-plane v0.9.9-0.20210512163311-63b5d3c536b0/go.mod h1:hliV/p42l8fGbc6Y9bQ70uLwIvmJyVE5k4iMKlh8wCQ=
github.com/envoyproxy/go-control-plane v0.9.10-0.20210907150352-cf90f659a021/go.mod h1:AFq3mo9L8Lqqiid3OhADV3RfLJnjiw63cSpi+fDTRC0=
github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ=
github.com/evanphx/json-patch v4.12.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
Expand Down Expand Up @@ -1407,8 +1409,9 @@ google.golang.org/genproto v0.0.0-20220304144024-325a89244dc8/go.mod h1:kGP+zUP2
google.golang.org/genproto v0.0.0-20220310185008-1973136f34c6/go.mod h1:kGP+zUP2Ddo0ayMi4YuN7C3WZyJvGLZRh8Z5wnAqvEI=
google.golang.org/genproto v0.0.0-20220324131243-acbaeb5b85eb/go.mod h1:hAL49I2IFola2sVEjAn7MEwsja0xp51I0tlGAf9hz4E=
google.golang.org/genproto v0.0.0-20220407144326-9054f6ed7bac/go.mod h1:8w6bsBMX6yCPbAVTeqQHvzxW0EIFigd5lZyahWgyfDo=
google.golang.org/genproto v0.0.0-20220413183235-5e96e2839df9 h1:XGQ6tc+EnM35IAazg4y6AHmUg4oK8NXsXaILte1vRlk=
google.golang.org/genproto v0.0.0-20220413183235-5e96e2839df9/go.mod h1:8w6bsBMX6yCPbAVTeqQHvzxW0EIFigd5lZyahWgyfDo=
google.golang.org/genproto v0.0.0-20220502173005-c8bf987b8c21 h1:hrbNEivu7Zn1pxvHk6MBrq9iE22woVILTHqexqBxe6I=
google.golang.org/genproto v0.0.0-20220502173005-c8bf987b8c21/go.mod h1:RAyBrSAP7Fh3Nc84ghnVLDPuV51xc9agzmm4Ph6i0Q4=
google.golang.org/grpc v1.17.0/go.mod h1:6QZJwpn2B+Zp71q/5VxRsJ6NXXVCE5NRUHRo+f3cWCs=
google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
google.golang.org/grpc v1.20.0/go.mod h1:chYK+tFQF0nDUGJgXMSgLCQk3phJEuONr2DCgLDdAQM=
Expand Down Expand Up @@ -1443,8 +1446,10 @@ google.golang.org/grpc v1.39.1/go.mod h1:PImNr+rS9TWYb2O4/emRugxiyHZ5JyHW5F+RPnD
google.golang.org/grpc v1.40.0/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34=
google.golang.org/grpc v1.40.1/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34=
google.golang.org/grpc v1.44.0/go.mod h1:k+4IHHFw41K8+bbowsex27ge2rCb65oeWqe4jJ590SU=
google.golang.org/grpc v1.45.0 h1:NEpgUqV3Z+ZjkqMsxMg11IaDrXY4RY6CQukSGK0uI1M=
google.golang.org/grpc v1.45.0/go.mod h1:lN7owxKUQEqMfSyQikvvk5tf/6zMPsrK+ONuO11+0rQ=
google.golang.org/grpc v1.46.0/go.mod h1:vN9eftEi1UMyUsIF80+uQXhHjbXYbm0uXoFCACuMGWk=
google.golang.org/grpc v1.47.0 h1:9n77onPX5F3qfFCqjy9dhn8PbNQsIKeVU04J9G7umt8=
google.golang.org/grpc v1.47.0/go.mod h1:vN9eftEi1UMyUsIF80+uQXhHjbXYbm0uXoFCACuMGWk=
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw=
google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
Expand Down Expand Up @@ -1532,10 +1537,14 @@ k8s.io/apimachinery v0.24.2/go.mod h1:82Bi4sCzVBdpYjyI4jY6aHX+YCUchUIrZrXKedjd2U
k8s.io/apiserver v0.24.2/go.mod h1:pSuKzr3zV+L+MWqsEo0kHHYwCo77AT5qXbFXP2jbvFI=
k8s.io/client-go v0.24.2 h1:CoXFSf8if+bLEbinDqN9ePIDGzcLtqhfd6jpfnwGOFA=
k8s.io/client-go v0.24.2/go.mod h1:zg4Xaoo+umDsfCWr4fCnmLEtQXyCNXCvJuSsglNcV30=
k8s.io/cloud-provider v0.24.2 h1:DYNf90zS/GAQbEHsTfJsH4Oas7vim4U+WU9GftMQlfs=
k8s.io/cloud-provider v0.24.2/go.mod h1:a7jyWjizk+IKbcIf8+mX2cj3NvpRv9ZyGdXDyb8UEkI=
k8s.io/code-generator v0.23.3/go.mod h1:S0Q1JVA+kSzTI1oUvbKAxZY/DYbA/ZUb4Uknog12ETk=
k8s.io/code-generator v0.24.2/go.mod h1:dpVhs00hTuTdTY6jvVxvTFCk6gSMrtfRydbhZwHI15w=
k8s.io/component-base v0.24.2 h1:kwpQdoSfbcH+8MPN4tALtajLDfSfYxBDYlXobNWI6OU=
k8s.io/component-base v0.24.2/go.mod h1:ucHwW76dajvQ9B7+zecZAP3BVqvrHoOxm8olHEg0nmM=
k8s.io/component-helpers v0.24.2/go.mod h1:TRQPBQKfmqkmV6c0HAmUs8cXVNYYYLsXy4zu8eODi9g=
k8s.io/controller-manager v0.24.2/go.mod h1:hpwCof4KxP4vrw/M5QiVxU6Zmmggmr1keGXtjGHF+vc=
k8s.io/gengo v0.0.0-20210813121822-485abfe95c7c/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
k8s.io/gengo v0.0.0-20211129171323-c02415ce4185/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=
Expand Down
17 changes: 3 additions & 14 deletions pkg/cloudprovider/provider/kubevirt/provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,18 +394,6 @@ func (p *provider) Get(ctx context.Context, machine *clusterv1alpha1.Machine, _
return nil, cloudprovidererrors.ErrInstanceNotFound
}

if virtualMachineInstance.Status.Phase == kubevirtv1.Failed ||
// The VMI enters phase succeeded if someone issues a kubectl
// delete pod on the virt-launcher pod it runs in
virtualMachineInstance.Status.Phase == kubevirtv1.Succeeded {
// The pod got deleted, delete the VMI and return ErrNotFound so the VMI
// will get recreated
if err := sigClient.Delete(ctx, virtualMachineInstance); err != nil {
return nil, fmt.Errorf("failed to delete failed VMI %s: %w", machine.Name, err)
}
return nil, cloudprovidererrors.ErrInstanceNotFound
}

return &kubeVirtServer{vmi: *virtualMachineInstance}, nil
}

Expand Down Expand Up @@ -543,14 +531,16 @@ func (p *provider) Create(ctx context.Context, machine *clusterv1alpha1.Machine,
return nil, fmt.Errorf("could not compute a random MAC address")
}

runStrategyOnce := kubevirtv1.RunStrategyOnce

virtualMachine := &kubevirtv1.VirtualMachine{
ObjectMeta: metav1.ObjectMeta{
Name: machine.Name,
Namespace: c.Namespace,
Labels: labels,
},
Spec: kubevirtv1.VirtualMachineSpec{
Running: utilpointer.BoolPtr(true),
RunStrategy: &runStrategyOnce,
Template: &kubevirtv1.VirtualMachineInstanceTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Annotations: annotations,
Expand Down Expand Up @@ -614,7 +604,6 @@ func (p *provider) Cleanup(ctx context.Context, machine *clusterv1alpha1.Machine
if !kerrors.IsNotFound(err) {
return false, fmt.Errorf("failed to get VirtualMachineInstance %s: %w", machine.Name, err)
}
// VMI is gone
return true, nil
}

Expand Down
91 changes: 76 additions & 15 deletions pkg/controller/machine/machine_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ import (
"k8s.io/client-go/tools/record"
"k8s.io/client-go/tools/reference"
"k8s.io/client-go/util/retry"
ccmapi "k8s.io/cloud-provider/api"
"k8s.io/klog"
"sigs.k8s.io/controller-runtime/pkg/client"
ctrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -419,7 +420,8 @@ func (r *Reconciler) reconcile(ctx context.Context, machine *clusterv1alpha1.Mac

// step 2: check if a user requested to delete the machine
if machine.DeletionTimestamp != nil {
return r.deleteMachine(ctx, prov, providerConfig.CloudProvider, machine)
skipEviction := false
return r.deleteMachine(ctx, prov, providerConfig.CloudProvider, machine, skipEviction)
}

// Step 3: Essentially creates an instance for the given machine.
Expand Down Expand Up @@ -453,6 +455,9 @@ func (r *Reconciler) reconcile(ctx context.Context, machine *clusterv1alpha1.Mac
}
} else {
// Node is not ready anymore? Maybe it got deleted
if r.nodeSettings.ExternalCloudProvider {
return r.handleNodeFailuresWithExternalCCM(ctx, prov, providerConfig, node, machine)
}
return r.ensureInstanceExistsForMachine(ctx, prov, machine, userdataPlugin, providerConfig)
}

Expand Down Expand Up @@ -504,7 +509,7 @@ func (r *Reconciler) shouldCleanupVolumes(ctx context.Context, machine *clusterv
func (r *Reconciler) shouldEvict(ctx context.Context, machine *clusterv1alpha1.Machine) (bool, error) {
// If the deletion got triggered a few hours ago, skip eviction.
// We assume here that the eviction is blocked by misconfiguration or a misbehaving kubelet and/or controller-runtime
if time.Since(machine.DeletionTimestamp.Time) > r.skipEvictionAfter {
if machine.DeletionTimestamp != nil && time.Since(machine.DeletionTimestamp.Time) > r.skipEvictionAfter {
klog.V(0).Infof("Skipping eviction for machine %q since the deletion got triggered %.2f minutes ago", machine.Name, r.skipEvictionAfter.Minutes())
return false, nil
}
Expand Down Expand Up @@ -559,11 +564,25 @@ func (r *Reconciler) shouldEvict(ctx context.Context, machine *clusterv1alpha1.M
}

// deleteMachine makes sure that an instance has gone in a series of steps.
func (r *Reconciler) deleteMachine(ctx context.Context, prov cloudprovidertypes.Provider, providerName providerconfigtypes.CloudProvider, machine *clusterv1alpha1.Machine) (*reconcile.Result, error) {
shouldEvict, err := r.shouldEvict(ctx, machine)
if err != nil {
return nil, err
func (r *Reconciler) deleteMachine(
ctx context.Context,
prov cloudprovidertypes.Provider,
providerName providerconfigtypes.CloudProvider,
machine *clusterv1alpha1.Machine,
skipEviction bool,
) (*reconcile.Result, error) {
var (
shouldEvict bool
err error
)

if !skipEviction {
shouldEvict, err = r.shouldEvict(ctx, machine)
if err != nil {
return nil, err
}
}

shouldCleanUpVolumes, err := r.shouldCleanupVolumes(ctx, machine, providerName)
if err != nil {
return nil, err
Expand Down Expand Up @@ -667,7 +686,6 @@ func (r *Reconciler) deleteCloudProviderInstance(ctx context.Context, prov cloud
message := fmt.Sprintf("%v. Please manually delete %s finalizer from the machine object.", err, FinalizerDeleteInstance)
return nil, r.updateMachineErrorIfTerminalError(machine, common.DeleteMachineError, message, err, "failed to delete machine at cloud provider")
}

if !completelyGone {
// As the instance is not completely gone yet, we need to recheck in a few seconds.
return &reconcile.Result{RequeueAfter: deletionRetryWaitPeriod}, nil
Expand Down Expand Up @@ -1036,14 +1054,6 @@ func (r *Reconciler) ensureNodeLabelsAnnotationsAndTaints(ctx context.Context, n
modifiers = append(modifiers, f(AnnotationAutoscalerIdentifier, autoscalerAnnotationValue))
}

taintExists := func(node *corev1.Node, taint corev1.Taint) bool {
for _, t := range node.Spec.Taints {
if t.MatchTaint(&taint) {
return true
}
}
return false
}
for _, t := range machine.Spec.Taints {
if !taintExists(node, t) {
f := func(t corev1.Taint) func(*corev1.Node) {
Expand Down Expand Up @@ -1166,6 +1176,15 @@ func findNodeByProviderID(instance instance.Instance, provider providerconfigtyp
return nil
}

func taintExists(node *corev1.Node, taint corev1.Taint) bool {
for _, t := range node.Spec.Taints {
if t.MatchTaint(&taint) {
return true
}
}
return false
}

func (r *Reconciler) ReadinessChecks(ctx context.Context) map[string]healthcheck.Check {
return map[string]healthcheck.Check{
"valid-info-kubeconfig": func() error {
Expand Down Expand Up @@ -1225,3 +1244,45 @@ func (r *Reconciler) updateNode(ctx context.Context, node *corev1.Node, modifier
return r.client.Update(ctx, node)
})
}

// handleNodeFailuresWithExternalCCM reacts to node status discovery of CCM's node lifecycle controller.
// If an instance at cloud provider is not found then it waits till CCM deletes node objects, that allows:
// - create a new instance at cloud provider
// - initialize a new node object - the object should not be reused between instance creation
// for example, instance foo that got deleted and recreated should initialize a completely new node object
// instead of reusing the old one as it can cause problems to update node's metadata, like IP address.
//
// If node is shut-down it allows MC to react accordingly to specific cloud provider requirements, those are:
// - wait for node to become online again or
// - delete a machine which cannot be recovered
func (r *Reconciler) handleNodeFailuresWithExternalCCM(
ctx context.Context,
prov cloudprovidertypes.Provider,
provConfig *providerconfigtypes.Config,
node *corev1.Node,
machine *clusterv1alpha1.Machine,
) (*reconcile.Result, error) {
taintShutdown := corev1.Taint{
Key: ccmapi.TaintNodeShutdown,
Effect: corev1.TaintEffectNoSchedule,
}

_, err := prov.Get(ctx, machine, r.providerData)
if err != nil {
if cloudprovidererrors.IsNotFound(err) {
klog.V(0).Infof("The node %q does not have corresponding instance, waiting for CCM to delete it", node.Name)
return &reconcile.Result{RequeueAfter: deletionRetryWaitPeriod}, nil
}
return nil, err
} else if taintExists(node, taintShutdown) {
switch provConfig.CloudProvider {
case providerconfigtypes.CloudProviderKubeVirt:
klog.V(0).Infof("Deleting a shut-down machine %q that cannot recover", machine.Name)
skipEviction := true
return r.deleteMachine(ctx, prov, providerconfigtypes.CloudProviderKubeVirt, machine, skipEviction)
}
}

klog.V(4).Infof("Waiting for a node to become %q", corev1.NodeReady)
return &reconcile.Result{RequeueAfter: deletionRetryWaitPeriod}, err
}
2 changes: 1 addition & 1 deletion test/e2e/provisioning/testdata/machinedeployment-kubevirt.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ spec:
primaryDisk:
osImage: http://image-repo.kube-system.svc/images/<< KUBEVIRT_OS_IMAGE >>.img
size: "25Gi"
storageClassName: px-csi-db
storageClassName: rook-ceph-block
dnsPolicy: "None"
dnsConfig:
nameservers:
Expand Down

0 comments on commit a0af4c0

Please sign in to comment.