implement non-graceful shutdown #2702

YZ775 · 2024-10-24T07:39:54Z

test is implemented in neco-apps
https://github.com/cybozu-private/neco-apps/pull/5795

https://github.com/cybozu-private/neco-apps/actions/workflows/dctest-with-neco-feature-branch.yaml

masa213f · 2025-02-14T07:14:19Z

etc/cke-template.yml

@@ -42,11 +42,15 @@ repair:
        command_retries: 2
        command_interval: 30
        watch_seconds: 600
+      - repair_command: ["sh", "-c", "neco non-graceful-node-shutdown shutdown $1", "non-graceful-node-shutdown"]


In this case, we need not use sh -c.
neco non-graceful-node-shutdown shutdon/cleanup command will work just add the IP Address to the end of the command.

Suggested change

- repair_command: ["sh", "-c", "neco non-graceful-node-shutdown shutdown $1", "non-graceful-node-shutdown"]

- repair_command: ["neco", "non-graceful-node-shutdown", "shutdown"]

masa213f · 2025-02-14T07:16:33Z

pkg/neco/cmd/non_graceful_shutdown.go

+	Short: "non-Graceful Node Shutdown related commands",
+	Long:  `non-Graceful Node Shutdown related commands.`,


Why not use capital letters?

Suggested change

Short: "non-Graceful Node Shutdown related commands",

Long: `non-Graceful Node Shutdown related commands.`,

Short: "Non-Graceful Node Shutdown related commands",

Long: `Non-Graceful Node Shutdown related commands.`,

masa213f · 2025-02-14T07:28:48Z

pkg/neco/cmd/non_graceful_shutdown_shutdown.go

+		powerCheckCmd := exec.Command("neco", "power", "status", node)
+		var out bytes.Buffer
+		powerCheckCmd.Stdout = &out
+		err = powerCheckCmd.Run()


In this case, Cmd.Output() is enough.

Suggested change

powerCheckCmd := exec.Command("neco", "power", "status", node)

var out bytes.Buffer

powerCheckCmd.Stdout = &out

err = powerCheckCmd.Run()

out, err := exec.Command("neco", "power", "status", node).Output()

https://pkg.go.dev/os/exec#Cmd.Output

masa213f · 2025-02-14T07:42:16Z

pkg/neco/cmd/non_graceful_shutdown.go

