Skip to content

annotate broker on config change#672

Merged
RafalKorepta merged 3 commits intorelease/v2.3.xfrom
ss-annotate-broker-on-config-change
Apr 14, 2025
Merged

annotate broker on config change#672
RafalKorepta merged 3 commits intorelease/v2.3.xfrom
ss-annotate-broker-on-config-change

Conversation

@simon0191
Copy link
Member

Annotate brokers with cluster config version to force a restart on config changes

Refs

@simon0191 simon0191 changed the base branch from main to release/v2.3.x April 8, 2025 10:55
@simon0191 simon0191 force-pushed the ss-annotate-broker-on-config-change branch 2 times, most recently from b25443e to 20c0cdc Compare April 8, 2025 11:32
@simon0191 simon0191 requested a review from jan-g April 8, 2025 12:14
@simon0191 simon0191 force-pushed the ss-annotate-broker-on-config-change branch from 20c0cdc to a5cf19e Compare April 8, 2025 12:16
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see one important test missing. When you have StatefulSet it would be good to assert in the test you changed that Pod generation roll over.

@simon0191 simon0191 requested a review from alenkacz April 8, 2025 13:12
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrongly guide you. Pods does not have generation. Higher level resources like StatefulSet and Deployment have such generation. Pods have only labels and resourceVersion and uid in metadata.

Maybe only one change I would make is to not change the assertion, but rather when the Condition is set to True. As far as I understand

	syncer := syncclusterconfig.Syncer{Client: client, Mode: syncclusterconfig.SyncerModeAdditive}
	configVersion, err := syncer.Sync(ctx, config, usersTXT)
	if err != nil {
		return errors.WithStack(err)
	}

	apimeta.SetStatusCondition(rp.GetConditions(), metav1.Condition{
		Type:               redpandav1alpha2.ClusterConfigSynced,
		Status:             metav1.ConditionTrue,
		ObservedGeneration: rp.Generation,
		Reason:             "ConfigSynced",
		Message:            fmt.Sprintf("ClusterConfig at Version %d", configVersion),
	})

will set the condition when Pods did not restart. Am I right?

Copy link
Contributor

@jan-g jan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the forced restart by applying a new/changed configuration doesn't cause a config version bump (I don't think it does, having just tried it) I think this approach is okay.

Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll want to include this in main and backport it to v2.4.x and v2.3.x? WDYT?

Implementation LGTM, just a couple things to fix up in tests.

@simon0191 simon0191 force-pushed the ss-annotate-broker-on-config-change branch 2 times, most recently from bb40b20 to 9057a75 Compare April 11, 2025 18:38
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! You need to run task lint -- --fix to get the linter to approve you.

@simon0191 simon0191 force-pushed the ss-annotate-broker-on-config-change branch from 9057a75 to de64fab Compare April 14, 2025 14:19
@simon0191
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
release/v2.4.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

1 similar comment
@simon0191
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
release/v2.4.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@simon0191
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
release/v2.4.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@simon0191 simon0191 mentioned this pull request Apr 14, 2025
@RafalKorepta RafalKorepta deleted the ss-annotate-broker-on-config-change branch April 15, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants