Skip to content

Add documentation and release image for cleanup controller - #391

Merged
k8s-ci-robot merged 5 commits into
kubernetes-sigs:masterfrom
justinblalock87:cleanup-docs
Jul 21, 2023
Merged

Add documentation and release image for cleanup controller#391
k8s-ci-robot merged 5 commits into
kubernetes-sigs:masterfrom
justinblalock87:cleanup-docs

Conversation

@justinblalock87

@justinblalock87 justinblalock87 commented Jul 19, 2023 •
edited by mauriciopoppe
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind documentation

What this PR does / why we need it:
Adds an example deployment and RBAC for the cleanup controller. This also adds the controller image to be pushed on release.

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 19, 2023
@k8s-ci-robot

k8s-ci-robot commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

Hi @justinblalock87. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 19, 2023
@msau42

msau42 commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2023
msau42
msau42 reviewed Jul 19, 2023
View reviewed changes
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread examples/cleanup-controller/README.md
## Overview

Pods using a Local Persistent Volume are always scheduled to the same Node as the Local PV it uses (as opposed to if they were using a HostPath volume, for instance). When Nodes fail while having a Local PV attached to them, Pods using the Local PV become stuck since they can't be scheduled to a deleted Node. In other words, the Local PV and its corresponding PVC aren’t cleaned up when a Node becomes unavailable and so each of them (PV/PVC/Pods) becomes stuck. This results in a degraded or unavailable workload. This controller aims to clean up the stale Local PVs and their bound PVCs after Node deletion, allowing workloads to automatically recover.

@msau42 msau42 Jul 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's important to add the requirements that someone should consider before running this controller, such
as:

  1. When the K8s Node is deleted, that actually means the underlying node and local PV data are already gone and irrecoverable. This is common in cloud environments, but not as common on-prem. This controller does not try to actively scrub data from the disks.

  2. Some other controller, such as StatefulSet, needs to be able to recreate the PVC when it is deleted.

@justinblalock87 justinblalock87 Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added these considerations.

Comment thread examples/cleanup-controller/README.md Outdated
Comment thread examples/cleanup-controller/README.md Outdated
Comment thread examples/cleanup-controller/deployment.yaml Outdated
Comment thread examples/cleanup-controller/deployment.yaml
Comment thread examples/cleanup-controller/README.md Outdated
msau42
msau42 reviewed Jul 19, 2023
View reviewed changes
Comment thread Makefile
@@ -24,6 +24,7 @@ WINDOWS_DISTROS ?=
DOCKER=DOCKER_CLI_EXPERIMENTAL=enabled docker

@msau42 msau42 Jul 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This script also needs to be enhanced to support publishing multiple images:
https://github.com/kubernetes-sigs/sig-storage-local-static-provisioner/blob/master/hack/release.sh#L110

@justinblalock87 justinblalock87 Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like this script would already publish multiple images since it ultimately calls make cross

sig-storage-local-static-provisioner/hack/release.sh

Line 205 in 6790dc1

make cross \
and I've added the cleanup-controller image to the make cross command in the Makefile. The rest of the release.sh script determines the version number and creates a multi arch manifest for the local provisioner. I would assume the cleanup controller would just use the version of the local provisioner, and I'm wondering if a multi arch manifest is needed for this?

@msau42 msau42 Jul 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The script appears to be doing some checks before "make cross", which may need to be extended to support multiple images:

https://github.com/kubernetes-sigs/sig-storage-local-static-provisioner/blob/master/hack/release.sh#L110

@justinblalock87 justinblalock87 Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added.

@justinblalock87
justinblalock87 requested a review from msau42 July 19, 2023 23:40
msau42
msau42 reviewed Jul 20, 2023
View reviewed changes
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread deployment/kubernetes/example/cleanup-controller/deployment.yaml
Comment thread deployment/kubernetes/example/cleanup-controller/deployment.yaml Outdated
Comment thread deployment/kubernetes/example/cleanup-controller/deployment.yaml Outdated
Comment thread deployment/kubernetes/example/cleanup-controller/rbac.yaml Outdated
Comment thread docs/cleanup-controller.md Outdated
@mauriciopoppe

mauriciopoppe commented Jul 20, 2023

Copy link
Copy Markdown
Member

/release-note-none

@justinblalock87
justinblalock87 force-pushed the cleanup-docs branch 2 times, most recently from acbd3c5 to 7c6f4f0 Compare July 20, 2023 22:13
@justinblalock87
justinblalock87 requested a review from msau42 July 20, 2023 22:16
msau42
msau42 reviewed Jul 20, 2023
View reviewed changes
Comment thread docs/node-cleanup-controller.md
@msau42

msau42 commented Jul 20, 2023

Copy link
Copy Markdown
Contributor

/approve

One minor comment and we should be good to go!

@k8s-ci-robot

k8s-ci-robot commented Jul 20, 2023

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinblalock87, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2023
@justinblalock87
justinblalock87 force-pushed the cleanup-docs branch 2 times, most recently from 8b55e29 to 3a92ef7 Compare July 21, 2023 16:14
@msau42

msau42 commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2023
@justinblalock87

justinblalock87 commented Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

/lgtm

Updated a grammatical error

@justinblalock87
justinblalock87 requested a review from msau42 July 21, 2023 16:23
@msau42

msau42 commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2023
@k8s-ci-robot
k8s-ci-robot merged commit d3588b7 into kubernetes-sigs:master Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mauriciopoppe mauriciopoppe Awaiting requested review from mauriciopoppe
@msau42 msau42 Awaiting requested review from msau42

Assignees

@msau42 msau42

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@justinblalock87 @k8s-ci-robot @msau42 @mauriciopoppe