Skip to content
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

✨ support config work driver. #256

Conversation

morvencao
Copy link
Member

@morvencao morvencao commented Mar 26, 2024

Summary

This PR aims to enhance the addon-manager by adding work driver configuration support.
This enables the addon manager to deploy addon agents using drivers other than kube.

Related issue(s)

ref: https://issues.redhat.com/browse/ACM-10423

@openshift-ci openshift-ci bot requested review from deads2k and zhiweiyin318 March 26, 2024 06:58
@morvencao morvencao force-pushed the br_add_work_driver branch from 81f461b to cd70817 Compare March 26, 2024 07:43
@morvencao
Copy link
Member Author

/hold

@morvencao
Copy link
Member Author

/assign @qiujian16 @skeeey

go.mod Outdated
open-cluster-management.io/api v0.13.0
open-cluster-management.io/sdk-go v0.13.0
open-cluster-management.io/ocm v0.13.1-0.20240325044258-22501d88f774
Copy link
Member

Choose a reason for hiding this comment

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

we should avoid import ocm

Copy link
Member Author

Choose a reason for hiding this comment

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

removed ocm dependency.

@morvencao morvencao force-pushed the br_add_work_driver branch 3 times, most recently from 6cafea7 to 0007165 Compare March 28, 2024 08:06
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

can we enable the integration for cloudevents?

}

// New returns a new addon manager for creating addon agents.
func New(config *rest.Config, opts *ManagerOptions) (AddonManager, error) {
Copy link
Member

Choose a reason for hiding this comment

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

this will break the existing signature of the function. the opts filed should be an optional field. And we should allow that it is not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated this to make managerOptions be optional.

@@ -66,22 +66,30 @@ func newCommand() *cobra.Command {
}

func newControllerCommand() *cobra.Command {
o := addonmanager.NewManagerOptions()
Copy link
Member

Choose a reason for hiding this comment

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

we should not impact the existing addon. We could build a new addon example that supports cloudevent, or select one addon as an example. It also ensure that existing addon won't be impacted by this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

built new example that supports cloudevent, following the existing examples.

@morvencao morvencao force-pushed the br_add_work_driver branch 4 times, most recently from f7a589a to 4f7535b Compare April 15, 2024 04:04
Comment on lines 29 to 39
test-kube-integration: ensure-kubebuilder-tools
go test -c ./test/integration/kube -o ./kube-integration.test
./kube-integration.test -ginkgo.slowSpecThreshold=15 -ginkgo.v -ginkgo.failFast
.PHONY: test-kube-integration

test-cloudevents-integration: ensure-kubebuilder-tools
go test -c ./test/integration/cloudevents -o ./cloudevents-integration.test
./cloudevents-integration.test -ginkgo.slowSpecThreshold=15 -ginkgo.v -ginkgo.failFast
.PHONY: test-cloudevents-integration

test-integration: test-kube-integration test-cloudevents-integration
Copy link
Member Author

Choose a reason for hiding this comment

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

created separate folder for cloudevents integration test.
make sense? @qiujian16 @skeeey

Copy link
Member

Choose a reason for hiding this comment

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

@morvencao morvencao force-pushed the br_add_work_driver branch 7 times, most recently from b480f3b to a5ebffe Compare April 16, 2024 02:48
@@ -90,6 +92,9 @@ func newManifestWork(addonNamespace, addonName, clusterName string, manifests []
Labels: map[string]string{
addonapiv1alpha1.AddonLabelKey: addonName,
},
Annotations: map[string]string{
common.CloudEventsDataTypeAnnotationKey: payload.ManifestBundleEventDataType.String(),
Copy link
Member

Choose a reason for hiding this comment

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

We need to add this annotation all the time? I do not think we should add any additionaly annotation to existing manifestwork.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we do need this all the time. /cc @skeeey

Copy link
Member

Choose a reason for hiding this comment

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

we need this annotation to indicate the manifest data type, but I think we may be able to enhance the sdk-go, if we don't find this annotation, we use ManifestBundle as a default value?

Copy link
Member

Choose a reason for hiding this comment

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

and for ocm/acm cases, it should be always ManifestBundle

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can set default value(bundle type) in sdk-go so that we don't need set it here explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

or we can give a fixed value(bundle type) now

Copy link
Member Author

Choose a reason for hiding this comment

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

updated sdk-go dependency, removed this annotation.

// New returns a new Manager for creating addon agents.
func New(config *rest.Config) (AddonManager, error) {
return &addonManager{
// ManagerOptions defines the flags for addon manager
Copy link
Member

Choose a reason for hiding this comment

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

can we put this Option flag in the separated dir specifically for cloudevent?

Copy link
Member Author

Choose a reason for hiding this comment

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

put this to separate cloudevents package.

// ManifestWork client that implements the ManifestWorkInterface and ManifestWork informer based on different
// driver configuration.
// Refer to Event Based Manifestwork proposal in enhancements repo to get more details.
_, config, err := cloudeventswork.NewConfigLoader(a.options.WorkDriver, a.options.WorkDriverConfig).
Copy link
Member

Choose a reason for hiding this comment

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

could we move all this cloudEvent setting to a separated func instead of the main Start func? or a separated cloudEvent manager? I am concerned about this change on the existing addon-framework breaking existing addons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented cloudeventsAddonManager in separate package cloudevents, so that we don't break existing addons. see: dcf9539

@morvencao morvencao force-pushed the br_add_work_driver branch from c5e39cb to dcf9539 Compare April 16, 2024 04:27
@morvencao morvencao force-pushed the br_add_work_driver branch from dcf9539 to da0d4f9 Compare April 16, 2024 07:36
@morvencao
Copy link
Member Author

kindly ping @zhujian7 @zhiweiyin318 ~

@zhiweiyin318
Copy link
Member

Look good to me.

return fmt.Errorf("method StartWithInformers is not implemented")
}

func (a *cloudeventsAddonManager) startWithInformers(ctx context.Context,
Copy link
Member

Choose a reason for hiding this comment

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

actully, how about we have a baseManager interface with StartWithInformers method and an implementation that new all controllers. The Manager interface inlines the baseManager interface, and the manager/cloudeventmanger have different implementation of start func, but share the same implementation in baseManager. Will that work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this, not sure if this is more clear.

Copy link
Member

Choose a reason for hiding this comment

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

why we still need this? I thought this should be in the baseManager.

Copy link
Member Author

@morvencao morvencao Apr 18, 2024

Choose a reason for hiding this comment

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

removed this, PTAL @qiujian16

Copy link
Member

Choose a reason for hiding this comment

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

I mean all the code to init controllers and start them. I think they are duplicated code and should exist only in baseManager. If we have any change on controllers in the future, we do not need to update multiple times.

Copy link
Member Author

Choose a reason for hiding this comment

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

We may not do that, due to the limitation source work client from sdk-go as I mentioned at #256 (comment)

we can't get the workinformers.SharedInformerFactory from clientHolder (from sdk-go), we can only get workv1informers.Informer type, so we can't call StartWithInformers to start the controllers in cloudevents addon manager.

@morvencao morvencao force-pushed the br_add_work_driver branch from a81869c to 2b67a4d Compare April 17, 2024 11:08
Signed-off-by: morvencao <[email protected]>
@morvencao morvencao force-pushed the br_add_work_driver branch from 2b67a4d to 7ad6107 Compare April 18, 2024 01:39
agentBroadcast: clusters/${MANAGED_CLUSTER_NAME}/agentbroadcast
EOF

# patch klusterlet-work-agent deployment to use mqtt as workload source driver
Copy link
Member

Choose a reason for hiding this comment

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

Can the klusterlet-work-agent be deployed in the way that the mqtt is the driver directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the latest work agent image already support mqtt and grpc drivers.

return addonfactory.StructToValues(manifestConfig), nil
}

func AgentHealthProber() *agent.HealthProber {
Copy link
Member

Choose a reason for hiding this comment

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

is there any specific reason that we need to use the work prober to check the health for cloud events driver addons?

Copy link
Member Author

Choose a reason for hiding this comment

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

the cloudevents driver added in this PR is only used for addon agent deploy, no other difference from the current "kube" driver. Therefore, it use similar prober to make sure addon agent is deployed successfully on managed cluster as "kube" driver. Make sense?

}
return nil
}

// New returns a new Manager for creating addon agents.
func New(config *rest.Config) (AddonManager, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it is better to provider a addon manager factory func, like:

func New(mtype AddonManagerType)(AddonManager, error){
  if mtype =="cloudevents"{
    return ...
  }
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

this will break existing code that using addonmanager.New(Config) to create addon manager, right?
we don't want make any change to current code, that's why we keep this unchanged.


// cloudeventsAddonManager is the implementation of AddonManager with
// the base implementation and cloudevents options
type cloudeventsAddonManager struct {
Copy link
Member

Choose a reason for hiding this comment

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

could you explain the difference between the cloudevents addon manager and the classic addon manager except for the building way of manifestwork client and informer? which I think is good for review.

Copy link
Member Author

Choose a reason for hiding this comment

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

the difference between the cloudevents addon manager and the classic addon manager is the way to build work client and work informer.

the cloudevents addon manager use cloudevents drivers(mqtt or grpc) to create work informer, so that the manifestwork used to deploy addon agent can be delivered to managed cluster through mqtt or grpc.

no other difference.

contents: read

jobs:
integration:
Copy link
Member

Choose a reason for hiding this comment

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

can we also add an e2e to at least check that the example cloud event addons are available when deploying into a kubernetes cluster?

Copy link
Member Author

Choose a reason for hiding this comment

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

e2e test was removed in this commit: d8d4f52
because the work client based on cloudevents drivers (sdk-go) are missing GC machnism, we will revisit and add e2e after GC machnism support for cloudevents drivers.

Signed-off-by: morvencao <[email protected]>
@morvencao
Copy link
Member Author

@qiujian16 @zhujian7 Another look?

Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve

workInformers := clientHolder.ManifestWorkInformer()

// addonDeployController
err = workInformers.Informer().AddIndexers(
Copy link
Member

Choose a reason for hiding this comment

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

nit: the indexer part should be put in a shared dir. It is duplicated with the manager.

Copy link
Contributor

openshift-ci bot commented Apr 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: morvencao, qiujian16

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@morvencao
Copy link
Member Author

morvencao commented Apr 19, 2024

added back the e2e testing for helloworld addon using cloudevents.
/unhold

@zhujian7 PTAL

Signed-off-by: morvencao <[email protected]>
@morvencao morvencao force-pushed the br_add_work_driver branch from f61bf8b to c7ba5b9 Compare April 19, 2024 05:47
@zhujian7
Copy link
Member

LGTM

@zhujian7
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 19, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e703fc5 into open-cluster-management-io:main Apr 19, 2024
14 checks passed
@morvencao morvencao deleted the br_add_work_driver branch April 19, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants