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

Update generic node removal documentation to schedule a drain when removing worker nodes #324

Merged
merged 2 commits into from
May 7, 2024

Conversation

HappyTetrahedron
Copy link
Contributor

@HappyTetrahedron HappyTetrahedron commented Apr 30, 2024

No description provided.

@HappyTetrahedron HappyTetrahedron requested a review from a team May 2, 2024 15:14
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

The current changes generate a lot of unnecessary content for the storage node how-tos. We never need to schedule a storage node drain for the next maintenance, so there's no point in including all that text in those how-tos.

Comment on lines 23 to 25
* First, we identify the correct node to remove and drain it.
* Then, we remove it from Kubernetes.
* Finally, we remove the associated VMs.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the commas here are correct in English, but I always trip over comma rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally you don't use commas after "first", "second" etc. but in the context of a list of things you use them anyway. The exception to the rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though I guess since this is a bullet list you could consider each bullet point a standalone sentence and skip the comma... Huh. Guess it's a special case.
(The comma would be needed if all these sentences came one-after-the-other in the same paragraph)

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess since this is a bullet list you could consider each bullet point a standalone sentence

I guess that's what confused me when reviewing. I don't have hard feelings either way, but I'm still mentally stumbling over the commas in the bullet list.

@HappyTetrahedron HappyTetrahedron requested a review from simu May 7, 2024 13:48
@simu simu changed the title Add documentation for scheduling a node drain when messing with nodes… Update generic node removal documentation to schedule a drain when removing worker nodes May 7, 2024
Comment on lines 65 to 77
+
ifeval::["{cloud_provider}" == "cloudscale"]
ifeval::["{delete-node-type}" == "storage"]
[TIP]
====
On cloudscale.ch, we configure Rook Ceph to setup the OSDs in "portable" mode.
This configuration enables OSDs to be scheduled on any storage node.

With this configuration, we don't have to migrate OSDs hosted on the old node(s) manually.
Instead, draining a node will cause any OSDs hosted on that node to be rescheduled on other storage nodes.
====
endif::[]
endif::[]
Copy link
Member

Choose a reason for hiding this comment

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

This tip isn't needed for the scheduling case

Suggested change
+
ifeval::["{cloud_provider}" == "cloudscale"]
ifeval::["{delete-node-type}" == "storage"]
[TIP]
====
On cloudscale.ch, we configure Rook Ceph to setup the OSDs in "portable" mode.
This configuration enables OSDs to be scheduled on any storage node.
With this configuration, we don't have to migrate OSDs hosted on the old node(s) manually.
Instead, draining a node will cause any OSDs hosted on that node to be rescheduled on other storage nodes.
====
endif::[]
endif::[]

Comment on lines 23 to 25
* First, we identify the correct node to remove and drain it.
* Then, we remove it from Kubernetes.
* Finally, we remove the associated VMs.
Copy link
Member

Choose a reason for hiding this comment

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

Though I guess since this is a bullet list you could consider each bullet point a standalone sentence

I guess that's what confused me when reviewing. I don't have hard feelings either way, but I'm still mentally stumbling over the commas in the bullet list.

@HappyTetrahedron HappyTetrahedron merged commit 72cd14f into master May 7, 2024
1 check passed
@HappyTetrahedron HappyTetrahedron deleted the schedule-drain branch May 7, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants