-
Notifications
You must be signed in to change notification settings - Fork 725
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
pdms: support primary/transfer api for scheduling and tso #8157
Conversation
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: husharp <[email protected]>
510f18d
to
7fa19d3
Compare
480b075
to
2a647ac
Compare
Signed-off-by: husharp <[email protected]>
2a647ac
to
1f13fa2
Compare
Signed-off-by: husharp <[email protected]>
5490d10
to
02a8c4a
Compare
Signed-off-by: husharp <[email protected]>
02a8c4a
to
8d36be5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8157 +/- ##
==========================================
+ Coverage 77.40% 77.49% +0.08%
==========================================
Files 472 473 +1
Lines 61821 61934 +113
==========================================
+ Hits 47854 47993 +139
+ Misses 10400 10373 -27
- Partials 3567 3568 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: husharp <[email protected]>
Signed-off-by: husharp <[email protected]>
1fe976d
to
dd72b9c
Compare
Signed-off-by: husharp <[email protected]>
Signed-off-by: husharp <[email protected]>
"--listen-addr=" + c.ListenAddr, | ||
"--advertise-listen-addr=" + c.AdvertiseListenAddr, | ||
"--backend-endpoints=" + c.BackendEndpoints, | ||
} | ||
|
||
flagSet := pflag.NewFlagSet("test", pflag.ContinueOnError) | ||
flagSet.StringP("name", "", "", "human-readable name for this scheduling member") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we need a default name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default name set by this code, which is like TSO-localhost
pd/pkg/mcs/tso/server/config.go
Lines 185 to 195 in a39300e
func (c *Config) Adjust(meta *toml.MetaData) error { | |
configMetaData := configutil.NewConfigMetadata(meta) | |
if err := configMetaData.CheckUndecoded(); err != nil { | |
c.WarningMsgs = append(c.WarningMsgs, err.Error()) | |
} | |
if c.Name == "" { | |
hostname, err := os.Hostname() | |
if err != nil { | |
return err | |
} | |
configutil.AdjustString(&c.Name, fmt.Sprintf("%s-%s", defaultName, hostname)) |
And your commented snippet is for testing to avoid using the same name for the same machine for local testing, I used addr here
Lines 87 to 88 in 51708b5
cfg.Name = cfg.ListenAddr | |
cfg, err := tso.GenerateConfig(cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got
Signed-off-by: husharp <[email protected]>
Signed-off-by: husharp <[email protected]>
Signed-off-by: husharp <[email protected]>
pkg/mcs/discovery/discover.go
Outdated
@@ -25,6 +23,7 @@ import ( | |||
"github.com/tikv/pd/pkg/utils/etcdutil" | |||
"go.etcd.io/etcd/clientv3" | |||
"go.uber.org/zap" | |||
"strconv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check why our static doesn't detect it here. 🤔
Signed-off-by: husharp <[email protected]>
pkg/mcs/utils/expected_primary.go
Outdated
// - changed by `{service}/primary/transfer` API. | ||
// - leader lease expired. | ||
// ONLY primary called this function. | ||
func KeepExpectedPrimaryAlive(ctx context.Context, cli *clientv3.Client, exitPrimary chan struct{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func KeepExpectedPrimaryAlive(ctx context.Context, cli *clientv3.Client, exitPrimary chan struct{}, | |
func KeepExpectedPrimaryAlive(ctx context.Context, cli *clientv3.Client, exitPrimary chan<- struct{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch
Signed-off-by: husharp <[email protected]>
@@ -376,6 +391,13 @@ func (ls *Leadership) Watch(serverCtx context.Context, revision int64) { | |||
zap.Int64("revision", wresp.Header.Revision), zap.String("leader-key", ls.leaderKey), zap.String("purpose", ls.purpose)) | |||
return | |||
} | |||
// ONLY `{service}/primary/transfer` API update primary will meet this condition. | |||
if ev.Type == mvccpb.PUT && ls.IsPrimary() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we can optimize the case that if the updated primary is still itself, no need to return to campaign again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skipped the member on the outer layer(in transfer primary
) which is not the same as oldPrimary, do you think I still need to add the restriction inside the watch
?
Maybe the expected primary flag
should not be modified when the leader is itself, because this flag must keep the lease alive after campaigning new leader, which means that it requires the Primary to quit the current watch
.
Signed-off-by: husharp <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JmPotato, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: husharp <[email protected]>
Completed manual testing of operator and tiup |
/unhold |
@HuSharp: Your PR was out of date, I have automatically updated it for you. If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
For tiup
We can upgrade pdms primary in last place(named defer feature) can avoid unnecessary primary transfer
Ref pingcap/tiup#2414
For operator
tidb-operator does not have the ability to
defer feature
, it can only upgrade the pods in order.Furthermore, Thinking about this situation:
To fix it, Assume that current primary ordinal is x, and range is [0, n]
Ref pingcap/tidb-operator#5643
Issue Number: Close #7995, Ref #5766, #7519
What is changed and how does it work?
primaryWatch
for the primary watch only, which is used to reuseWatch
interface inLeadership
.primaryWatch
watches/ms/primary/transfer
API whether changed the primary./ms/primary/transfer
API.leader_key
is deleted.expected_primary
is set before deleting theleader_key
.expected_primary
firstly,then delete theleader_key
which will trigger the follower to campaign a new primary./ms/primary/transfer
API to change primarythe members info are
curl --location --request GET 'http://127.0.0.1:2379/pd/api/v2/ms/members/tso'
get
Check List
Tests
Release note