Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Able to clean filesystem volume with job #863

Merged
merged 1 commit into from
Jul 20, 2018

Conversation

cofyc
Copy link
Contributor

@cofyc cofyc commented Jul 19, 2018

Fixes #737.

  • Add -delete-contents flag to delete contents in directory (use /scripts/fsclean.sh script now)
    • Reuse in-process delete code
  • Rename DeviceAnnotation ("device") to VolumePathAnnotation ("volume-path")
  • Create job to clean filesystem volume too when useJobForCleaning is true

Discussion:

  • Should we allow user to specify minimum capacity of volumes to clean by jobs? For examples, provisioner only starts cleaner jobs for volumes larger than 1T. For small volumes, in-process will be much quicker.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2018
@cofyc cofyc changed the title Able to clean filesystem volume with job. WIP: Able to clean filesystem volume with job. Jul 19, 2018
@cofyc cofyc changed the title WIP: Able to clean filesystem volume with job. Able to clean filesystem volume with job Jul 19, 2018
)

func main() {
rand.Seed(time.Now().UTC().UnixNano())
flag.StringVar(&optListenAddress, "listen-address", ":8080", "address on which to expose metrics")
flag.StringVar(&optMetricsPath, "metrics-path", "/metrics", "path under which to expose metrics")
flag.StringVar(&optDeleteContents, "delete-contents", "", "delete contents in directory")
Copy link
Contributor

Choose a reason for hiding this comment

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

It is kind of confusing to reuse the same process to do a different function. Can we create a new util binary for this? Or can the job just use a shell command to do the deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -46,8 +46,8 @@ const (
PVLabel = "pv"
// PVUuidLabel is the label name whose value is the pv uuid.
PVUuidLabel = "pvuuid"
// DeviceAnnotation is the annotation that specifies the device path.
DeviceAnnotation = "device"
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this could break any monitoring/logging built on top of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.
I updated it because the name is a bit confusing now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it is a little confusing now. Would it make sense to have a new annotation for the filesystem case?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 20, 2018
@msau42
Copy link
Contributor

msau42 commented Jul 20, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018
@wongma7 wongma7 merged commit 0baad4f into kubernetes-retired:master Jul 20, 2018
@cofyc cofyc deleted the fix737 branch October 31, 2018 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/local-volume cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants