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

api/etcdhttp: change /health type back to string for backwards compatibility #9143

Merged
merged 1 commit into from
Jan 17, 2018

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Jan 15, 2018

Fixes #9144
Fixes breaking change in /health API response prior to 3.3.0 release

Needs picking to release-3.3

Example impact of compatibility break is at kubernetes/kubernetes#58240

@liggitt
Copy link
Contributor Author

liggitt commented Jan 15, 2018

cc @gyuho

@liggitt liggitt changed the title Change /health type back to string for backwards compatibility api/etcdhttp: change /health type back to string for backwards compatibility Jan 15, 2018
@xiang90
Copy link
Contributor

xiang90 commented Jan 15, 2018

if we are going to revert this, we need to revert the newly added reason fields. I do not want to evolve the broken health endpoint if we do not want to fix it.

But before doing anything, I want to understand why it is hard for application to adopt this change.

@liggitt
Copy link
Contributor Author

liggitt commented Jan 15, 2018

if we are going to revert this, we need to revert the newly added reason fields. I do not want to evolve the broken health endpoint if we do not want to fix it.

That doesn't follow. A well-behaved, forwards-compatible json client ignores unknown fields, so adding the error details is far safer than changing the type of an existing field.

But before doing anything, I want to understand why it is hard for application to adopt this change.

I'd actually ask the opposite question... I want to understand why it is hard for etcd to maintain compatibility here. Unless there's a compelling reason, etcd updates shouldn't require clients to rewrite their checks, or add check-bool-then-fallback-to-string logic.

@xiang90
Copy link
Contributor

xiang90 commented Jan 15, 2018

That doesn't follow.

I do not want to evolve an API with a broken field that can never be fixed. The health API is not versioned from the beginning.

@xiang90
Copy link
Contributor

xiang90 commented Jan 15, 2018

@liggitt

If you have a way to fix this broken field while keep this exact http path and does not introduce any compatibility issue, I would like to hear.

@liggitt
Copy link
Contributor Author

liggitt commented Jan 15, 2018

The health API is not versioned from the beginning.

Then it seems like the right approach would be to live with the current state of the existing fields and only make changes in an additive way.

@xiang90
Copy link
Contributor

xiang90 commented Jan 15, 2018

Then it seems like the right approach would be to live with the current state of the existing fields and only make changes in an additive way.

Again,

  1. if the change affects a lot of clients, tell me which are affected. there are tradeoffs to be made. If this breaking change only affects one or two applications/clients, i would rather fix the client/application.

  2. if you hope we can evolve (meaning that we can add,remove,replace fields and its types) a unversioned API without breaking users, tell me exactly how. if there is no way to do that, and we want to keep the current API. I would rather keep the current one as simple as possible, and start to version this API to make it evolveable in the future. Or if you believe that the health API can be stabilized and evolved without versioning, provide reasons behind it.

If you do not help me to understand the above two questions, we will simply go circles here.

@liggitt
Copy link
Contributor Author

liggitt commented Jan 15, 2018

if the change affects a lot of clients, tell me which are affected.

That is unknowable. It affects any client that wrote a health check following the documented API. Don't assume all of those uses are published and public.

there are tradeoffs to be made

I don't see a strong case for breaking the existing API. The string can express everything the boolean can.

if you hope we can evolve (meaning that we can add,remove,replace fields and its types) a unversioned API without breaking users, tell me exactly how.

You can only add new fields, and as you do, indicate they are optional. For such a simple API as this, that takes you a long way.

@xiang90
Copy link
Contributor

xiang90 commented Jan 15, 2018

You can only add new fields, and as you do, indicate they are optional.

I fully understand how to keep a json API backward and forward compatible.

For such a simple API as this,

As I said, I would like to keep it as simple as possible if the API is unversioned and we 100% do not want to break it. So I told you to revert the other newly added fields, which make this API complicated.

If we want a complicated healthy API, let us do it correctly if nothing can be changed.

@liggitt
Copy link
Contributor Author

liggitt commented Jan 15, 2018

I told you to revert the other newly added fields, which make this API complicated.

My only concern was fixing the compatibility break before 3.3.0 is released, which this PR does. I'll let @gyuho handle a complete revert of #8312 if that is what is desired instead.

I will point out that @heyitsanthony had the same opinion on changing this field when you asked

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@7a8c192). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9143   +/-   ##
=========================================
  Coverage          ?   76.06%           
=========================================
  Files             ?      359           
  Lines             ?    30006           
  Branches          ?        0           
=========================================
  Hits              ?    22825           
  Misses            ?     5599           
  Partials          ?     1582
Impacted Files Coverage Δ
proxy/grpcproxy/health.go 8.33% <0%> (ø)
etcdserver/api/etcdhttp/metrics.go 76.31% <100%> (ø)

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 7a8c192...f77e54e. Read the comment docs.

@gyuho
Copy link
Contributor

gyuho commented Jan 17, 2018

Talked with @xiang90. We think it's best to not break backward compatibility in this case.
Next time, we will clearly document whether it's stable or experimental.
/health endpoint wasn't clearly documented.

@gyuho gyuho merged commit 6a80e94 into etcd-io:master Jan 17, 2018
@liggitt liggitt deleted the healthz-schema branch June 11, 2018 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants