-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add a controller to support addon template #187
Add a controller to support addon template #187
Conversation
9c3af5b
to
2f182f7
Compare
bec3355
to
61380f8
Compare
61380f8
to
05e4473
Compare
kubeConfig, | ||
hubKubeClient, | ||
addonClient, | ||
addonInformerFactory.Addon().V1alpha1().ClusterManagementAddOns(), |
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.
Does this controller need a filter? Considering the case when cma is configured with addontemplate but claim managed by self by annotation addon.open-cluster-management.io/lifecycle: "self"
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.
addonConfigController and managementAddonConfigController should be considered to filter too in self case ?
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.
discussed with haoqing offline, the utils.FilterOutTheBuiltInAddOnConfigGVRs(a.addonConfigs)
will do the filter. so I think self filter is not necessary.
pkg/utils/common.go
Outdated
return false | ||
} | ||
|
||
for _, config := range cma.Spec.SupportedConfigs { |
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.
Today the cma.Spec.SupportedConfigs is more like a global configs for all the addons. The actual addon supported config is hard code with WithConfigGVRs()
and print in mca.Status.SupportedConfigs.
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.
for the template type addon, the developers do not write code for the hub side controller, so they do not have a chance to define the WithConfigGVRs
, we need to check the cma.spec.supportedConfigs
field to determine whether an addon is a template type addon.
name: hello-template-agent | ||
namespace: open-cluster-management-agent-addon | ||
annotations: | ||
"addon.open-cluster-management.io/deletion-orphan": "" |
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.
the annotation deletion-orphan still can work in addonTemplate?
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, it can work since the template controller just start a go routine to run the framework code.
kubeConfig, | ||
hubKubeClient, | ||
addonClient, | ||
addonInformerFactory.Addon().V1alpha1().ClusterManagementAddOns(), |
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.
addonConfigController and managementAddonConfigController should be considered to filter too in self case ?
26b7d23
to
34ed983
Compare
Signed-off-by: zhujian <jiazhu@redhat.com>
Signed-off-by: zhujian <jiazhu@redhat.com>
Signed-off-by: zhujian <jiazhu@redhat.com>
Signed-off-by: zhujian <jiazhu@redhat.com>
34ed983
to
76114d5
Compare
Signed-off-by: zhujian <jiazhu@redhat.com>
/cc @qiujian16 PTAL |
@zhujian7: GitHub didn't allow me to request PR reviews from the following users: PTAL. Note that only open-cluster-management-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold |
/unhold |
fcf27c5
to
1d10946
Compare
Signed-off-by: zhujian <jiazhu@redhat.com>
1d10946
to
6a1a57a
Compare
/approve @zhiweiyin318 @haoqing0110 Please also take a look |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhujian7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/addonmanager/manager.go
Outdated
dynamicInformers, | ||
a.addonConfigs, | ||
utils.FilterOutTheBuiltInAddOnConfigGVRs(a.addonConfigs), |
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.
May need to consider the case when the addon upgrades the framework but still wants to be managed by self, which component is responsible to update the spec hash and lastobservedgeneration for addondeploymentconfig?
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.
deleted the addonconfig controller in the addon manager, will use the controller in addon-framework to update the spec hash for both addon templates and addon deployment config.
Signed-off-by: zhujian <jiazhu@redhat.com>
5b8f4bf
to
133f697
Compare
/lgtm |
/unhold |
0056b14
into
open-cluster-management-io:main
) | ||
managementAddonConfigController = managementaddonconfig.NewManagementAddonConfigController( | ||
addonClient, | ||
addonInformers.Addon().V1alpha1().ClusterManagementAddOns(), | ||
dynamicInformers, | ||
a.addonConfigs, | ||
utils.FilterByAddonName(a.addonAgents), |
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.
filter by the addon name so the controller will only handle its own clustermanagementaddon.
implement for the proposal: open-cluster-management-io/enhancements#87