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: update PodDecoration API design #122

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

Eikykun
Copy link
Member

@Eikykun Eikykun commented Nov 21, 2023

1. Does this PR affect any open issues?(Y/N) and add issue references:

  • N
  • Y

2. What is the scope of this PR (e.g. component or file name):

PodDecoration Proposal

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Copy link

github-actions bot commented Nov 21, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (412b4f2) 46.17% compared to head (3ad33e4) 59.58%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #122       +/-   ##
===========================================
+ Coverage   46.17%   59.58%   +13.40%     
===========================================
  Files          45       46        +1     
  Lines        3558     3489       -69     
===========================================
+ Hits         1643     2079      +436     
+ Misses       1700     1190      -510     
- Partials      215      220        +5     
Flag Coverage Δ
unittests 59.58% <ø> (+13.40%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Eikykun
Copy link
Member Author

Eikykun commented Nov 21, 2023

Oth proposal,

  1. Should PD select pods by collaset? such as
  selector:               # Select the Pods to inject config.
    matchCollaSet:
    - collaSet-a
    - collaSet-b
    matchLabels:          # Select a group of Pods by labelSelector.
      ...
    matchExpressions:     # Or select specific Pods by matchExpressions.
      ...
  1. Support RollingUpdate by CollaSet partition?
  updateStrategy:
    rollingUpdate: 
      collaSet:
      - name: collaSet-a
         partition: 2 

cc @wu8685 @zoumo @shaofan-hs

- name: init
image: init-docker:0.1.0
primaryContainers: # Overwrite old container
- targetPolicy: ByName # ByName | All | First | Last, default by name
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to use All as the default value. There usually is only one primary container.

@wu8685
Copy link
Collaborator

wu8685 commented Nov 21, 2023

Oth proposal,

  1. Should PD select pods by collaset? such as
  selector:               # Select the Pods to inject config.
    matchCollaSet:
    - collaSet-a
    - collaSet-b
    matchLabels:          # Select a group of Pods by labelSelector.
      ...
    matchExpressions:     # Or select specific Pods by matchExpressions.
      ...
  1. Support RollingUpdate by CollaSet partition?
  updateStrategy:
    rollingUpdate: 
      collaSet:
      - name: collaSet-a
         partition: 2 

cc @wu8685 @zoumo @shaofan-hs

I think PD users need not to provide the CollaSet name. Telling PD which pods to inject could be enough.

About the item 2, I can't get your meaning. Why does PD need to know the CollaSet partition?

@Eikykun
Copy link
Member Author

Eikykun commented Nov 21, 2023

I think PD users need not to provide the CollaSet name. Telling PD which pods to inject could be enough.

About the item 2, I can't get your meaning. Why does PD need to know the CollaSet partition?

If PD selects Pods under multiple CollaSet, can I still use updateStrategy.rollingUpdate.partition to control the upgrade?
If so, how do I decide the pods under multiple CollaSets corresponding to the partition?

@Eikykun Eikykun self-assigned this Nov 21, 2023
@Eikykun Eikykun added the kind/enhancement New feature or request label Nov 21, 2023
@wu8685
Copy link
Collaborator

wu8685 commented Nov 22, 2023

I think PD users need not to provide the CollaSet name. Telling PD which pods to inject could be enough.
About the item 2, I can't get your meaning. Why does PD need to know the CollaSet partition?

If PD selects Pods under multiple CollaSet, can I still use updateStrategy.rollingUpdate.partition to control the upgrade? If so, how do I decide the pods under multiple CollaSets corresponding to the partition?

How about sharing the same data between CollaSet and PD controller? PD controller decides the Pod partition, and store this info into the shared memory. Then CollaSet controller is able to know the Pod partition info under each CollaSet from the shared memory.

Take the reference from this part of the doc

cons: CollaSet controller can not run separately from PD controller

Copy link
Collaborator

@wu8685 wu8685 left a comment

Choose a reason for hiding this comment

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

lgtm

@wu8685 wu8685 merged commit 29ff994 into KusionStack:main Dec 5, 2023
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants