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.

Sign up for GitHub

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

change CKE to proceed rebooting immediately after draining of node is completed #707

Merged
merged 6 commits into from
May 7, 2024
Merged

change CKE to proceed rebooting immediately after draining of node is completed #707

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f48336
change CKE to proceed rebooting immediately after draining of node i…
YZ775 Mar 11, 2024
5798f89
change priority of RebootRebootOp
YZ775 Apr 18, 2024
639d297
add cancel check in drainBackOff
YZ775 Apr 19, 2024
609f6aa
change priority of RebootDequeueOp
YZ775 Apr 23, 2024
7eab07c
fixup! change priority of RebootDequeueOp
YZ775 Apr 23, 2024
24224de
delete unnecessary return value phaseReboot
YZ775 Apr 26, 2024
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
7 changes: 7 additions & 0 deletions op/reboot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,13 @@ func drainBackOff(ctx context.Context, inf cke.Infrastructure, entry *cke.Reboot
"name": entry.Node,
log.FnError: err,
})
etcdEntry, err := inf.Storage().GetRebootsEntry(ctx, entry.Index)
if err != nil {
return err
}
if etcdEntry.Status == cke.RebootStatusCancelled {
return nil
}
entry.Status = cke.RebootStatusQueued
entry.LastTransitionTime = time.Now().Truncate(time.Second).UTC()
entry.DrainBackOffCount++
Expand Down
8 changes: 3 additions & 5 deletions op/reboot_decide.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,10 @@ func ChooseDrainedNodes(c *cke.Cluster, apiServers map[string]bool, rqEntries []
return nil
}
}
if len(workerInProgress) >= maxConcurrentReboots {
return nil
} else if len(workerInProgress)+len(workerDrainable) <= maxConcurrentReboots {
return workerDrainable
if len(workerInProgress) < maxConcurrentReboots && len(workerDrainable) > 0 {
return workerDrainable[:1]
} else {
return workerDrainable[:maxConcurrentReboots-len(workerInProgress)]
return nil
}
}

Expand Down
22 changes: 10 additions & 12 deletions server/strategy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,12 +885,19 @@ func rebootOps(c *cke.Cluster, constraints *cke.Constraints, rebootArgs DecideOp
}

if len(rebootArgs.RebootCancelled) > 0 {
phaseReboot = true
ops = append(ops, op.RebootCancelOp(rebootArgs.RebootCancelled))
return ops, phaseReboot
}
if len(rebootArgs.RebootDequeued) > 0 {
ops = append(ops, op.RebootDequeueOp(rebootArgs.RebootDequeued))
}
if len(ops) > 0 {
return ops, true
}

if len(rebootArgs.DrainCompleted) > 0 {
ops = append(ops, op.RebootRebootOp(nf.HealthyAPIServer(), rebootArgs.DrainCompleted, &c.Reboot))
}
if len(rebootArgs.NewlyDrained) > 0 {
phaseReboot = true
sshCheckNodes := make([]*cke.Node, 0, len(nf.cluster.Nodes))
for _, node := range nf.cluster.Nodes {
if !rebootProcessing(rebootArgs.RQEntries, node.Address) {
Expand All @@ -903,18 +910,9 @@ func rebootOps(c *cke.Cluster, constraints *cke.Constraints, rebootArgs DecideOp
ops = append(ops, op.RebootDrainStartOp(nf.HealthyAPIServer(), rebootArgs.NewlyDrained, &c.Reboot))
}
}
if len(rebootArgs.DrainCompleted) > 0 {
phaseReboot = true
ops = append(ops, op.RebootRebootOp(nf.HealthyAPIServer(), rebootArgs.DrainCompleted, &c.Reboot))
}
if len(rebootArgs.DrainTimedout) > 0 {
phaseReboot = true
ops = append(ops, op.RebootDrainTimeoutOp(rebootArgs.DrainTimedout))
}
if len(rebootArgs.RebootDequeued) > 0 {
phaseReboot = true
ops = append(ops, op.RebootDequeueOp(rebootArgs.RebootDequeued))
}
if len(ops) > 0 {
phaseReboot = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for fixing phaseReboot!

I checked this variable carefully, and I was convinced that this function need not return phaseReboot.
This function should only determine ops from the given information.

The caller(DecideOps()) should determine the phase. And which can be determined only by the returned ops.

Could you change the return value of this function and change the caller as following?
It's okay to change it in another PR.

https://github.com/cybozu-go/cke/blob/v1.28.0/server/strategy.go#L106-L113

	// 11. Reboot nodes if reboot request has been arrived to the reboot queue, and the number of unreachable nodes is less than a threshold.
	if ops := rebootOps(c, constraints, rebootArgs, nf); len(ops) > 0 {
		if !nf.EtcdIsGood() {
			log.Warn("cannot reboot nodes because etcd cluster is not responding and in-sync", nil)
			return nil, cke.PhaseRebootNodes
		}
		return ops, cke.PhaseRebootNodes
	}

Expand Down