✨ run work and registration as a single binary #201

qiujian16 · 2023-06-28T09:15:36Z

Summary

This is start the registration/work together with registration-operator agent command

Related issue(s)

Fixes #

openshift-ci · 2023-06-28T09:15:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: ldpliu <daliu@redhat.com>

codecov · 2023-07-04T06:21:11Z

Codecov Report

Patch coverage: 61.38% and project coverage change: +0.37 🎉

Comparison is base (c8ec750) 59.91% compared to head (788b5da) 60.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #201      +/-   ##
==========================================
+ Coverage   59.91%   60.28%   +0.37%     
==========================================
  Files         128      130       +2     
  Lines       13241    13494     +253     
==========================================
+ Hits         7933     8135     +202     
- Misses       4568     4609      +41     
- Partials      740      750      +10

Flag	Coverage Δ
unit	`60.28% <61.38%> (+0.37%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...stermanagercontroller/clustermanager_controller.go	`54.14% <0.00%> (ø)`
...atuscontroller/clustermanager_status_controller.go	`79.59% <0.00%> (ø)`
pkg/registration/clientcert/certificate.go	`42.61% <0.00%> (ø)`
...gistration/spoke/managedcluster/claim_reconcile.go	`71.73% <0.00%> (ø)`
pkg/registration/spoke/spokeagent.go	`13.22% <23.21%> (-17.68%)`	⬇️
...sterletcontroller/klusterlet_cleanup_controller.go	`55.10% <60.00%> (+0.41%)`	⬆️
pkg/registration/spoke/options.go	`61.11% <61.11%> (ø)`
...usterletcontroller/klusterlet_runtime_reconcile.go	`56.93% <65.51%> (-0.09%)`	⬇️
pkg/operator/helpers/queuekey.go	`84.00% <75.00%> (+14.39%)`	⬆️
...llers/ssarcontroller/klusterlet_ssar_controller.go	`77.05% <75.00%> (ø)`
... and 5 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Jian Qiu <jqiu@redhat.com>

zhiweiyin318 · 2023-07-10T15:17:00Z

manifests/klusterlet/management/klusterlet-agent-deployment.yaml

+          requests:
+            cpu: 2m
+            memory: 16Mi
+      volumes:


should have spoke-kubeconfig-secret volume for Hosted mode ?
https://github.com/open-cluster-management-io/ocm/blob/main/manifests/klusterlet/management/klusterlet-registration-deployment.yaml#L118-L121

it is not supported in hosted mode, if we want hosted mode, need another mode

zhiweiyin318 · 2023-07-10T15:44:06Z

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go

@@ -174,6 +179,9 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 		ExternalManagedKubeConfigWorkSecret:         helpers.ExternalManagedKubeConfigWork,
 		InstallMode:                                 klusterlet.Spec.DeployOption.Mode,
 		HubApiServerHostAlias:                       klusterlet.Spec.HubApiServerHostAlias,
+
+		RegistrationServiceAccount: serviceAccountName("registration-sa", klusterlet),
+		WorkServiceAccount:         serviceAccountName("work-sa", klusterlet),


in InstallModeSingleton, the registration and work sa have the same name %s-agent-sa , currently it's ok because they have the same content in the yaml. but it may bring some confusion, and in the future if someone change one of them, there may meet issue.
could we consider to define a new sa yaml for Singleton mode?

it will need to check the mode for each rolebinding and clusterrolebinding files. I think it is harder to maintain then just mutate the service account name.

manifests/klusterlet/management/klusterlet-agent-deployment.yaml

zhujian7 · 2023-07-12T02:47:54Z

pkg/cmd/spoke/operator.go

+	features.SpokeMutableFeatureGate.AddFlag(flags)
+
+	// add disable leader election flag
+	flags.BoolVar(&cmdConfig.DisableLeaderElection, "disable-leader-election", false, "Disable leader election for the agent.")


is this a common option？

no, it is a flag provided by library-go, we cannot set in the common options.

...erator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_runtime_reconcile.go

pkg/singleton/spoke/agent.go

zhujian7 · 2023-07-12T03:28:26Z

pkg/singleton/spoke/agent.go

+
+	workCfg := work.NewWorkAgentConfig(a.agentOption, a.workOption)
+	// start work agent at first
+	go func() {


can the work agent and registration agent share some informers？

they do not share the same informer.

test/integration/operator/klusterlet_singleton_test.go

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 · 2023-07-13T01:24:26Z

/assign @skeeey

skeeey · 2023-07-13T02:25:42Z

pkg/common/options/options.go

@@ -33,6 +60,10 @@ func (o *AgentOptions) AddFlags(flags *pflag.FlagSet) {
 	_ = flags.MarkDeprecated("cluster-name", "use spoke-cluster-name flag")
 	flags.StringVar(&o.SpokeClusterName, "cluster-name", o.SpokeClusterName,
 		"Name of the spoke cluster.")
+	flags.StringVar(&o.HubKubeconfigDir, "hub-kubeconfig-dir", o.HubKubeconfigDir,


I think this may have some confusion with hubkubeconfigfile, this dir is used to store the hubkubeconfig certs, could we rename it to hubkubeconfigsecretdir/hubkubeconfigcertdir

~~and we may allow this empty, if empty, we will find the certs in the dir of hubkubeconfigfile?~~

we allow hubkubeconfigfile empty

hrm, I thought we also need kubeconfig to put in the same directory in registration agent?

zhiweiyin318 · 2023-07-14T02:45:22Z

/lgtm
/hold

skeeey · 2023-07-14T02:53:34Z

/lgtm

skeeey · 2023-07-14T02:53:55Z

/unhold

openshift-ci bot requested review from skeeey and zhiweiyin318 June 28, 2023 09:15

openshift-ci bot added the approved label Jun 28, 2023

qiujian16 changed the title ~~✨ run work and registration as a single binary~~ [wip]✨ run work and registration as a single binary Jun 28, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Jun 28, 2023

elgnay pushed a commit to elgnay/ocm that referenced this pull request Jun 29, 2023

update min tls to 1.13 (open-cluster-management-io#201)

3cc0d80

Signed-off-by: ldpliu <daliu@redhat.com>

qiujian16 force-pushed the singleton branch 3 times, most recently from aa9f250 to 4ad212e Compare July 3, 2023 01:41

openshift-merge-robot added the needs-rebase label Jul 3, 2023

qiujian16 force-pushed the singleton branch from 4ad212e to 9fb999c Compare July 3, 2023 06:16

openshift-merge-robot removed the needs-rebase label Jul 3, 2023

qiujian16 force-pushed the singleton branch from 9fb999c to 118b1bc Compare July 3, 2023 06:20

openshift-merge-robot added the needs-rebase label Jul 4, 2023

qiujian16 force-pushed the singleton branch from 705a57e to 399698d Compare July 4, 2023 06:13

openshift-merge-robot removed the needs-rebase label Jul 4, 2023

qiujian16 force-pushed the singleton branch from 399698d to 3206ac3 Compare July 4, 2023 06:21

qiujian16 added 3 commits July 6, 2023 12:24

run registratin/work together

a2ab0f3

Signed-off-by: Jian Qiu <jqiu@redhat.com>

Fix integration test and lint issue

05b2a1e

Signed-off-by: Jian Qiu <jqiu@redhat.com>

Update operator to deploy singleton mode

81c6ed6

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 force-pushed the singleton branch 8 times, most recently from 7939442 to e989bfe Compare July 7, 2023 08:39

qiujian16 force-pushed the singleton branch from e989bfe to 22bcf99 Compare July 10, 2023 04:08

qiujian16 changed the title ~~[wip]✨ run work and registration as a single binary~~ ✨ run work and registration as a single binary Jul 10, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 10, 2023

zhiweiyin318 reviewed Jul 10, 2023

View reviewed changes

zhiweiyin318 reviewed Jul 11, 2023

View reviewed changes

manifests/klusterlet/management/klusterlet-agent-deployment.yaml Outdated Show resolved Hide resolved

zhujian7 reviewed Jul 12, 2023

View reviewed changes

Update deps

788b5da

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 force-pushed the singleton branch from 22bcf99 to 788b5da Compare July 12, 2023 06:15

openshift-ci bot assigned skeeey Jul 13, 2023

skeeey reviewed Jul 13, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Jul 14, 2023

openshift-ci bot assigned zhiweiyin318 Jul 14, 2023

openshift-ci bot added the lgtm label Jul 14, 2023

openshift-ci bot removed the do-not-merge/hold label Jul 14, 2023

openshift-merge-robot merged commit f7cd140 into open-cluster-management-io:main Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ run work and registration as a single binary #201

✨ run work and registration as a single binary #201

qiujian16 commented Jun 28, 2023

openshift-ci bot commented Jun 28, 2023

codecov bot commented Jul 4, 2023 •

edited

Loading

zhiweiyin318 Jul 10, 2023

qiujian16 Jul 11, 2023

zhiweiyin318 Jul 10, 2023

qiujian16 Jul 12, 2023

zhujian7 Jul 12, 2023

qiujian16 Jul 12, 2023

zhujian7 Jul 12, 2023

qiujian16 Jul 12, 2023

qiujian16 commented Jul 13, 2023

skeeey Jul 13, 2023 •

edited

Loading

qiujian16 Jul 13, 2023

zhiweiyin318 commented Jul 14, 2023

skeeey commented Jul 14, 2023

skeeey commented Jul 14, 2023

✨ run work and registration as a single binary #201

✨ run work and registration as a single binary #201

Conversation

qiujian16 commented Jun 28, 2023

Summary

Related issue(s)

openshift-ci bot commented Jun 28, 2023

codecov bot commented Jul 4, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

qiujian16 commented Jul 13, 2023

skeeey Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiweiyin318 commented Jul 14, 2023

skeeey commented Jul 14, 2023

skeeey commented Jul 14, 2023

codecov bot commented Jul 4, 2023 •

edited

Loading

skeeey Jul 13, 2023 •

edited

Loading