-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 etcd status condition #8724
Conversation
05189b7
to
5ea1400
Compare
1e052d1
to
28aad7e
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #8724 +/- ##
==========================================
+ Coverage 47.03% 47.42% +0.38%
==========================================
Files 148 145 -3
Lines 15658 15672 +14
==========================================
+ Hits 7365 7432 +67
+ Misses 7089 6981 -108
- Partials 1204 1259 +55
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
28aad7e
to
e76b3c8
Compare
e76b3c8
to
8a7b5ea
Compare
8a7b5ea
to
e574792
Compare
Signed-off-by: Vitor <vitor.savian@suse.com>
e574792
to
7c16229
Compare
@vitorsavian @brandond
The only exception is the READY condition of a node. If you take Memorypressure, diskpressure, pidpressue, networkunavailable as the standard conditions K8s supplies, you will see that each of them will report FALSE as the NON-FAULTY condition. Please stick to this convention, it makes life down the line much easier. If the "ok" / "not ok" classification doesn't apply for your etcd status condition (e.g. being a learner is per se neither bad nor good), then the NodeCondition is the wrong API field to use, but it should rather be reported via the etcd endpoint. |
I don't agree. The convention is to use true=bad for error conditions, and true=good for readiness conditions. This etcd status condition is intended to indicate the readiness of the etcd cluster member, so for us true=good. We are not planning on changing it at this point.
The whole point of this is to remove the need to query etcd directly. |
Unfortunately K8s is itself inconsistent, thus we are at this situation now. They should have just called the condition Ready instead NotReady and then everything would be fine. Because the differentiation between an error condition and a readiness condition is a philosophical one. If NetworkUnavailable would be called NetworkAvailable, it would be a readiness condition (is the network ready) . A etcd member to be a voting member is for me not a readiness condition of a node. The node is ready even if the etcd member status would be learner. The EtcdIsVoter falls under the same category as the pressure and the network condition. I am not getting the argument, why it should be classified as a readiness condition. As an example, node problem detector I believe follows also the convention: true=bad |
Absent a standard, its a matter of maintainer preference. We've chosen to do it this way, and are highly unlikely to invert the logic now that it's been working this way for several releases. |
Proposed Changes
Types of Changes
Verification
You can run a two control-planes with etcd and use this command
kubectl get node {NODE_NAME} -o=jsonpath='Node Name: {.metadata.name}{"\n"}Conditions:{"\n"}{range .status.conditions[*]}- Type: {.type}{"\n"} Status: {.status}{"\n"} LastHeartbeatTime: {.lastHeartbeatTime}{"\n"} LastTransitionTime: {.lastTransitionTime}{"\n"} Reason: {.reason}{"\n"} Message: {.message}{"\n\n"}{end}'
Testing
Linked Issues
User-Facing Change
Further Comments