Skip to content

🐛 addon fix for Install namespace potential race condition with addondeploymentconfig - For issue: https://github.com/open-cluster-management-io/ocm/issues/1465 #381

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.

Sign up for GitHub

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

Open
tesshuflower wants to merge 2 commits into
base: main
Choose a base branch
from
+316 −14
Open

🐛 addon fix for Install namespace potential race condition with addondeploymentconfig - For issue: https://github.com/open-cluster-management-io/ocm/issues/1465 #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
712d095
fix(utils): guard against race where addon config not yet synced to M…
tesshuflower May 1, 2026
6c7647d
refactor(utils): move race condition guard into GetDesiredAddOnDeploy…
tesshuflower May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions pkg/utils/addon_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"

"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -47,11 +48,6 @@ func AgentInstallNamespaceFromDeploymentConfigFunc(
return "", fmt.Errorf("failed to get deployment config for addon %s: %v", addon.Name, err)
}

// For now, we have no way of knowing if the addon depleoyment config is not configured, or
// is configured but not yet been added to the managedclusteraddon status config references,
// we expect no error will be returned when the addon deployment config is not configured
// so we can use the default namespace.
// TODO: Find a way to distinguish between the above two cases
if config == nil {
klog.V(4).InfoS("Addon deployment config is nil, return an empty string for agent install namespace",
"addonNamespace", addon.Namespace, "addonName", addon.Name)
Expand All @@ -71,6 +67,14 @@ func GetDesiredAddOnDeploymentConfig(
ok, configRef := GetAddOnConfigRef(addon.Status.ConfigReferences,
AddOnDeploymentConfigGVR.Group, AddOnDeploymentConfigGVR.Resource)
if !ok {
// If the addon declares support for addondeploymentconfigs but the Configured condition
// is not yet True, the addonconfiguration controller either hasn't processed this MCA
// or hasn't finished rolling out configs. In either case configReferences may be
// incomplete. Return an error so callers retry rather than proceeding with no config.
if addonSupportsDeploymentConfig(addon) && !addonConfiguredTrue(addon) {
return nil, fmt.Errorf("addon %s supports addondeploymentconfigs but Configured condition is not True yet, need to retry",

@qiujian16 qiujian16 May 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we have an integration test on this? Also if it returns err, the controller will backoff upon error. Instead of returning an error, should we return a state that "nothing is configured yet" so the caller will know nothing should be handled, and when addon status is updated, this func will be triggered again.

@tesshuflower tesshuflower May 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a concern I mentioned in my description as well - the issue is the function AgentInstallNamespaceFromDeploymentConfigFunc() is essentially a helper function provided by the addon-framework that addons are already using to get the namespace from the deploymentconfig - Should I consider deprecating it and making a new function instead that can return the extra status?

@tesshuflower tesshuflower May 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@qiujian16 What do you think about my comment above? My main concern with changing the function signature is this is a function already called by users of the addon-framework - If I want to return another parameter to tell them to retry, this necessitates a change in the function or perhaps a new one. Do you have a preference here?

@qiujian16 qiujian16 May 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes I think it makes sense.

@tesshuflower tesshuflower Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@qiujian16 @haoqing0110 I've come back to this one, and was going through changes required to add new functions to be able to get the namespace (with retries) - however there are a lot of cascading affected places.

For example addon framework interface functions would need updating too, example:

addon-framework/pkg/agent/interface.go

Line 73 in 929daf0

AgentInstallNamespace AgentInstallNamespaceFunc

And then even from getValues functions like here:

addon-framework/pkg/addonfactory/helm_agentaddon.go

Line 170 in 929daf0

func (a *HelmAgentAddon) getValues(

Essentially all of those don't currently have a strict retry mechanism and would need to change or get updated.

But then I found we actually have this ConfigCheckEnabled setting already:

addon-framework/pkg/agent/interface.go

Line 112 in 929daf0

ConfigCheckEnabled bool

And this is used in a couple places before calling the agent Manifests() function.

Do you think perhaps I should pivot to a fix that simply uses ConfigCheckEnabled instead? Ultimately it's the same exact guard - it checks for configured=true in the status before proceeding. I feel like my current fix in this PR is maybe bypassing the `ConfigCheckEnabled as well as getting overly complicated.

Essentially a fix would be to make sure we do something similar to this:

https://github.com/open-cluster-management-io/addon-framework/blob/main/pkg/addonmanager/controllers/agentdeploy/controller.go#L409-L413

Anytime before the getAgentNamespace (of even agent Manifests()) is called. This would essentially give us the same protection, but would only be turned on when users set ConfigCheckEnabled.

Sorry the above is very long, I've made an alternative draft PR here to demonstrate the changes required with this approach: #382

The comments on the original function with the race condition attempt to explain it as well: https://github.com/open-cluster-management-io/addon-framework/pull/382/changes#diff-0268a17aba5249e6bd421172ca6ac3c9c0e93ef819030b1cba41afbe50d89743

@tesshuflower tesshuflower Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@qiujian16 and @haoqing0110 - sorry do you mind taking a look again at the above? Would you be ok with the alternative approach in #382 ?

addon.Name)
}
return nil, nil
}

Expand Down Expand Up @@ -134,3 +138,17 @@ func GetAddOnConfigRef(

return false, addonapiv1beta1.ConfigReference{}
}

func addonSupportsDeploymentConfig(addon *addonapiv1beta1.ManagedClusterAddOn) bool {
for _, sc := range addon.Status.SupportedConfigs {

@haoqing0110 haoqing0110 May 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

addon.Status.SupportedConfigs is for addon users to know to supported config type. In code level, https://github.com/open-cluster-management-io/addon-framework/blob/main/pkg/addonmanager/controllers/addonconfig/controller.go#L177 , should check the configGVRs.

@tesshuflower tesshuflower May 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this @haoqing0110 - I guess I was having trouble seeing why the configGVRs would get loaded in this code - but using Status.SupportedConfigs has a similar timing issue as it's something else that's setting that status.

I think in the end, are you ok if we use configured=True condition to decide that the configReferences are ready to be consumed? It seems that this is the correct condition to check.

However, there is the one caveat that where a ClusterManagementAddOn may have this annotation: addon.open-cluster-management.io/lifecycle: "self" - in this case, the external controller does not ever set configured=True condition.

Do you think this is a concern? I think with v1beta1 this is less of a concern since there is no such idea of "supportedConfigs" anymore - and if the annotation is set by a user, do we not expect them to call AgentInstallNamespaceFromDeploymentConfigFunc() at all?

@haoqing0110 haoqing0110 May 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tesshuflower I think it's good to check the condition configured=True. And after migrating to v1beta1, the code won't have the annotation any more. open-cluster-management-io/ocm#1428

if sc.Group == AddOnDeploymentConfigGVR.Group && sc.Resource == AddOnDeploymentConfigGVR.Resource {
return true
}
}
return false
}

func addonConfiguredTrue(addon *addonapiv1beta1.ManagedClusterAddOn) bool {
cond := meta.FindStatusCondition(addon.Status.Conditions, addonapiv1beta1.ManagedClusterAddOnConditionConfigured)
return cond != nil && cond.Status == metav1.ConditionTrue
}
302 changes: 293 additions & 9 deletions pkg/utils/addon_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ func newTestAddOnDeploymentConfigGetter(adc *addonapiv1beta1.AddOnDeploymentConf
func TestAgentInstallNamespaceFromDeploymentConfigFunc(t *testing.T) {

cases := []struct {
name string
getter AddOnDeploymentConfigGetter
mca *addonapiv1beta1.ManagedClusterAddOn
expected string
name string
getter AddOnDeploymentConfigGetter
mca *addonapiv1beta1.ManagedClusterAddOn
expected string
expectError bool
}{
{
name: "addon is nil",
Expand All @@ -40,8 +41,9 @@ func TestAgentInstallNamespaceFromDeploymentConfigFunc(t *testing.T) {
},
Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{},
}),
mca: nil,
expected: "",
mca: nil,
expected: "",
expectError: true,
},
{
name: "addon no deployment config reference",
Expand Down Expand Up @@ -93,7 +95,8 @@ func TestAgentInstallNamespaceFromDeploymentConfigFunc(t *testing.T) {
},
},
},
expected: "",
expected: "",
expectError: true,
},
// {
// name: "addon deployment config reference spec hash not match",
Expand Down Expand Up @@ -165,13 +168,294 @@ func TestAgentInstallNamespaceFromDeploymentConfigFunc(t *testing.T) {
},
expected: "testns",
},
{
name: "addon supports deployment config but Configured condition absent - should requeue",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
},
Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{
AgentInstallNamespace: "custom-ns",
},
}),
mca: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
Namespace: "cluster1",
},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{
{
Group: "addon.open-cluster-management.io",
Resource: "addondeploymentconfigs",
},
},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
},
},
expected: "",
expectError: true,
},
{
name: "addon supports deployment config but Configured condition is False - should requeue",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
},
Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{
AgentInstallNamespace: "custom-ns",
},
}),
mca: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
Namespace: "cluster1",
},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{
{
Group: "addon.open-cluster-management.io",
Resource: "addondeploymentconfigs",
},
},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
Conditions: []metav1.Condition{
{
Type: addonapiv1beta1.ManagedClusterAddOnConditionConfigured,
Status: metav1.ConditionFalse,
Reason: "ConfigurationsNotConfigured",
},
},
},
},
expected: "",
expectError: true,
},
{
name: "addon supports deployment config and Configured=True but no config exists - use default",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
},
Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{},
}),
mca: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
Namespace: "cluster1",
},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{
{
Group: "addon.open-cluster-management.io",
Resource: "addondeploymentconfigs",
},
},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
Conditions: []metav1.Condition{
{
Type: addonapiv1beta1.ManagedClusterAddOnConditionConfigured,
Status: metav1.ConditionTrue,
Reason: "ConfigurationsConfigured",
},
},
},
},
expected: "",
expectError: false,
},
{
name: "addon does not support deployment config and no config - use default",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
},
Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{},
}),
mca: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{
Name: "test1",
Namespace: "cluster1",
},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
},
},
expected: "",
expectError: false,
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
nsFunc := AgentInstallNamespaceFromDeploymentConfigFunc(c.getter)
ns, _ := nsFunc(context.TODO(), c.mca)
assert.Equal(t, c.expected, ns, "should be equal")
ns, err := nsFunc(context.TODO(), c.mca)
assert.Equal(t, c.expected, ns, "namespace should be equal")
if c.expectError {
assert.Error(t, err, "should return error")
} else {
assert.NoError(t, err, "should not return error")
}
})
}
}

func TestGetDesiredAddOnDeploymentConfig(t *testing.T) {
cases := []struct {
name string
getter AddOnDeploymentConfigGetter
addon *addonapiv1beta1.ManagedClusterAddOn
expectNil bool
expectError bool
}{
{
name: "no config ref and addon does not support deployment config - return nil",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{}),
addon: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{Name: "test1", Namespace: "cluster1"},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
ConfigReferences: []addonapiv1beta1.ConfigReference{},
},
},
expectNil: true,
},
{
name: "no config ref, supports deployment config, Configured absent - error to retry",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{}),
addon: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{Name: "test1", Namespace: "cluster1"},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{
{Group: "addon.open-cluster-management.io", Resource: "addondeploymentconfigs"},
},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
},
},
expectNil: true,
expectError: true,
},
{
name: "no config ref, supports deployment config, Configured=False - error to retry",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{}),
addon: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{Name: "test1", Namespace: "cluster1"},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{
{Group: "addon.open-cluster-management.io", Resource: "addondeploymentconfigs"},
},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
Conditions: []metav1.Condition{
{
Type: addonapiv1beta1.ManagedClusterAddOnConditionConfigured,
Status: metav1.ConditionFalse,
Reason: "ConfigurationsNotConfigured",
},
},
},
},
expectNil: true,
expectError: true,
},
{
name: "no config ref, supports deployment config, Configured=True - nil config is authoritative",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{}),
addon: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{Name: "test1", Namespace: "cluster1"},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
SupportedConfigs: []addonapiv1beta1.ConfigGroupResource{
{Group: "addon.open-cluster-management.io", Resource: "addondeploymentconfigs"},
},
ConfigReferences: []addonapiv1beta1.ConfigReference{},
Conditions: []metav1.Condition{
{
Type: addonapiv1beta1.ManagedClusterAddOnConditionConfigured,
Status: metav1.ConditionTrue,
Reason: "ConfigurationsConfigured",
},
},
},
},
expectNil: true,
expectError: false,
},
{
name: "config ref with valid spec hash - returns config",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{Name: "test1"},
Spec: addonapiv1beta1.AddOnDeploymentConfigSpec{
AgentInstallNamespace: "custom-ns",
},
}),
addon: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{Name: "test1", Namespace: "cluster1"},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
ConfigReferences: []addonapiv1beta1.ConfigReference{
{
ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{
Group: "addon.open-cluster-management.io",
Resource: "addondeploymentconfigs",
},
DesiredConfig: &addonapiv1beta1.ConfigSpecHash{
ConfigReferent: addonapiv1beta1.ConfigReferent{Name: "test1"},
SpecHash: "f97b3f6af1f786ec6f3273e2d6fc8717e45cb7bc9797ba7533663a7de84a5538",
},
},
},
},
},
expectNil: false,
expectError: false,
},
{
name: "config ref with empty spec hash - error",
getter: newTestAddOnDeploymentConfigGetter(
&addonapiv1beta1.AddOnDeploymentConfig{
ObjectMeta: metav1.ObjectMeta{Name: "test1"},
}),
addon: &addonapiv1beta1.ManagedClusterAddOn{
ObjectMeta: metav1.ObjectMeta{Name: "test1", Namespace: "cluster1"},
Status: addonapiv1beta1.ManagedClusterAddOnStatus{
ConfigReferences: []addonapiv1beta1.ConfigReference{
{
ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{
Group: "addon.open-cluster-management.io",
Resource: "addondeploymentconfigs",
},
DesiredConfig: &addonapiv1beta1.ConfigSpecHash{
ConfigReferent: addonapiv1beta1.ConfigReferent{Name: "test1"},
},
},
},
},
},
expectNil: true,
expectError: true,
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
config, err := GetDesiredAddOnDeploymentConfig(c.addon, c.getter)
if c.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
if c.expectNil {
assert.Nil(t, config)
} else {
assert.NotNil(t, config)
}
})
}
}
Loading