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: add a built in garbage-collect policy to application #2575

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

yangsoon
Copy link
Collaborator

@yangsoon yangsoon commented Oct 28, 2021

Description of your changes

add a built in garbage-collect policy, while now only support one option keepLegacyResource, which means gc mechanism will keep the legacy resource when application is updated. you can enable the keepLegacyResource options like:

apiVersion: core.oam.dev/v1beta1
kind: Application
metadata:
  name: first-vela-app
spec:
  components:
    - name: express-server
      type: webservice
      properties:
        image: crccheck/hello-world
        port: 8000
      traits:
        - type: ingress-1-20
          properties:
            domain: testsvc.example.com
            http:
              "/": 8000
  policies:
    - name: keep-legacy-resource
      type: garbage-collect
      properties:
        keepLegacyResource: true

what will the gc mechanism do when enable the keepLegacyResource options

  1. resources with the same GVK and name will be normal updated, and the OwnerReference of those resources will refer to the new resourcesTracker.
  2. the expired resources and resourceTracker will be keep in cluster.
  3. you can delete all resource by delete the application

TODO

  • add more test

Fixes #

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

cc @Somefive @wangyikewxgm @captainroy-hy

@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #2575 (9cc9273) into master (8bcffb9) will decrease coverage by 0.04%.
The diff coverage is 47.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2575      +/-   ##
==========================================
- Coverage   59.86%   59.81%   -0.05%     
==========================================
  Files         223      223              
  Lines       22735    22824      +89     
==========================================
+ Hits        13610    13652      +42     
- Misses       7473     7515      +42     
- Partials     1652     1657       +5     
Flag Coverage Δ
apiserver-unittests 26.71% <0.00%> (-0.37%) ⬇️
core-unittests 54.61% <41.07%> (-0.03%) ⬇️
e2e-multicluster-test 23.30% <25.00%> (-0.17%) ⬇️
e2e-rollout-tests 31.05% <12.50%> (-0.07%) ⬇️
e2etests 36.23% <24.10%> (-0.34%) ⬇️

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

Impacted Files Coverage Δ
apis/core.oam.dev/v1alpha1/envbinding_types.go 0.00% <ø> (ø)
pkg/appfile/parser.go 63.52% <17.64%> (-3.59%) ⬇️
...dev/v1alpha2/application/application_controller.go 71.54% <28.57%> (-1.15%) ⬇️
...r/core.oam.dev/v1alpha2/application/dispatch/gc.go 66.37% <48.00%> (-14.51%) ⬇️
...troller/core.oam.dev/v1alpha2/application/apply.go 84.12% <53.84%> (-2.32%) ⬇️
pkg/appfile/appfile.go 75.41% <63.63%> (-0.65%) ⬇️
....oam.dev/v1alpha2/application/dispatch/dispatch.go 77.58% <100.00%> (+0.26%) ⬆️
...ller/core.oam.dev/v1alpha2/application/revision.go 73.03% <100.00%> (+0.37%) ⬆️
...tepdefinition/workflowstepdefinition_controller.go 63.82% <0.00%> (-5.32%) ⬇️
pkg/utils/apply/apply.go 84.69% <0.00%> (-2.05%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bcffb9...9cc9273. Read the comment docs.

@yangsoon yangsoon force-pushed the add-gc-policy branch 2 times, most recently from b1a4c97 to 33c3756 Compare October 28, 2021 12:49
@yangsoon yangsoon changed the title Feat: add a built in garbage-collect policy to application [WIP]Feat: add a built in garbage-collect policy to application Oct 28, 2021
@yangsoon yangsoon changed the title [WIP]Feat: add a built in garbage-collect policy to application Feat: add a built in garbage-collect policy to application Oct 29, 2021
@yangsoon
Copy link
Collaborator Author

cc @leejanee

@yangsoon yangsoon force-pushed the add-gc-policy branch 4 times, most recently from 6c03303 to a50a7b9 Compare November 1, 2021 02:09
@hongchaodeng
Copy link
Member

What's the use case? Why keep those legacy resource?

@yangsoon yangsoon force-pushed the add-gc-policy branch 5 times, most recently from 859e9b0 to 995ac57 Compare November 3, 2021 06:34
Copy link
Collaborator

@wangyikewxgm wangyikewxgm left a comment

Choose a reason for hiding this comment

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

是否可以清理appRev的时候,清理对应对应的resourceTracker

@yangsoon
Copy link
Collaborator Author

yangsoon commented Nov 4, 2021

是否可以清理appRev的时候,清理对应对应的resourceTracker

清理resourceTracker会导致历史资源被删除,后面会提供接口让用户主动去删

@wonderflow
Copy link
Collaborator

Need to write examples(docs) for this policy and usage. The docs don't need to show in our website, but contributors should be able to understand it

@yangsoon
Copy link
Collaborator Author

yangsoon commented Nov 7, 2021

garbage-collect policy usages doc.

@yangsoon yangsoon force-pushed the add-gc-policy branch 3 times, most recently from 264364f to 81d2e4f Compare November 15, 2021 11:24
Copy link
Collaborator

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

please fix CI

@yangsoon yangsoon force-pushed the add-gc-policy branch 3 times, most recently from b10d06a to 213d699 Compare November 19, 2021 02:05
Copy link
Member

@leejanee leejanee left a comment

Choose a reason for hiding this comment

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

please add scenes-oriented test cases.

@yangsoon
Copy link
Collaborator Author

@yangsoon
Copy link
Collaborator Author

yangsoon commented Nov 19, 2021

The test coverage results seem to be incorrect, I am sure that the critical path has corresponding tests.
please cc @chivalryq

@yangsoon yangsoon force-pushed the add-gc-policy branch 3 times, most recently from b813dd6 to 60d9669 Compare November 23, 2021 06:05
@wonderflow wonderflow merged commit 3d2fcac into kubevela:master Nov 24, 2021
@yangsoon yangsoon deleted the add-gc-policy branch November 24, 2021 07:38
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.

6 participants