k8s: Error out when annotation can not be set

[RafalKorepta]

Given the following operator logic

redpanda/src/go/k8s/controllers/redpanda/cluster_controller.go

Lines 266 to 275 in 5176601

	// The following should be at the last part as it requires AdminAPI to be running
	if err := r.setPodNodeIDAnnotation(ctx, &vectorizedCluster, log, ar); err != nil {
	return ctrl.Result{}, fmt.Errorf("setting pod node_id annotation: %w", err)
	}
	if err := r.setPodNodeIDLabel(ctx, &vectorizedCluster, log, ar); err != nil {
	return ctrl.Result{}, fmt.Errorf("setting pod node_id label: %w", err)
	}
	if err := r.decommissionGhostBrokers(ctx, &vectorizedCluster, log, ar); err != nil {
	return ctrl.Result{}, fmt.Errorf("deleting ghost brokers: %w", err)
	}

The chain of events that causes wrong decommission logic is:

• setPodNodeIDAnnotation failed to get the Redpanda Node ID
• setPodNodeIDAnnotation does not return an error, but only it logs it out
• setPodNodeIDLabel was able to get Redpanda Node ID, so broker was able to boot up and register its Node ID
• decommissionGhostBrokers is performing filtering based on Pod annotation which is incorrect

Logs from the wrong execution:

{"level":"info","ts":"2023-09-27T06:51:41.644Z","logger":"ClusterReconciler.Reconcile.setPodNodeIDLabel","msg":"setting node-id label","controller":"cluster","controllerGroup":"redpanda.vectorized.io","controllerKind":"Cluster","Cluster":{"name":"repanda-cluster","namespace":"redpanda"},"namespace":"redpanda","name":"repanda-cluster","reconcileID":"57bf8b57-60b5-4248-bedd-3ffcaa5c4e07","pod-name":"repanda-cluster-1","new-node-id":3}
{"level":"info","ts":"2023-09-27T06:51:41.718Z","logger":"ClusterReconciler.Reconcile.setPodNodeIDAnnotation","msg":"decommission old node-id","controller":"cluster","controllerGroup":"redpanda.vectorized.io","controllerKind":"Cluster","Cluster":{"name":"repanda-cluster","namespace":"redpanda"},"namespace":"redpanda","name":"repanda-cluster","reconcileID":"fa64d3d8-02cf-4c1f-8a67-13a2dfb339ea","pod-name":"repanda-cluster-1","old-node-id":1}
{"level":"info","ts":"2023-09-27T06:51:41.758Z","logger":"ClusterReconciler.Reconcile.setPodNodeIDAnnotation","msg":"setting node-id annotation","controller":"cluster","controllerGroup":"redpanda.vectorized.io","controllerKind":"Cluster","Cluster":{"name":"repanda-cluster","namespace":"redpanda"},"namespace":"redpanda","name":"repanda-cluster","reconcileID":"fa64d3d8-02cf-4c1f-8a67-13a2dfb339ea","pod-name":"repanda-cluster-1","new-node-id":3}
{"level":"error","ts":"2023-09-27T06:51:42.497Z","msg":"Reconciler error","controller":"cluster","controllerGroup":"redpanda.vectorized.io","controllerKind":"Cluster","Cluster":{"name":"repanda-cluster","namespace":"redpanda"},"namespace":"redpanda","name":"repanda-cluster","reconcileID":"fa64d3d8-02cf-4c1f-8a67-13a2dfb339ea","error":"deleting ghost brokers: failed to decommission ghost broker: request PUT http://repanda-cluster-0.repanda-cluster.redpanda.svc.cluster.local/.:9644/v1/brokers/1/decommission failed: Bad Request, body: \"{\\\"message\\\": \\\"can not update broker 1 state, invalid state transition requested\\\", \\\"code\\\": 400}\"\n"}

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v23.2.x
• v23.1.x
• v22.3.x

Release Notes

Bug Fixes

• do not allow unsafe decommission procedure if annotation was not changed

[alejandroEsc]

lgtm, it seems the major change was the checking and returning early if brokers ordinal matchines the nodeID? was this the case you were referring to in your preamble?

[alejandroEsc]

interesting, so this needed to have locks so sequence is important?

[RafalKorepta]

Our reconciliation has changed and we require Pod Annotation with Redpanda Node ID. This test was written without our constraints. To mimic actual world example I added ghost borker function.

[RafalKorepta]

it seems the major change was the checking and returning early if brokers ordinal matchines the nodeID?

I'm not fully understand.

was this the case you were referring to in your preamble?

As I described in the cover letter, the problem is with the timing. We can not run unsafe decommission function if annotation for every Pod doesn't match the reality.

[alejandroEsc]

Can you move the v2 tests changes in another PR to better isolate problems?

[RafalKorepta]

you move the v2 tests changes in another PR to better isolate

Here you go #13898.

I removed all commits that was trying to fix e2e v2 tests. Please review

[alejandroEsc]

lgtm, a lot of maintenance changes but good cleaning

[vbotbuildovich]

/backport v23.2.x