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

Feat: support custom cascading gc policy #6059

Merged

Conversation

Somefive
Copy link
Collaborator

@Somefive Somefive commented May 31, 2023

Description of your changes

The default behaviour of deleting Job in Kubernetes will orphan the underlying pods. The initial design is to ensure the logs of pods can be reserved. But there are cases that KubeVela users do not want to orphan pods and want to delete them when Job or upper layer application is recycled.

This PR adds a "cascading" fields in the "garbage-collect" policy, which allows user to customize the deletion behaviour for selected resources. If not set, KubeVela controller will use the native Kubernetes behaviour for deletion.

Example:

apiVersion: core.oam.dev/v1beta1
kind: Application
metadata:
  name: gc-propagation
spec:
  components:
    - type: k8s-objects
      name: cascading-gc
      properties:
        objects:
          - apiVersion: batch/v1
            kind: Job
            spec:
              template:
                spec:
                  containers:
                    - name: pi
                      image: perl:5.34.0
                      command: ["perl",  "-Mbignum=bpi", "-wle", "print bpi(2000)"]
                  restartPolicy: Never
              backoffLimit: 4
  policies:
    - type: garbage-collect
      properties:
        rules:
          - selector:
              componentNames: ["cascading-gc"]
            propagation: cascading

In the above example, the job will recycle the underlying pods when it is deleted. If the policy is not configured, the pods will be left even if the job and the app is deleted.

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Special notes for your reviewer

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: +3.87 🎉

Comparison is base (1c0f2c4) 57.16% compared to head (3541d64) 61.03%.

❗ Current head 3541d64 differs from pull request most recent head 07471ef. Consider uploading reports for the commit 07471ef to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6059      +/-   ##
==========================================
+ Coverage   57.16%   61.03%   +3.87%     
==========================================
  Files         220      224       +4     
  Lines       31136    31278     +142     
==========================================
+ Hits        17800    19092    +1292     
+ Misses      11695    10422    -1273     
- Partials     1641     1764     +123     
Flag Coverage Δ
core-unittests 56.24% <20.00%> (-0.01%) ⬇️
e2e-multicluster-test 24.94% <80.00%> (?)
e2etests 25.04% <80.00%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/resourcekeeper/gc.go 78.77% <80.00%> (+3.06%) ⬆️

... and 58 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -80,12 +80,6 @@ jobs:
go install github.com/onsi/ginkgo/v2/ginkgo
go get github.com/onsi/gomega/...

- name: Setup KinD
Copy link
Member

Choose a reason for hiding this comment

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

Why are we deleting this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we have two "Setup KinD" in e2e-test.

// GarbageCollectPropagationOrphan orphan child resources while deleting target resources
GarbageCollectPropagationOrphan = "orphan"
// GarbageCollectPropagationBackground delete child resources in background while deleting target resources
GarbageCollectPropagationBackground = "background"
Copy link
Member

Choose a reason for hiding this comment

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

How about cascading? I think backgroud is kinda confusing since we don't allow foreground GC option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, "foreground" is okay for client-go but we should not allow it here. It will block the controller and should only be a CLI case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think about "cascading" as a good choice because that's how kubectl use for flags. But I later confirmed from the bare API parameter and see the PropagationPolicy used for DELETE api.
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great, I renamed the value of "background" to "cascading".

@Somefive Somefive force-pushed the feat/support-custom-propagation branch from 3541d64 to 68cc8f3 Compare May 31, 2023 09:50
@Somefive Somefive changed the title Feat: support custom propagation gc policy Feat: support custom cascading gc policy May 31, 2023
@Somefive Somefive force-pushed the feat/support-custom-propagation branch from 68cc8f3 to 3541d64 Compare May 31, 2023 09:52
Signed-off-by: Somefive <yd219913@alibaba-inc.com>
@Somefive Somefive force-pushed the feat/support-custom-propagation branch from 3541d64 to 07471ef Compare May 31, 2023 10:02
@@ -12,6 +12,8 @@ template: {
selector: #ResourcePolicyRuleSelector
// +usage=Specify the strategy for target resource to recycle
strategy: *"onAppUpdate" | "onAppDelete" | "never"
// +usage=Specify the deletion propagation strategy for target resource to delete
propagation?: "orphan" | "cascading"
Copy link
Member

Choose a reason for hiding this comment

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

Which propagation will be used by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default case is having no propagation settings. For Kubernetes, when the propagation setting is absent, it will determine the propagation by resources. For example, for deployments, it will be "cascading" (or in other word "background"), but for jobs, it will be "orphan". It works like "auto".

@chivalryq chivalryq merged commit 1a6dafa into kubevela:master Jun 1, 2023
23 checks passed
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.

None yet

3 participants