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 termination condition to deleting nodes #492

Merged
merged 1 commit into from
Aug 19, 2020
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
1 change: 1 addition & 0 deletions kubernetes/deployment/in-tree/control-cluster-role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ rules:
- ""
resources:
- nodes
- nodes/status
- configmaps
- secrets
- endpoints
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ rules:
- ""
resources:
- nodes
- nodes/status
- configmaps
- secrets
- endpoints
Expand Down
58 changes: 54 additions & 4 deletions pkg/controller/controller_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (

"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
machineapi "github.com/gardener/machine-controller-manager/pkg/client/clientset/versioned/typed/machine/v1alpha1"
conditionutils "github.com/gardener/machine-controller-manager/pkg/util/conditions"
hashutil "github.com/gardener/machine-controller-manager/pkg/util/hash"
taintutils "github.com/gardener/machine-controller-manager/pkg/util/taints"
v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -88,8 +89,8 @@ const (
SlowStartInitialBatchSize = 1
)

// UpdateTaintBackoff is the backoff period used while updating taint
var UpdateTaintBackoff = wait.Backoff{
// Backoff is the backoff period used while updating nodes
var Backoff = wait.Backoff{
Steps: 5,
Duration: 100 * time.Millisecond,
Jitter: 1.0,
Expand Down Expand Up @@ -901,7 +902,7 @@ func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taints ...*v
return nil
}
firstTry := true
return clientretry.RetryOnConflict(UpdateTaintBackoff, func() error {
return clientretry.RetryOnConflict(Backoff, func() error {
var err error
var oldNode *v1.Node
// First we try getting node from the API server cache, as it's cheaper. If it fails
Expand Down Expand Up @@ -958,7 +959,7 @@ func RemoveTaintOffNode(c clientset.Interface, nodeName string, node *v1.Node, t
}

firstTry := true
return clientretry.RetryOnConflict(UpdateTaintBackoff, func() error {
return clientretry.RetryOnConflict(Backoff, func() error {
var err error
var oldNode *v1.Node
// First we try getting node from the API server cache, as it's cheaper. If it fails
Expand Down Expand Up @@ -1032,6 +1033,55 @@ func UpdateNodeTaints(c clientset.Interface, nodeName string, oldNode *v1.Node,
return nil
}

// GetNodeCondition get the nodes condition matching the specified type
func GetNodeCondition(c clientset.Interface, nodeName string, conditionType v1.NodeConditionType) (*v1.NodeCondition, error) {
node, err := c.CoreV1().Nodes().Get(nodeName, metav1.GetOptions{})
if err != nil {
return nil, err
}
return conditionutils.GetNodeCondition(node, conditionType), nil
}

// AddOrUpdateConditionsOnNode adds a condition to the node's status
func AddOrUpdateConditionsOnNode(c clientset.Interface, nodeName string, condition v1.NodeCondition) error {
firstTry := true
return clientretry.RetryOnConflict(Backoff, func() error {
var err error
var oldNode *v1.Node
// First we try getting node from the API server cache, as it's cheaper. If it fails
// we get it from etcd to be sure to have fresh data.
if firstTry {
oldNode, err = c.CoreV1().Nodes().Get(nodeName, metav1.GetOptions{ResourceVersion: "0"})
hardikdr marked this conversation as resolved.
Show resolved Hide resolved
firstTry = false
} else {
oldNode, err = c.CoreV1().Nodes().Get(nodeName, metav1.GetOptions{})
}

if err != nil {
return err
}

var newNode *v1.Node
oldNodeCopy := oldNode
newNode = conditionutils.AddOrUpdateCondition(oldNodeCopy, condition)
return UpdateNodeConditions(c, nodeName, oldNode, newNode)
})
}

// UpdateNodeConditions is for updating the node conditions from oldNode to the newNode
// using the nodes Update() method
func UpdateNodeConditions(c clientset.Interface, nodeName string, oldNode *v1.Node, newNode *v1.Node) error {
newNodeClone := oldNode.DeepCopy()
newNodeClone.Status.Conditions = newNode.Status.Conditions

_, err := c.CoreV1().Nodes().UpdateStatus(newNodeClone)
if err != nil {
hardikdr marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("failed to create update conditions for node %q: %v", nodeName, err)
}

return nil
}

// WaitForCacheSync is a wrapper around cache.WaitForCacheSync that generates log messages
// indicating that the controller identified by controllerName is waiting for syncs, followed by
// either a successful or failed sync.
Expand Down
74 changes: 42 additions & 32 deletions pkg/controller/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -565,10 +565,52 @@ func (c *controller) machineDelete(machine *v1alpha1.Machine, driver driver.Driv

if finalizers := sets.NewString(machine.Finalizers...); finalizers.Has(DeleteFinalizerName) {
klog.V(2).Infof("Deleting Machine %q", machine.Name)
var (
forceDeletePods = false
forceDeleteMachine = false
timeOutOccurred = false
maxEvictRetries = int32(math.Min(float64(*c.getEffectiveMaxEvictRetries(machine)), c.getEffectiveDrainTimeout(machine).Seconds()/PodEvictionRetryInterval.Seconds()))
pvDetachTimeOut = c.safetyOptions.PvDetachTimeout.Duration
timeOutDuration = c.getEffectiveDrainTimeout(machine).Duration
forceDeleteLabelPresent = machine.Labels["force-deletion"] == "True"
)

// Timeout value obtained by subtracting last operation with expected time out period
timeOut := metav1.Now().Add(-timeOutDuration).Sub(machine.Status.CurrentStatus.LastUpdateTime.Time)
timeOutOccurred = timeOut > 0

if forceDeleteLabelPresent || timeOutOccurred {
// To perform forceful machine drain/delete either one of the below conditions must be satified
// 1. force-deletion: "True" label must be present
// 2. Deletion operation is more than drain-timeout minutes old
forceDeleteMachine = true
forceDeletePods = true
timeOutDuration = 1 * time.Minute
maxEvictRetries = 1

klog.V(2).Infof(
"Force deletion has been triggerred for machine %q due to ForceDeletionLabel:%t, Timeout:%t",
machine.Name,
forceDeleteLabelPresent,
timeOutOccurred,
)
}

// If machine was created on the cloud provider
machineID, _ := driver.GetExisting()

// update node with the machine's state prior to termination
Copy link
Contributor

@prashanth26 prashanth26 Nov 20, 2020

Choose a reason for hiding this comment

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

Hi @guydaichs ,

Looks like this change in moving the machine phase to termination is affecting the drain logic making it trigger a force deletion (as force deletion is dependent on the last update time whicch is to be set on machine termination) many of times. Refer - #563. I will have to move the setting machine Terminating phase to before updating the node conditions.

if nodeName != "" && machineID != "" {
if err = c.UpdateNodeTerminationCondition(machine); err != nil {
if forceDeleteMachine {
klog.Warningf("failed to update node conditions: %v. However, since it's a force deletion shall continue deletion of VM.", err)
} else {
klog.Error(err)
return err
}
}
}

if machine.Status.CurrentStatus.Phase != v1alpha1.MachineTerminating {
lastOperation := v1alpha1.LastOperation{
Description: "Deleting machine from cloud provider",
Expand Down Expand Up @@ -601,38 +643,6 @@ func (c *controller) machineDelete(machine *v1alpha1.Machine, driver driver.Driv

if machineID != "" && nodeName != "" {
// Begin drain logic only when the nodeName & providerID exist's for the machine

var (
forceDeletePods = false
forceDeleteMachine = false
timeOutOccurred = false
maxEvictRetries = int32(math.Min(float64(*c.getEffectiveMaxEvictRetries(machine)), c.getEffectiveDrainTimeout(machine).Seconds()/PodEvictionRetryInterval.Seconds()))
pvDetachTimeOut = c.safetyOptions.PvDetachTimeout.Duration
timeOutDuration = c.getEffectiveDrainTimeout(machine).Duration
forceDeleteLabelPresent = machine.Labels["force-deletion"] == "True"
)

// Timeout value obtained by subtracting last operation with expected time out period
timeOut := metav1.Now().Add(-timeOutDuration).Sub(machine.Status.CurrentStatus.LastUpdateTime.Time)
timeOutOccurred = timeOut > 0

if forceDeleteLabelPresent || timeOutOccurred {
// To perform forceful machine drain/delete either one of the below conditions must be satified
// 1. force-deletion: "True" label must be present
// 2. Deletion operation is more than drain-timeout minutes old
forceDeleteMachine = true
forceDeletePods = true
timeOutDuration = 1 * time.Minute
maxEvictRetries = 1

klog.V(2).Infof(
"Force deletion has been triggerred for machine %q due to ForceDeletionLabel:%t, Timeout:%t",
machine.Name,
forceDeleteLabelPresent,
timeOutOccurred,
)
}

buf := bytes.NewBuffer([]byte{})
errBuf := bytes.NewBuffer([]byte{})

Expand Down
56 changes: 56 additions & 0 deletions pkg/controller/machine_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"encoding/json"

"github.com/gardener/machine-controller-manager/pkg/apis/machine/validation"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog"

machineapi "github.com/gardener/machine-controller-manager/pkg/apis/machine"
Expand All @@ -40,6 +42,12 @@ import (
const (
// LastAppliedALTAnnotation contains the last configuration of annotations, labels & taints applied on the node object
LastAppliedALTAnnotation = "node.machine.sapcloud.io/last-applied-anno-labels-taints"
// NodeTerminationCondition describes nodes that are terminating
NodeTerminationCondition = "Termination"
// NodeUnhealthy is a node termination reason for failed machines
NodeUnhealthy = "Unhealthy"
// NodeScaledDown is a node termination reason for healthy deleted machines
NodeScaledDown = "ScaleDown"
)

var (
Expand Down Expand Up @@ -496,3 +504,51 @@ func SyncMachineTaints(

return toBeUpdated
}

func (c *controller) UpdateNodeTerminationCondition(machine *v1alpha1.Machine) error {
if machine.Status.CurrentStatus.Phase == "" {
return nil
}

nodeName := machine.Status.Node

terminationCondition := v1.NodeCondition{
hardikdr marked this conversation as resolved.
Show resolved Hide resolved
Type: NodeTerminationCondition,
Status: v1.ConditionTrue,
LastHeartbeatTime: metav1.Now(),
LastTransitionTime: metav1.Now(),
}

// check if condition already exists
cond, err := GetNodeCondition(c.targetCoreClient, nodeName, NodeTerminationCondition)
if err != nil {
if apierrors.IsNotFound(err) {
return nil
}
return err
}

if cond != nil && machine.Status.CurrentStatus.Phase == v1alpha1.MachineTerminating {
// do not consider machine terminating phase if node already terminating
terminationCondition.Reason = cond.Reason
terminationCondition.Message = cond.Message
} else {
setTerminationReasonByPhase(machine.Status.CurrentStatus.Phase, &terminationCondition)
}

err = AddOrUpdateConditionsOnNode(c.targetCoreClient, nodeName, terminationCondition)
if apierrors.IsNotFound(err) {
return nil
}
return err
}

func setTerminationReasonByPhase(phase v1alpha1.MachinePhase, terminationCondition *v1.NodeCondition) {
if phase == v1alpha1.MachineFailed { // if failed, terminated due to health
terminationCondition.Reason = NodeUnhealthy
terminationCondition.Message = "Machine Controller is terminating failed machine"
} else { // in all other cases (except for already terminating): assume scale down
terminationCondition.Reason = NodeScaledDown
terminationCondition.Message = "Machine Controller is scaling down machine"
}
}
Loading