operator: use pod annotation for node_id and avoid mixing maintenance mode with decommissioning #7581

joejulian · 2022-12-01T00:37:11Z

There were places where the pod ordinal was being used for the node_id (broker id). With v22.3.x those numbers may not match. By using the pod annotation, we can use the correct node_id.

This strategy doesn't work, however, when mixing decommission and maintenance mode. The preStop script would always set maintenance mode, even if the node was decommissioned, or in the process of being decommissioned. This avoids the need for running that workaround by checking in the preStop script and not setting maintenance mode if decommissioned/decommissioning.

Fixes #7541

Backports Required

UX Changes

N/A

Release Notes

Bug Fixes

fixes the use of the pod ordinal for node_id when they are not aligned

alenkacz

LGTM, no blockers but couple of nits

pretty interesting we did not have to touch much tests 😅

alenkacz · 2022-12-15T06:41:19Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

 	// "selector" are unavailable after the eviction, i.e. even in absence of
-	// the evicted pod. For example, one can prevent all voluntary evictions
+	// the evicted Pod. For example, one can prevent all voluntary evictions


nit: just curious why is Pod capital? :)

k8s resources are proper nouns. Making this distinction helps comprehension, especially with Node and Service. I forget why this caught my attention but I did put it in its own commit in case it was hated 😁

alenkacz · 2022-12-15T06:42:56Z

src/go/k8s/cmd/configurator/main.go

