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

Split etcdctl into etcdctl (public API access) & etcdutl (direct surgery on files) #12971

Merged
merged 5 commits into from
May 17, 2021

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented May 14, 2021

Split etcdctl into etcdctl (public API access) & etcdutl (direct surgery on files)

Motivation is as follows:

  • etcdctl we only depend on clientv3 APIs, no dependencies of bolt, backend, mvcc, file-layout
  • etcdctl can be officially supported across wide range of versions, while etcdutl is pretty specific to file format at particular version.
  • it's step towards desired modules layout, documented in: https://etcd.io/docs/next/dev-internal/modules/

This PR defines new binary: etcdutl with following jobs:

  • (V2) etcdctl backup -> etcdutl backup
  • (V3) etcdctl snapshot restore -> etcdutl snapshot restore
  • (V3) etcdctl snapshot status -> etcdutl snapshot status
  • (V3) etcdctl defrag --date-dir=/..' -> etcdutl defrag --date-dir=/..`
  • (V3) etcdctl migrate -> I decided to decommission that tool as we support migration by 1 minor version... so continous support of V2->V3 sounds too generous (and the code is very subtle),

The etcdctl commands keep being served as they used to, but just print additional warning with recommendation to migrate to etcdutl.

This PR is on top of: #12969 [rebased]
It fullfils one one of the goals from: #12330 (comment)

@gyuho
Copy link
Contributor

gyuho commented May 14, 2021

In general, I like this approach (decoupling server-side dependencies for client-side operation). Can we make this more incremental? I think cobrautil can be a separate PR? (Just saw a PR)

@ptabor
Copy link
Contributor Author

ptabor commented May 14, 2021

Thank you for merging in the base. I rebased the PR.

@gyuho
Copy link
Contributor

gyuho commented May 14, 2021

I believe if we make this transition, this should be in 3.5. I am in favor of merging this, as long as @hexfusion @lilic are comfortable, since they are initial 3.5 release managers.

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2021

Codecov Report

Merging #12971 (3f7a038) into main (1675101) will decrease coverage by 11.07%.
The diff coverage is 66.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #12971       +/-   ##
===========================================
- Coverage   58.14%   47.06%   -11.08%     
===========================================
  Files         400      393        -7     
  Lines       32769    31672     -1097     
===========================================
- Hits        19053    14908     -4145     
- Misses      11807    14876     +3069     
+ Partials     1909     1888       -21     
Flag Coverage Δ
all 47.06% <66.48%> (-11.08%) ⬇️

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

Impacted Files Coverage Δ
etcdctl/ctlv3/command/defrag_command.go 52.17% <0.00%> (+20.59%) ⬆️
etcdctl/ctlv3/command/printer.go 48.38% <ø> (+4.26%) ⬆️
etcdctl/ctlv3/command/printer_fields.go 7.14% <ø> (+0.28%) ⬆️
etcdctl/ctlv3/command/printer_json.go 75.00% <ø> (-0.52%) ⬇️
etcdctl/ctlv3/command/printer_simple.go 68.00% <ø> (+2.31%) ⬆️
etcdctl/ctlv3/command/printer_table.go 0.00% <ø> (ø)
etcdctl/ctlv3/ctl.go 93.75% <ø> (-0.13%) ⬇️
etcdutl/etcdutl/printer_fields.go 0.00% <0.00%> (ø)
etcdutl/etcdutl/printer_protobuf.go 0.00% <0.00%> (ø)
etcdutl/etcdutl/printer_simple.go 0.00% <0.00%> (ø)
... and 218 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 1675101...3f7a038. Read the comment docs.

@ptabor ptabor force-pushed the 20210514-split-etcdctl branch 3 times, most recently from 10d8560 to d35ddb0 Compare May 16, 2021 14:30
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Wow amazing work here! I briefly had a look.

Do you think there is any risk in including this in 3.5? From what I can tell, a lot of things were mainly moved over and nicely cleaned up, so risk is low in bugs?

etcdutl/README.md Outdated Show resolved Hide resolved
etcdutl/README.md Outdated Show resolved Hide resolved
etcdutl/README.md Outdated Show resolved Hide resolved
etcdutl/README.md Outdated Show resolved Hide resolved
etcdutl/etcdutl/backup_command.go Outdated Show resolved Hide resolved
CHANGELOG-3.5.md Outdated Show resolved Hide resolved
@ptabor
Copy link
Contributor Author

ptabor commented May 17, 2021

@hexfusion I hope that the change is complete, in particular it contains:

  • changes to release script to take in consideration etcdutl binary existence (tested)
  • e2e tests executed against both: etcdctl & etcdutl variants of the commands
  • README.md

…ery on files)

Motivation is as follows:

  - etcdctl we only depend on clientv3 APIs, no dependencies of bolt, backend, mvcc, file-layout
  - etcdctl can be officially supported across wide range of versions, while etcdutl is pretty specific to file format at particular version.
it's step towards desired modules layout, documented in: https://etcd.io/docs/next/dev-internal/modules/
@ptabor
Copy link
Contributor Author

ptabor commented May 17, 2021

Wow amazing work here! I briefly had a look.

Do you think there is any risk in including this in 3.5? From what I can tell, a lot of things were mainly moved over and nicely cleaned up, so risk is low in bugs?

Thank you for review. I don't expect any changes in the pre-existing functionality of etdctl (except v2 migrate that is gone). The functionality is covered with e2e tests. So I don't think its a risky change.

@lilic
Copy link
Contributor

lilic commented May 17, 2021

Overall from a code perspective and dev perspective, this makes perfect sense. 👍

My only concern is that this would be making the user experience a bit more complex, as users would need 3 binaries/tools to run and manage etcd. But we can see how it goes and gather user feedback when end-users start using etcdutl?

Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

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

The etcdctl commands keep being served as they used to, but just print additional warning with recommendation to migrate to etcdutl.

With the above messaging, I feel this change is reasonable. By separating these very admin-specific actions of backing up and restoring state to a separate binary we provider at least basic isolation. The alternative would be RBAC level controls similar to oc adm but since etcd RBAC will probably never be a part of kubernetes 1.x +1.

@ptabor
Copy link
Contributor Author

ptabor commented May 17, 2021

Thank you for quick review.

@ptabor ptabor merged commit 932d42b into etcd-io:main May 17, 2021
@ptabor ptabor deleted the 20210514-split-etcdctl branch May 17, 2021 12:07
@gyuho
Copy link
Contributor

gyuho commented May 17, 2021

Highlighted this in the 3.5 blog as well.

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.

5 participants