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

Data mover micro-service watcher #7999

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

Lyndon-Li
Copy link
Contributor

@Lyndon-Li Lyndon-Li commented Jul 11, 2024

Fix #7932. Data mover micro-service watcher according to design #7576

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 67.96117% with 99 lines in your changes missing coverage. Please review.

Project coverage is 58.88%. Comparing base (6a3e226) to head (faa704d).
Report is 35 commits behind head on main.

Files Patch % Lines
pkg/datapath/micro_service_watcher.go 59.34% 87 Missing ⚠️
pkg/util/logging/log_merge_hook.go 89.74% 3 Missing and 1 partial ⚠️
pkg/cmd/cli/nodeagent/server.go 0.00% 3 Missing ⚠️
pkg/util/kube/pod.go 91.89% 3 Missing ⚠️
pkg/util/logging/default_logger.go 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7999      +/-   ##
==========================================
+ Coverage   58.82%   58.88%   +0.06%     
==========================================
  Files         346      351       +5     
  Lines       28891    29367     +476     
==========================================
+ Hits        16996    17294     +298     
- Misses      10463    10639     +176     
- Partials     1432     1434       +2     

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

@kaovilai
Copy link
Contributor

I think re-titling ms to microservice would be less confusing.

@Lyndon-Li Lyndon-Li changed the title Data mover ms watcher Data mover micro-service watcher Jul 23, 2024
@Lyndon-Li
Copy link
Contributor Author

I think re-titling ms to microservice would be less confusing.

Done

blackpiglet
blackpiglet previously approved these changes Jul 24, 2024
sseago
sseago previously approved these changes Jul 24, 2024

defer func() {
if !succeeded {
if err := eventInformer.RemoveEventHandler(eventHandler); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use ms.Close() here ? Are we not missing the cancel() call as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not so confident to call ms.Close() in the Init, as ms.Close() is doing more things and may do even more in future, if it is not required, we should not couple them together.
So here I moved ms.ctx, ms.cancel = context.WithCancel(ctx) to the end, actually, this ctx with cancel is used after Init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the caller may also call ms.Close() when Init returns error. At present, there is no problem.
But here I think we should avoid adding one more coupling if we can, in future, if ms.Close() is not suitable to be called when Init returns error, we should correct the caller.


defer func() {
if !succeeded {
if err := podInformer.RemoveEventHandler(podHandler); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use ms.Close() here ? Are we not missing the cancel() call as well ?
same as line 148 comment earlier. Curious to know if my understanding is incorrect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

or do we need to combine both the defer calls for podhandler and eventhandler and then call ms.Close()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Lyndon-Li <[email protected]>
@Lyndon-Li
Copy link
Contributor Author

@shubham-pampattiwar @sseago @blackpiglet I changed the code slightly per discussion #7999 (comment), please have another look. Thanks.

@Lyndon-Li Lyndon-Li merged commit 53b57f8 into vmware-tanzu:main Jul 26, 2024
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data mover micro service - dual mode watcher
5 participants