+
+type CephCluster struct {
+	Name      string
+	NameSpace string


In the context of Kubernetes, Namespace is a proper noun.
https://kubernetes.io/docs/reference/glossary/?fundamental=true#term-namespace

Suggested change

NameSpace string

Namespace string

masa213f · 2025-02-14T08:25:22Z

pkg/neco/cmd/non_graceful_shutdown_cleanup.go

+		if err != nil {
+			return err
+		}
+		sabakanStatus := machines[0].Status.State


I'm not confident, so please tell me.
Is it necessary to check the sabakan status before unfencing?

sabakanStatus is used in L90.
In the end of cleanup, we need to untaint the node if repair of the machine is succeeded and become healthy.

masa213f · 2025-02-17T04:27:12Z

pkg/neco/cmd/non_graceful_shutdown_shutdown.go

+			poweroffCmd := exec.Command("neco", "power", "stop", node)
+			err = poweroffCmd.Run()


IMO, it's better to run command directly. In this case, it's enough.

Suggested change

poweroffCmd := exec.Command("neco", "power", "stop", node)

err = poweroffCmd.Run()

err := exec.Command("neco", "power", "stop", node).Run()

masa213f · 2025-02-17T04:37:35Z

pkg/neco/cmd/non_graceful_shutdown_cleanup.go

+			kubernetesNode := &corev1.Node{}
+			err = kubeClient.Get(ctx, client.ObjectKey{Name: node}, kubernetesNode)


This will be an error with the spare machines, won't it?

masa213f · 2025-02-17T04:41:52Z

pkg/neco/cmd/non_graceful_shutdown_shutdown.go

+				}
+				err = kubeClient.Create(ctx, &networkFence)
+				if err != nil {
+					if client.IgnoreAlreadyExists(err) == nil {


IMO, in this case IsAlreadyExists is readable.

Suggested change

if client.IgnoreAlreadyExists(err) == nil {

if apierrors.IsAlreadyExists(err) {

masa213f · 2025-02-17T04:46:08Z

pkg/neco/cmd/non_graceful_shutdown_shutdown.go

+			return err
+		}
+
+		// Add taint to the node


What happens if a command is executed for spare machines?

I forgotten to implement the case if the target is spare machine.
There are no need to do non-graceful shutdown against non-k8s node, so I'll fix the command to skip procedures.

masa213f · 2025-02-17T05:02:44Z

pkg/neco/cmd/non_graceful_shutdown.go

+	Long:  `non-Graceful Node Shutdown related commands.`,
+}
+
+type CephCluster struct {


This is fine. But, IMO, NamespacedName is enough.
https://pkg.go.dev/k8s.io/apimachinery/pkg/types#NamespacedName

YZ775 · 2025-02-17T06:46:49Z

I'm checking that the fix works fine in CI now.
https://github.com/cybozu-private/neco-apps/actions/runs/13364264101

YZ775 · 2025-02-20T07:14:34Z

CI
https://github.com/cybozu-private/neco-apps/actions/runs/13429721361

YZ775 · 2025-02-25T06:55:35Z

CI was passed.
https://github.com/cybozu-private/neco-apps/actions/runs/13513549671/job/37758134998
@masa213f
This PR is ready for review. Sorry for late.

YZ775 force-pushed the non-graceful-shutdown branch 3 times, most recently from 5b4e8b8 to eb0c558 Compare October 28, 2024 00:56

YZ775 force-pushed the non-graceful-shutdown branch 3 times, most recently from b173db3 to 3768397 Compare November 7, 2024 04:52

YZ775 force-pushed the non-graceful-shutdown branch from 69b7b44 to 800de3e Compare November 12, 2024 05:06

YZ775 force-pushed the non-graceful-shutdown branch 2 times, most recently from 806283a to bc98450 Compare November 26, 2024 01:51

yokaze assigned YZ775 Dec 4, 2024

YZ775 force-pushed the non-graceful-shutdown branch from 507deec to e405bd0 Compare December 23, 2024 04:08

YZ775 force-pushed the non-graceful-shutdown branch 5 times, most recently from 7b740ab to 151d645 Compare January 16, 2025 01:53

YZ775 force-pushed the non-graceful-shutdown branch 7 times, most recently from a5ee9a1 to 0cd28be Compare February 3, 2025 02:17

YZ775 force-pushed the non-graceful-shutdown branch 3 times, most recently from f0e57db to 42b10c7 Compare February 6, 2025 08:15

YZ775 force-pushed the non-graceful-shutdown branch 2 times, most recently from b30cabb to 801f1dc Compare February 13, 2025 07:32

YZ775 marked this pull request as ready for review February 13, 2025 07:48

YZ775 force-pushed the non-graceful-shutdown branch from 801f1dc to 93c94be Compare February 13, 2025 07:56

YZ775 force-pushed the non-graceful-shutdown branch from 93c94be to 8c1c789 Compare February 13, 2025 08:14

YZ775 requested a review from masa213f February 14, 2025 04:43

masa213f requested changes Feb 17, 2025

View reviewed changes

YZ775 force-pushed the non-graceful-shutdown branch 2 times, most recently from ef648bc to 0069a33 Compare February 17, 2025 07:36

YZ775 requested a review from masa213f February 17, 2025 07:57

YZ775 force-pushed the non-graceful-shutdown branch 3 times, most recently from 2eb352d to e5cc3f6 Compare February 20, 2025 07:01

YZ775 force-pushed the non-graceful-shutdown branch from e5cc3f6 to 7b00171 Compare February 20, 2025 07:29

YZ775 added 4 commits February 21, 2025 01:52

implement non-graceful shutdown

365c2b0

fix

df37e9a

fix for review

8c458dd

fix for review

d80e821

YZ775 force-pushed the non-graceful-shutdown branch from 7b00171 to d80e821 Compare February 21, 2025 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement non-graceful shutdown #2702

implement non-graceful shutdown #2702

YZ775 commented Oct 24, 2024 •

edited

Loading

masa213f Feb 14, 2025

masa213f Feb 14, 2025

masa213f Feb 14, 2025

masa213f Feb 14, 2025

masa213f Feb 14, 2025

YZ775 Feb 17, 2025

masa213f Feb 17, 2025

masa213f Feb 17, 2025

masa213f Feb 17, 2025

masa213f Feb 17, 2025

YZ775 Feb 17, 2025

masa213f Feb 17, 2025

YZ775 commented Feb 17, 2025

YZ775 commented Feb 20, 2025

YZ775 commented Feb 25, 2025

	- repair_command: ["sh", "-c", "neco non-graceful-node-shutdown shutdown $1", "non-graceful-node-shutdown"]
	- repair_command: ["neco", "non-graceful-node-shutdown", "shutdown"]

		Short: "non-Graceful Node Shutdown related commands",
		Long: `non-Graceful Node Shutdown related commands.`,

		poweroffCmd := exec.Command("neco", "power", "stop", node)
		err = poweroffCmd.Run()

	poweroffCmd := exec.Command("neco", "power", "stop", node)
	err = poweroffCmd.Run()
	err := exec.Command("neco", "power", "stop", node).Run()

		kubernetesNode := &corev1.Node{}
		err = kubeClient.Get(ctx, client.ObjectKey{Name: node}, kubernetesNode)

	if client.IgnoreAlreadyExists(err) == nil {
	if apierrors.IsAlreadyExists(err) {

implement non-graceful shutdown #2702

Are you sure you want to change the base?

implement non-graceful shutdown #2702

Conversation

YZ775 commented Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YZ775 commented Feb 17, 2025

YZ775 commented Feb 20, 2025

YZ775 commented Feb 25, 2025

YZ775 commented Oct 24, 2024 •

edited

Loading