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: use StatefulSet to manage meta Pods #312

Merged
merged 2 commits into from
Jan 18, 2023
Merged

feat: use StatefulSet to manage meta Pods #312

merged 2 commits into from
Jan 18, 2023

Conversation

arkbriar
Copy link
Collaborator

@arkbriar arkbriar commented Jan 10, 2023

Signed-off-by: arkbriar [email protected]

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Use StatefulSet instead of Deployment to manage the meta Pods. For reasons for this change, please refer to feat: start meta node with statefulset #311.
  • I have deleted the tests for the meta workload objects. It's not expected, but adding tests in the current framework is hard. I will try to optimize the test cases later.
  • This is a breaking change. When upgrading from the lower versions, a user must execute the following command to make the operator work again (but be noticed the commands have side effects of terminating the meta nodes).
kubectl delete deploy -l risingwave/component=meta -A

Checklist

  • I have written the necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

close #311
relates to #313

@codecov
Copy link

codecov bot commented Jan 10, 2023

Codecov Report

Merging #312 (3f5a0f4) into main (c868be4) will decrease coverage by 0.85%.
The diff coverage is 45.00%.

@@            Coverage Diff             @@
##             main     #312      +/-   ##
==========================================
- Coverage   76.46%   75.61%   -0.85%     
==========================================
  Files          52       52              
  Lines        5022     5028       +6     
==========================================
- Hits         3840     3802      -38     
- Misses       1115     1160      +45     
+ Partials       67       66       -1     
Flag Coverage Δ
unittests 75.61% <45.00%> (-0.85%) ⬇️

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

Impacted Files Coverage Δ
pkg/factory/risingwave_object_factory.go 90.47% <36.84%> (-3.20%) ⬇️
pkg/manager/risingwave_controller_manager_impl.go 41.29% <41.17%> (ø)
pkg/controller/risingwave_controller.go 65.64% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@shanicky shanicky left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you very much!
But we may need to add -endpoint=$(POD_NAME) to argsForMeta after the PR risingwavelabs/risingwave#7179 merged

@arkbriar
Copy link
Collaborator Author

But we may need to add -endpoint=$(POD_NAME) to argsForMeta after the PR risingwavelabs/risingwave#7179 merged

Let's update the operator, then.

@arkbriar arkbriar requested a review from CAJan93 January 11, 2023 08:38
PodManagementPolicy: appsv1.ParallelPodManagement,
PersistentVolumeClaimRetentionPolicy: &kruiseappsv1beta1.StatefulSetPersistentVolumeClaimRetentionPolicy{
WhenDeleted: kruiseappsv1beta1.DeletePersistentVolumeClaimRetentionPolicyType,
WhenScaled: kruiseappsv1beta1.DeletePersistentVolumeClaimRetentionPolicyType,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the consequence of this? Will deleting the PVC Retention Policy Type also trigger a deletion of the PV? If so, do we want that? Does this mean that all pods loose their data if we scale the stateful set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, it means

  • when deleting the StatefulSet, all PVCs from the PVC template will be deleted
  • when scaling down the Pod of the StatefulSet, the corresponding PVC will also be deleted (and then the cloud controller should delete the PV as well)

You can check the details here.

For meta nodes, AFAIK, we won't have PVCs. Even if we have, that's for cache purposes and could be safely deleted.

Copy link
Contributor

@CAJan93 CAJan93 left a comment

Choose a reason for hiding this comment

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

LGTM

@arkbriar arkbriar added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jan 18, 2023
@arkbriar
Copy link
Collaborator Author

Since the test coverage drops, the mergify won't merge it. Will merge it manually. The improvement of the test coverage is planned in #313.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change It's a breaking change feature mergify/can-merge Indicates that the PR can be added to the merge queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: start meta node with statefulset
3 participants