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

pdms: support primary/transfer api for scheduling and tso #8157

Merged
merged 37 commits into from
Aug 13, 2024

Conversation

HuSharp
Copy link
Member

@HuSharp HuSharp commented May 9, 2024

What problem does this PR solve?

For tiup

  1. When we have 3 pdms, pdms-0/pdms-1/pdms-2, and pdms-2 is primary
  2. upgrade pdms-2 firstly maybe transfer primary to pdms-0
  3. uprade pdms-0 will transfer primary again

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:

  1. When we have 3 pdms, pdms-0/pdms-1/pdms-2, and pdms-2 is primary
  2. upgrade pdms-2 firstly maybe transfer primary to pdms-1
  3. upgrade pdms-1 maybe transfer primary to pdms-0.

To fix it, Assume that current primary ordinal is x, and range is [0, n]

  1. Find the max suitable ordinal in (x, n], because they have been upgraded
  2. If no suitable ordinal, find the min suitable ordinal in [0, x) to reduce the count of transfer

Ref pingcap/tidb-operator#5643

Issue Number: Close #7995, Ref #5766, #7519

What is changed and how does it work?

  1. Add primaryWatch for the primary watch only, which is used to reuse Watch interface in Leadership.
  2. primaryWatch watches /ms/primary/transfer API whether changed the primary.
    1. modify the expected primary flag to the new primary
    2. modify memory status
    3. exit the primary watch loop
    4. delete the leader key
  3. Expected primary flag is the path to store the expected primary , ONLY Triggered BY /ms/primary/transfer API.
    • This flag likes a fence to avoid exited 2 primaries in the cluster simultaneously.
    • Since follower will campaign a new primary when it found the leader_key is deleted.
    • We can ensure expected_primary is set before deleting the leader_key.
    • Old primary will set expected_primary firstly,then delete the leader_key which will trigger the follower to campaign a new primary.
  4. support /ms/primary/transfer API to change primary
$ curl --location --request GET 'http://127.0.0.1:2379/pd/api/v2/ms/primary/tso'
"http://127.0.0.1:2382"%

$ curl --location --request POST 'http://127.0.0.1:2379/pd/api/v2/ms/primary/transfer/tso' \
--header 'Content-Type: text/plain' \
--data-raw '{
    "new_primary": "tso-0"
}'
"success"%

$ curl --location --request GET 'http://127.0.0.1:2379/pd/api/v2/ms/primary/tso'
"http://127.0.0.1:2384"

$ curl --location --request POST 'http://127.0.0.1:2379/pd/api/v2/ms/primary/transfer/tso' \
--header 'Content-Type: application/json' \
--data-raw '{
    "new_primary": ""
}'
"success"

$ curl --location --request GET 'http://127.0.0.1:2379/pd/api/v2/ms/primary/tso'
"http://127.0.0.1:2382"

the members info are

curl --location --request GET 'http://127.0.0.1:2379/pd/api/v2/ms/members/tso'

get

[
    {
        "name": "tso-0",
        "service-addr": "http://127.0.0.1:2384",
        "version": "v8.2.0-alpha-23-gdd72b9c19-dirty",
        "git-hash": "dd72b9c19707ccbdb1801d379b3982a7944df23f",
        "deploy-path": "/Users/pingcap/CS/PingCAP/pd/bin",
        "start-timestamp": 1715577605,
        "member-value": "ChtodHRwOi8vMTI3LjAuMC4xOjIzODQtMDAwMDAQp+L2iMCp3NUaGhVodHRwOi8vMTI3LjAuMC4xOjIzODQ="
    },
    {
        "name": "tso-1",
        "service-addr": "http://127.0.0.1:2386",
        "version": "v8.2.0-alpha-23-gdd72b9c19-dirty",
        "git-hash": "dd72b9c19707ccbdb1801d379b3982a7944df23f",
        "deploy-path": "/Users/pingcap/CS/PingCAP/pd/bin",
        "start-timestamp": 1715577605,
        "member-value": "ChtodHRwOi8vMTI3LjAuMC4xOjIzODYtMDAwMDAQj9ro5Yq9mY8mGhVodHRwOi8vMTI3LjAuMC4xOjIzODY="
    }
]

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Release note

None.

Copy link
Contributor

ti-chi-bot bot commented May 9, 2024

[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 /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 9, 2024
Signed-off-by: husharp <[email protected]>
@HuSharp HuSharp force-pushed the support_transfer_primary2 branch from 510f18d to 7fa19d3 Compare May 9, 2024 03:03
@HuSharp HuSharp force-pushed the support_transfer_primary2 branch 2 times, most recently from 480b075 to 2a647ac Compare May 9, 2024 03:34
Signed-off-by: husharp <[email protected]>
@HuSharp HuSharp force-pushed the support_transfer_primary2 branch from 2a647ac to 1f13fa2 Compare May 9, 2024 03:55
pkg/election/leadership.go Outdated Show resolved Hide resolved
pkg/election/leadership.go Outdated Show resolved Hide resolved
pkg/mcs/discovery/discover.go Outdated Show resolved Hide resolved
pkg/mcs/scheduling/server/server.go Outdated Show resolved Hide resolved
pkg/mcs/scheduling/server/server.go Outdated Show resolved Hide resolved
@HuSharp HuSharp force-pushed the support_transfer_primary2 branch 2 times, most recently from 5490d10 to 02a8c4a Compare May 9, 2024 13:17
@HuSharp HuSharp force-pushed the support_transfer_primary2 branch from 02a8c4a to 8d36be5 Compare May 9, 2024 13:19
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 75.70093% with 52 lines in your changes missing coverage. Please review.

Project coverage is 77.49%. Comparing base (3f32f54) to head (2d9a3b0).
Report is 2 commits behind head on master.

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     
Flag Coverage Δ
unittests 77.49% <75.70%> (+0.08%) ⬆️

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]>
@HuSharp HuSharp force-pushed the support_transfer_primary2 branch from 1fe976d to dd72b9c Compare May 10, 2024 09:09
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")
Copy link
Member

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?

Copy link
Member Author

@HuSharp HuSharp May 13, 2024

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

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

pd/tests/testutil.go

Lines 87 to 88 in 51708b5

cfg.Name = cfg.ListenAddr
cfg, err := tso.GenerateConfig(cfg)

Copy link
Member

Choose a reason for hiding this comment

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

got

pkg/mcs/scheduling/server/server.go Outdated Show resolved Hide resolved
pkg/mcs/utils/util.go Outdated Show resolved Hide resolved
pkg/election/leadership.go Outdated Show resolved Hide resolved
pkg/election/leadership.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2024
@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024
Signed-off-by: husharp <[email protected]>
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024
@@ -25,6 +23,7 @@ import (
"github.com/tikv/pd/pkg/utils/etcdutil"
"go.etcd.io/etcd/clientv3"
"go.uber.org/zap"
"strconv"
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change.

Copy link
Member Author

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/election/leadership.go Outdated Show resolved Hide resolved
pkg/election/lease_test.go Outdated Show resolved Hide resolved
pkg/mcs/discovery/discover.go Outdated Show resolved Hide resolved
pkg/mcs/scheduling/server/server.go Show resolved Hide resolved
pkg/mcs/scheduling/server/server.go Outdated Show resolved Hide resolved
// - 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{},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func KeepExpectedPrimaryAlive(ctx context.Context, cli *clientv3.Client, exitPrimary chan struct{},
func KeepExpectedPrimaryAlive(ctx context.Context, cli *clientv3.Client, exitPrimary chan<- struct{},

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch

pkg/mcs/utils/expected_primary.go Outdated Show resolved Hide resolved
Signed-off-by: husharp <[email protected]>
@HuSharp
Copy link
Member Author

HuSharp commented Aug 12, 2024

@JmPotato @rleungx Can you put in a lgtm to indicate that you have agreed? Then I'll cancel the hold label. :) Thx!

pkg/mcs/utils/expected_primary.go Outdated Show resolved Hide resolved
@@ -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() {
Copy link
Member

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.

Copy link
Member Author

@HuSharp HuSharp Aug 13, 2024

Choose a reason for hiding this comment

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

https://github.com/HuSharp/pd/blob/43830ec9ea80d92008be663ec5c26b3d137373a9/pkg/mcs/utils/expected_primary.go#L141-L146

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]>
Copy link
Contributor

ti-chi-bot bot commented Aug 13, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: husharp <[email protected]>
@HuSharp
Copy link
Member Author

HuSharp commented Aug 13, 2024

Completed manual testing of operator and tiup

@HuSharp
Copy link
Member Author

HuSharp commented Aug 13, 2024

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024
Copy link
Contributor

ti-chi-bot bot commented Aug 13, 2024

@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.

@ti-chi-bot ti-chi-bot bot merged commit 1c1cd99 into tikv:master Aug 13, 2024
20 of 21 checks passed
@HuSharp HuSharp deleted the support_transfer_primary2 branch August 13, 2024 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

primary changed twice when pdms rolling update
3 participants