@@ -281,7 +279,7 @@ func registerAdvertisedKafkaAPI(
 }

 func registerAdvertisedPandaproxyAPI(
-	c *configuratorConfig, cfg *config.Config, index brokerID, proxyAPIPort int,
+	c *configuratorConfig, cfg *config.Config, index int, proxyAPIPort int,


this index is podOrdinal? Would. be nice to use just one name for the thing to avoid confusion

I was thinking the same thing but was concerned about blowing up scope since this was originally wanted within the day. I think I've addressed this.

alenkacz · 2022-12-15T06:45:13Z

src/go/k8s/controllers/redpanda/cluster_controller_configuration.go

@@ -414,7 +414,7 @@ func isSafeToRestart(
 ) bool {
 	configVersions := make(map[int64]bool)
 	for i := range clusterStatus {
-		log.Info(fmt.Sprintf("Node %d is using config version %d", clusterStatus[i].NodeID, clusterStatus[i].ConfigVersion))
+		log.Info(fmt.Sprintf("Node %d is using config version %d", clusterStatus[i].NodeID, clusterStatus[i].ConfigVersion)) // actually pod ordinal


should all this then say pod with ordinal X is using....

I think I was wrong. Removed the comment.

alenkacz

Thanks!

RafalKorepta

I have problem with recommission logic and new feature gate. Other than that LGTM

src/go/k8s/pkg/resources/secret.go

src/go/k8s/pkg/resources/statefulset.go

RafalKorepta · 2022-12-21T14:36:02Z

src/go/k8s/pkg/resources/statefulset_scale.go

@@ -224,25 +228,30 @@ func (r *StatefulSetResource) handleRecommission(ctx context.Context) error {
 		return err
 	}

-	broker, err := getNodeInfoFromCluster(ctx, *r.pandaCluster.Status.DecommissioningNode, adminAPI)
+	broker, err := getNodeInfoFromCluster(ctx, targetOrdinal, adminAPI)


There is problem with this call.

redpanda/src/go/k8s/pkg/resources/statefulset_scale.go

Lines 354 to 368 in ff319d3

// getNodeInfoFromCluster allows to get broker information using the admin API

func getNodeInfoFromCluster(

ctx context.Context, ordinal int32, adminAPI adminutils.AdminAPIClient,

) (*admin.Broker, error) {

brokers, err := adminAPI.Brokers(ctx)

if err != nil {

return nil, fmt.Errorf("could not get the list of brokers for checking decommission: %w", err)

}

for i := range brokers {

if brokers[i].NodeID == int(ordinal) {

return &brokers[i], nil

}

}

return nil, nil

}

The second argument is the Pod ordinal. The for loop will look for node with wrong ID. I think you should swap the getPodNodeIDFromAnnotation with getNodeInfoFromCluster and change targetOrdinal with brokerID as second argument in getNodeInfoFromCluster function.

It would be good to have e2e test that verifies that recommission broker is called. Maybe unit test would be enough to simulate 2 disjoint collections of ids:

POD ordinals - 0,1,2

Broker ID - 7, 8, 9

Unit test would need to assert that RecommissionBroker was called.

src/go/k8s/controllers/redpanda/cluster_controller_configuration.go

alejandroEsc

just one item, otherwise lgtm

alejandroEsc · 2023-01-18T00:48:22Z

src/go/k8s/cmd/configurator/main.go

-// the last number is the index of replica. This index is then propagated
-// to redpanda.node_id.
-func hostIndex(hostName string) (brokerID, error) {
+// getPodOrdinalFromHostname takes advantage of pod naming convention in Kubernetes StatfulSet


Suggested change

// getPodOrdinalFromHostname takes advantage of pod naming convention in Kubernetes StatfulSet

// getPodOrdinalFromHostname takes advantage of Pod naming convention in Kubernetes StatefulSet

joejulian · 2023-06-05T22:15:12Z

closing as it's horribly out-of-date and needs to be redone (if at all).

joejulian added the area/k8s label Dec 1, 2022

joejulian requested a review from a team as a code owner December 1, 2022 00:37

joejulian self-assigned this Dec 1, 2022

joejulian force-pushed the redpanda_use_annotation branch 2 times, most recently from bcba247 to 2d1e2a3 Compare December 2, 2022 07:34

joejulian added 4 commits December 2, 2022 15:47

operator: docs: capitalize Pod and fix typos

dd6d683

separate pod ordinal from broker id

2703e4e

operator: prevent setting maintenance mode on decommissioning clusters

ff319d3

operator: add pod with node annotation for recommission test

b588c51

joejulian force-pushed the redpanda_use_annotation branch from 2d1e2a3 to b588c51 Compare December 2, 2022 23:47

alenkacz previously approved these changes Dec 15, 2022

View reviewed changes

operator: be more specific and call a pod ordinal a pod ordinal

8ed7cdf

joejulian dismissed alenkacz’s stale review via 8ed7cdf December 15, 2022 17:40

joejulian requested a review from alenkacz December 15, 2022 17:43

alenkacz approved these changes Dec 15, 2022

View reviewed changes

RafalKorepta reviewed Dec 21, 2022

View reviewed changes

alejandroEsc reviewed Jan 18, 2023

View reviewed changes

joejulian added needs rebase needs tests labels Mar 1, 2023

joejulian closed this Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: use pod annotation for node_id and avoid mixing maintenance mode with decommissioning #7581

operator: use pod annotation for node_id and avoid mixing maintenance mode with decommissioning #7581

joejulian commented Dec 1, 2022

alenkacz left a comment

alenkacz Dec 15, 2022

joejulian Dec 15, 2022

alenkacz Dec 15, 2022

joejulian Dec 15, 2022

alenkacz Dec 15, 2022

joejulian Dec 15, 2022

alenkacz left a comment

RafalKorepta left a comment

RafalKorepta Dec 21, 2022

RafalKorepta Dec 21, 2022 •

edited

Loading

alejandroEsc left a comment

alejandroEsc Jan 18, 2023

joejulian commented Jun 5, 2023

	// getNodeInfoFromCluster allows to get broker information using the admin API
	func getNodeInfoFromCluster(
	ctx context.Context, ordinal int32, adminAPI adminutils.AdminAPIClient,
	) (*admin.Broker, error) {
	brokers, err := adminAPI.Brokers(ctx)
	if err != nil {
	return nil, fmt.Errorf("could not get the list of brokers for checking decommission: %w", err)
	}
	for i := range brokers {
	if brokers[i].NodeID == int(ordinal) {
	return &brokers[i], nil
	}
	}
	return nil, nil
	}

	// getPodOrdinalFromHostname takes advantage of pod naming convention in Kubernetes StatfulSet
	// getPodOrdinalFromHostname takes advantage of Pod naming convention in Kubernetes StatefulSet

operator: use pod annotation for node_id and avoid mixing maintenance mode with decommissioning #7581

operator: use pod annotation for node_id and avoid mixing maintenance mode with decommissioning #7581

Conversation

joejulian commented Dec 1, 2022

Backports Required

UX Changes

Release Notes

Bug Fixes

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

RafalKorepta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalKorepta Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

alejandroEsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joejulian commented Jun 5, 2023

RafalKorepta Dec 21, 2022 •

edited

Loading