-
Notifications
You must be signed in to change notification settings - Fork 577
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
k8s: Wait for restarted broker to catch up #7594
k8s: Wait for restarted broker to catch up #7594
Conversation
c5fe25d
to
38e0c58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor issue on the logic. Other than that, it looks good.
continue | ||
} | ||
if m.Gauge == nil { | ||
r.logger.Info("cluster_partition_under_replicated_replicas metric does not have value", "labels", m.Label) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continue
missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx added
@@ -139,6 +140,7 @@ func NewStatefulSet( | |||
decommissionWaitInterval, | |||
logger.WithValues("Kind", statefulSetKind()), | |||
nil, | |||
0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there's no way to customize this. Is this expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. In ideal world Redpanda operator should have close loop for rolling update. When it detects the need for restart, then Redpanda should be throttled to the point where operator can observe no lag on all Redpanda instances. This is ideal, but in real deployment we can not throttle producers, so there are to options:
- threshold in cluster custom resource definition
- threshold in operator configuration (currently command line flags)
I would lean towards threshold in cluster custom resource definition.
What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would also feel better if this is customizable - in case this yields cluster never catching up so we can at least tweak it and let it move on...
I am fine with both options you presented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a restartConfig section in the CRD where this can fit. I'm thinking to cases where we do an upgrade between slightly incompatible versions and the cluster is unable to replicate partitions until all nodes are at the same version. The flag allows to unlock the upgrade..
if err = r.queryRedpandaUnderReplicatedPartition(ctx, &adminURL); err != nil { | ||
return &RequeueAfterError{ | ||
RequeueAfter: RequeueDuration, | ||
Msg: fmt.Sprintf("under replicated partition is not ready: %v", err), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking) Maybe a better description of the error...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try with broker reported under replicated partitions: %v
38e0c58
to
c5bb116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, great addition to make rollout safer! I left some comments though...
@@ -139,6 +140,7 @@ func NewStatefulSet( | |||
decommissionWaitInterval, | |||
logger.WithValues("Kind", statefulSetKind()), | |||
nil, | |||
0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would also feel better if this is customizable - in case this yields cluster never catching up so we can at least tweak it and let it move on...
I am fine with both options you presented.
client := &http.Client{Timeout: adminAPITimeout} | ||
|
||
// TODO right now we support TLS only on one listener so if external | ||
// connectivity is enabled, TLS is enabled only on external listener. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is true, tls could be enabled on internal listener as well. I think the behavior you're describing is true only for schema registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be brutally honest I copied it from your previous implementation
redpanda/src/go/k8s/pkg/resources/statefulset_update.go
Lines 417 to 419 in a6a021b
// TODO right now we support TLS only on one listener so if external | |
// connectivity is enabled, TLS is enabled only on external listener. This | |
// will be fixed by https://github.com/redpanda-data/redpanda/issues/1084 |
6235e96#diff-5619b1cd9bdb790f53d380d84b884e952da245749cb78145fdf8f6b328b1be2cR182-R184
} | ||
|
||
for name, metricFamily := range metrics { | ||
if name != "vectorized_cluster_partition_under_replicated_replicas" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is per node metric? Don't we have to look for metric for this particular node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will ask core how to discover under replicated replicas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I forget how I implemented this function. We query current POD. You can see that headless service with exact pod name is used
Host: fmt.Sprintf("%s.%s", pod.Name, headlessServiceWithPort), |
That means metrics comes from one POD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, thanks
12aac25
to
4515dc8
Compare
It works after merging Prove is in https://buildkite.com/redpanda/vtools/builds/4999 |
af460c4
to
77934ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, no blockers
@@ -177,6 +177,26 @@ type ClusterSpec struct { | |||
type RestartConfig struct { | |||
// DisableMaintenanceModeHooks deactivates the preStop and postStart hooks that force nodes to enter maintenance mode when stopping and exit maintenance mode when up again | |||
DisableMaintenanceModeHooks *bool `json:"disableMaintenanceModeHooks,omitempty"` | |||
|
|||
// UnderReplicatedPartitionThreshold regulate when rolling update will continue with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: regulates - or rather controls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -217,6 +217,19 @@ func TestDefault(t *testing.T) { | |||
redpandaCluster.Default() | |||
assert.Equal(t, v1alpha1.DefaultLicenseSecretKey, redpandaCluster.Spec.LicenseRef.Key) | |||
}) | |||
|
|||
t.Run("", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should the test have a name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name added
|
||
func TestEvaluateRedpandaUnderReplicatedPartition(t *testing.T) { | ||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
f, err := os.Open("testdata/metrics.gold.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the filed are supposed to be called golden, not gold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, name changed.
When rolling restart is performed then the broker might need to catch up with other replica. This change will parse /metrics endpoint to see if `vectorized_cluster_partition_under_replicated_replicas` is under threshold.
Kminion will create artificial load to have any values for `vectorized_cluster_partition_under_replicated_replicas` metric.
77934ba
to
f40b53c
Compare
…dpanda-datagh-3023/check-under-replicated-partitions-in-upgrade-procedure
…edpanda-datagh-3023/check-under-replicated-partitions-in-upgrade-procedure
UnderReplicatedPartitionThreshold regulate when rolling update will continue with
restarts. The procedure can be described as follows:
maintenance mode when new Pod starts)
The metric
vectorized_cluster_partition_under_replicated_replicas
is used in the comparisonMentioned metrics has the following help description:
vectorized_cluster_partition_under_replicated_replicas
Number of under replicated replicasBy default, the UnderReplicatedPartitionThreshold will be 0, which means all partitions needs to catch up without any lag.
Backports Required
UX Changes
Release Notes
Features
REF
#3023 (comment)