Skip to content

feat: add redisreplication status masterNode#849

Merged
drivebyer merged 4 commits intoOT-CONTAINER-KIT:masterfrom
drivebyer:rr-status
Mar 30, 2024
Merged

feat: add redisreplication status masterNode#849
drivebyer merged 4 commits intoOT-CONTAINER-KIT:masterfrom
drivebyer:rr-status

Conversation

@drivebyer
Copy link
Copy Markdown
Collaborator

Description

Add redisreplication status masterNode

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Tests have been added/modified and all tests pass.
  • Functionality/bugs have been confirmed to be unchanged or fixed.
  • I have performed a self-review of my own code.
  • Documentation has been updated or added where necessary.

Additional Context

Signed-off-by: drivebyer <yang.wu@daocloud.io>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 19.35484% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 38.08%. Comparing base (d121d86) to head (f923e05).
Report is 15 commits behind head on master.

Files Patch % Lines
controllers/redisreplication_controller.go 0.00% 17 Missing ⚠️
k8sutils/redis.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
+ Coverage   35.20%   38.08%   +2.88%     
==========================================
  Files          19       19              
  Lines        3213     2649     -564     
==========================================
- Hits         1131     1009     -122     
+ Misses       2015     1569     -446     
- Partials       67       71       +4     

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

Signed-off-by: drivebyer <yang.wu@daocloud.io>
Signed-off-by: drivebyer <yang.wu@daocloud.io>
Signed-off-by: drivebyer <yang.wu@daocloud.io>
@drivebyer drivebyer enabled auto-merge (squash) March 29, 2024 08:21
@drivebyer drivebyer requested a review from shubham-cmyk March 29, 2024 08:21
@wkd-woo
Copy link
Copy Markdown
Contributor

wkd-woo commented Mar 29, 2024

Hello @drivebyer :) !!!
Could it be a feature of the same purpose as the Pull Request I previously requested? #785

@drivebyer
Copy link
Copy Markdown
Collaborator Author

@wkd-woo At first glance, they may seem identical, but they actually serve different purposes. With this pull request, we can obtain the conclusion status from a custom resource without having to delve into the details of the pod.

@wkd-woo
Copy link
Copy Markdown
Contributor

wkd-woo commented Mar 29, 2024

Thank you for your response! @drivebyer
Actually, there were some problems with the features I added.

As you said, the feature I requested was a workflow in the form of diving in and out of the pod, so it took a lot of time for reconcilers to update the "master" label after failover (about 1 minute)

This was too fatal to be applied in a production environment in reality. It means that HA outages take more than a minute to access a single endpoint as a service.

It would be great if this PR merges. Thanks!


  • I have an additional question, I'm not a k8s expert, so I'm asking, then is it possible to access master/slave from outside the cluster as a single endpoint through service?

@shubham-cmyk
Copy link
Copy Markdown
Member

@drivebyer can we try some failover testing here and check who is the master now

@shubham-cmyk shubham-cmyk marked this pull request as draft March 29, 2024 13:51
auto-merge was automatically disabled March 29, 2024 13:51

Pull request was converted to draft

@drivebyer
Copy link
Copy Markdown
Collaborator Author

@drivebyer can we try some failover testing here and check who is the master now

Sure

@drivebyer
Copy link
Copy Markdown
Collaborator Author

It would be great if this PR merges. Thanks!

I would review it.

@drivebyer
Copy link
Copy Markdown
Collaborator Author

drivebyer commented Mar 30, 2024

@shubham-cmyk, I tested it on my local k8s setup(sentinel + replication). The status gets updated to the new master once the old master pod is killed.

@drivebyer drivebyer marked this pull request as ready for review March 30, 2024 07:44
@drivebyer drivebyer enabled auto-merge (squash) March 30, 2024 13:25
Copy link
Copy Markdown
Member

@shubham-cmyk shubham-cmyk left a comment

Choose a reason for hiding this comment

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

LGTM

@drivebyer drivebyer merged commit eb1cc0d into OT-CONTAINER-KIT:master Mar 30, 2024
@shubham-cmyk shubham-cmyk added the tests-needed Test are needed label Mar 30, 2024
@drivebyer drivebyer deleted the rr-status branch March 30, 2024 13:54
wkd-woo pushed a commit to wkd-woo/redis-operator that referenced this pull request Apr 1, 2024
* feat: add redisreplication status masterNode

Signed-off-by: drivebyer <yang.wu@daocloud.io>

* fix e2e

Signed-off-by: drivebyer <yang.wu@daocloud.io>

* fix lint

Signed-off-by: drivebyer <yang.wu@daocloud.io>

* fix lint

Signed-off-by: drivebyer <yang.wu@daocloud.io>

---------

Signed-off-by: drivebyer <yang.wu@daocloud.io>
Signed-off-by: wkd-woo <wkdwoos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests-needed Test are needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants