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

Add downgrade commands #13687

Merged
merged 8 commits into from
Feb 22, 2022
Merged

Add downgrade commands #13687

merged 8 commits into from
Feb 22, 2022

Conversation

serathius
Copy link
Member

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Followup on #13360 @LeoYang90

With downgrades implemented we can introduce etcdctl downgrade command. There are still some problems with API (request always times out however I think this is enough for alpha.

Would be great if someone took a look why downgrade api returns timeout.

cc @ptabor @spzala @ahrtr

etcdctl/ctlv3/command/downgrade_command.go Outdated Show resolved Hide resolved
etcdctl/ctlv3/command/downgrade_command.go Outdated Show resolved Hide resolved
etcdctl/ctlv3/command/printer_simple.go Outdated Show resolved Hide resolved
fmt.Printf("Downgrade validate success, cluster version %s", r.Version)
}
func (s *simplePrinter) DowngradeEnable(r v3.DowngradeResponse) {
fmt.Printf("Downgrade enable success, cluster version %s", r.Version)
Copy link
Member

Choose a reason for hiding this comment

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

It's clearer to change to "cluster version" to "target cluster version".

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

etcdctl/ctlv3/command/printer_simple.go Outdated Show resolved Hide resolved
@serathius serathius force-pushed the etcdctl branch 2 times, most recently from 34c2523 to 12d7d86 Compare February 14, 2022 22:01
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

We can discuss/revisit the cancel() vs defer cancel() in separate session.

etcdctl/README.md Outdated Show resolved Hide resolved

```bash
./etcdctl downgrade validate 3.5.0
Downgrade validate success, cluster version 3.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect automated scripts to parse that output, or there is more suitable format for automation, e.g.:

currentClusterVersion: 3.6.0
proposedClusterVersion: 3.5.0
validationStatus: SUCCESS
``` ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • shell we document that the request version needs to be 'major' (x.y.0) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

For automation scripts I think we should add support for json output.

As for versions, I think we should change it to "{Major}.{Minor}" version.

Copy link
Member Author

@serathius serathius Feb 22, 2022

Choose a reason for hiding this comment

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

Done, for automated scripts -w json will already work and write full response with cluster version.


```bash
./etcdctl downgrade cancel
Downgrade cancel success, cluster version 3.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

What does success mean in the context of cancellation ?
That quorum is operating on 3.6.0 ? That all the nodes are at 3.6.0.

We should document the expectations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me properly document the commands.

etcdctl/ctlv3/command/printer_simple.go Outdated Show resolved Hide resolved
fmt.Printf("Downgrade validate success, cluster version %s", r.Version)
}
func (s *simplePrinter) DowngradeEnable(r v3.DowngradeResponse) {
fmt.Printf("Downgrade enable success, cluster version %s", r.Version)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #13687 (42faf9f) into main (bf0b1a7) will increase coverage by 0.56%.
The diff coverage is 31.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #13687      +/-   ##
==========================================
+ Coverage   72.57%   73.14%   +0.56%     
==========================================
  Files         465      467       +2     
  Lines       37856    38847     +991     
==========================================
+ Hits        27475    28413     +938     
- Misses       8596     8603       +7     
- Partials     1785     1831      +46     
Flag Coverage Δ
all 73.14% <31.76%> (+0.56%) ⬆️

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

Impacted Files Coverage Δ
client/v3/maintenance.go 55.97% <0.00%> (ø)
etcdctl/ctlv3/command/printer.go 44.35% <0.00%> (-2.26%) ⬇️
etcdctl/ctlv3/command/printer_simple.go 75.30% <0.00%> (-2.90%) ⬇️
server/etcdserver/v3_server.go 78.83% <0.00%> (ø)
etcdctl/ctlv3/command/downgrade_command.go 40.62% <40.62%> (ø)
etcdctl/ctlv3/ctl.go 86.20% <100.00%> (+0.24%) ⬆️
client/v3/concurrency/election.go 79.68% <0.00%> (-2.35%) ⬇️
raft/raft.go 90.18% <0.00%> (-0.25%) ⬇️
server/etcdserver/api/v3discovery/discovery.go 80.00% <0.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf0b1a7...42faf9f. Read the comment docs.

@serathius serathius merged commit 6af7601 into etcd-io:main Feb 22, 2022
@ahrtr
Copy link
Member

ahrtr commented Mar 9, 2022

@serathius Are you going add an item for this PR into 3.6 CHANGELOG?

@serathius
Copy link
Member Author

Yes, I should add a note in umbrella issue for downgrades

@serathius serathius deleted the etcdctl branch June 15, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants