add poolcoordinator controller and webhooks to replace nodelifecycle … #1040

LindaYu17 · 2022-10-29T14:10:38Z

…controller

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage

What this PR does / why we need it:

replace with nodelifecycle controller from k8s with pool-coordinator's pod management.

Which issue(s) this PR fixes:

Fixes #776

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot · 2022-10-29T14:10:43Z

@LindaYu17: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

…controller

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage

What this PR does / why we need it:

replace with nodelifecycle controller from k8s with pool-coordinator's pod management.

Which issue(s) this PR fixes:

Fixes #776

Special notes for your reviewer:

Does this PR introduce a user-facing change?
other Note

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.

openyurt-bot · 2022-10-29T14:10:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LindaYu17
To complete the pull request process, please assign kadisi
You can assign the PR to them by writing /assign @kadisi in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

luc99hen · 2022-10-30T16:14:48Z

It seems that this controller follows the code style of kubebuilder. If this is fine, I will build my cert management part based on this framework. @rambohe-ch

rambohe-ch · 2022-10-31T02:40:15Z

It seems that this controller follows the code style of kubebuilder. If this is fine, I will build my cert management part based on this framework. @rambohe-ch

@LindaYu17 @luc99hen I think that newly added controllers should follow the code style of kube-controller-manager instead of kubebuilder.

rambohe-ch · 2022-10-31T02:58:31Z

pkg/controller/poolcoordinator/client/client.go

@@ -0,0 +1,41 @@
+package client


please add license header for newly added file.

rambohe-ch · 2022-10-31T03:01:45Z

pkg/controller/poolcoordinator/webhook/pod_admission.go

+}
+
+// extracts pod from admission request
+func (pv *PodAdmission) getPod() error {


rambohe-ch · 2022-10-31T03:10:53Z

pkg/controller/poolcoordinator/utils/nodepool.go

+		klog.Error(err)
+		return false
+	}
+	diff := time.Now().Sub(lease.GetCreationTimestamp().Time)


we should use lease.Spec.RenewTime instead of GetCreationTimestamp() to specify the last heartbeat time

rambohe-ch · 2022-10-31T03:19:58Z

pkg/controller/poolcoordinator/webhook/pod_admission.go

+		return reviewResponse(pv.request.UID, false, http.StatusBadRequest, e), err
+	}
+
+	if pv.pod.Annotations == nil || pv.pod.Annotations[constant.PodAvailableAnnotation] != "true" {


toleration should also be added to pods for node with autonomy annotation.

rambohe-ch · 2022-10-31T03:22:03Z

pkg/controller/poolcoordinator/utils/nodepool.go

+
+func NewNodepoolMap() *NodepoolMap {
+	return &NodepoolMap{
+		nodepools: make(map[string]*NodeSet),


how about use sets.String instead of defining NodeSet?

codecov · 2022-10-31T14:40:16Z

Codecov Report

❗ No coverage uploaded for pull request base (pool-coordinator-dev@0db2b87). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 2e7024e differs from pull request most recent head 1f25094. Consider uploading reports for the commit 1f25094 to get more accurate results

@@                   Coverage Diff                   @@
##             pool-coordinator-dev    #1040   +/-   ##
=======================================================
  Coverage                        ?   49.29%           
=======================================================
  Files                           ?      101           
  Lines                           ?    12201           
  Branches                        ?        0           
=======================================================
  Hits                            ?     6015           
  Misses                          ?     5662           
  Partials                        ?      524

Flag	Coverage Δ
unittests	`49.29% <0.00%> (?)`

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rambohe-ch · 2022-11-02T02:23:21Z

pkg/controller/poolcoordinator/poolcoordinator_controller.go

+	"github.com/openyurtio/openyurt/pkg/controller/poolcoordinator/constant"
+	"github.com/openyurtio/openyurt/pkg/controller/poolcoordinator/lister"
+	"github.com/openyurtio/openyurt/pkg/controller/poolcoordinator/utils"
+	"github.com/openyurtio/openyurt/pkg/controller/poolcoordinator/webhook"


lib of openyurt should be put at the last and independent section.

rambohe-ch · 2022-11-02T02:35:31Z

pkg/controller/poolcoordinator/lister/lister.go

+func CreateNodeInformer(client client.Interface, factory informers.SharedInformerFactory, stopper chan (struct{})) v1.NodeInformer {
+	nodeInformer := factory.Core().V1().Nodes()
+	factory.Start(stopper)
+	factory.WaitForCacheSync(stopper)


we can not start informer at here, because all of informers will be started at https://github.com/openyurtio/openyurt/blob/master/cmd/yurt-controller-manager/app/controllermanager.go#L208

and factory.WaitForCacheSync(stopper) should be executed in a independent goroutine, you can take YurtCSRApprover controller as a example: https://github.com/openyurtio/openyurt/blob/master/pkg/controller/certificates/csrapprover.go#L181

luc99hen · 2022-11-02T14:59:37Z

pkg/controller/poolcoordinator/poolcoordinator_controller.go

+	nc.nodeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    onNodeCreate,
+		UpdateFunc: onNodeUpdate,
+		DeleteFunc: onNodeDelete,
+	})
+	nc.leaseInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    onLeaseCreate,
+		UpdateFunc: onLeaseUpdate,
+		DeleteFunc: nil,
+	})


How about moving these configurations to the NewController func

rambohe-ch · 2022-11-11T11:41:01Z

pkg/controller/poolcoordinator/poolcoordinator_controller.go

+}
+
+var (
+	ctl *Controller


please do not define Controller var.

rambohe-ch · 2022-11-11T11:41:13Z

pkg/controller/poolcoordinator/poolcoordinator_controller.go

+var (
+	ctl *Controller
+
+	ldc *LeaseDelegatedCounter


rambohe-ch · 2022-11-11T11:59:14Z

pkg/controller/poolcoordinator/poolcoordinator_controller.go

+	go pcc.nodePoolWorker()
+
+	klog.Info("create webhook")
+	go webhook.Run(pcc.nodeLister, pcc.leaseLister, pcc.nodepoolMap)


maybe it's hard to understand that run webhook in the controller, how about create an independent webhook server for all webhooks in yurt-controller-manager? so it's easy to add other webhooks in the new server in the future.

rambohe-ch · 2022-11-11T12:05:43Z

pkg/controller/poolcoordinator/poolcoordinator_controller.go

+		ldc.Inc(nl.Name)
+		if ldc.Counter(nl.Name) >= constant.LeaseDelegationThreshold {
+			item.taint = true
+			GetController().nodeUpdateQueue.Add(item)


in order to improve the readability of code, maybe we can add node name instead nodeTaintItem into nodeUpdateQueue.

rambohe-ch · 2022-11-11T12:08:23Z

pkg/controller/poolcoordinator/webhook/pod_admission.go

+
+	nodeLister  listerv1.NodeLister
+	leaseLister leaselisterv1.LeaseNamespaceLister
+	nodepoolMap *utils.NodepoolMap


we should try best not to define file-scope var.

huiwq1990 · 2022-11-29T08:33:39Z

charts/openyurt/templates/admission-webhooks/webhookService.yaml

+  name: pool-coordinator-webhook
+  namespace: {{ .Release.Namespace }}
+  labels:
+    helm.sh/chart: pool-coordinator-0.1.0


helm releated labels should be removed, as it will be generated when helm install.

huiwq1990 · 2022-11-29T08:42:03Z

charts/openyurt/templates/yurt-controller-manager.yaml

+        env:
+        - name: WEBHOOK_CERT_DIR
+          value: "/tmp/k8s-webhook-server/serving-certs"


maybe change to {{ .Values.admissionWebhooks.certificate.mountPath }}

huiwq1990 · 2022-11-29T08:54:41Z

charts/openyurt/templates/admission-webhooks/webhookService.yaml

+      name: https
+  selector:
+      app.kubernetes.io/name: pool-coordinator
+      app.kubernetes.io/instance: pool-coordinator


yurt-controller-manager pod should add the labels.

huiwq1990 · 2022-11-29T09:09:58Z

pkg/webhook/poolcoordinator_webhook.go

+	MaxRetries          = 30
+)
+
+type PoolCoordinatorWebhook struct {


Could we use controller-runtime implement the webhook, as it could make the code more elegant, just like https://github.com/openyurtio/yurt-app-manager/blob/master/pkg/yurtappmanager/webhook/nodepool/nodepool_validation.go

huiwq1990 · 2022-11-29T09:20:56Z

charts/openyurt/templates/admission-webhooks/validatingwebhookconfiguration.yaml

+  name: vpoolcoordinator.kb.io
+  rules:
+  - apiGroups:
+    - pool-coordinator.openyurt.io


If the webhook validate pod resources, the apigroup may change to "".

…controller

…controller (openyurtio#1040) make yurt-controller-manager take care of webhook configurations and certs add unit tests

openyurt-bot added the kind/feature kind/feature label Oct 29, 2022

openyurt-bot requested review from huangyuqi and rambohe-ch October 29, 2022 14:10

openyurt-bot added the size/XXL label Oct 29, 2022

rambohe-ch reviewed Oct 31, 2022

View reviewed changes

LindaYu17 force-pushed the pool-coordinator-dev branch 6 times, most recently from a085a2c to 20222a7 Compare November 1, 2022 11:51

rambohe-ch reviewed Nov 2, 2022

View reviewed changes

LindaYu17 force-pushed the pool-coordinator-dev branch from 20222a7 to e46af29 Compare November 2, 2022 09:12

luc99hen reviewed Nov 2, 2022

View reviewed changes

LindaYu17 force-pushed the pool-coordinator-dev branch 3 times, most recently from 8f1118d to 9c834b4 Compare November 4, 2022 08:36

rambohe-ch reviewed Nov 11, 2022

View reviewed changes

pkg/controller/poolcoordinator/poolcoordinator_controller.go Outdated

var (

ctl *Controller

ldc *LeaseDelegatedCounter

Copy link

Member

rambohe-ch Nov 11, 2022

Choose a reason for hiding this comment

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

ditto

rambohe-ch reviewed Nov 11, 2022

View reviewed changes

LindaYu17 force-pushed the pool-coordinator-dev branch from 9c834b4 to f743a57 Compare November 14, 2022 17:31

This was linked to issues Nov 15, 2022

[feature request]NodePool Governance Capability: remove nodelifecycle controller and add pool-coordinator controller in yurt-controller-manager component. #776

Closed

[feature request]NodePool Governance Capability: Add admission webhook #775

Closed

LindaYu17 force-pushed the pool-coordinator-dev branch from f743a57 to 9023aba Compare November 20, 2022 16:15

rambohe-ch force-pushed the pool-coordinator-dev branch from f87a711 to 3a15a1d Compare November 21, 2022 03:11

LindaYu17 force-pushed the pool-coordinator-dev branch 3 times, most recently from 7a82781 to 198a747 Compare November 21, 2022 09:51

rambohe-ch force-pushed the pool-coordinator-dev branch from 3a15a1d to 0db2b87 Compare November 23, 2022 06:40

LindaYu17 force-pushed the pool-coordinator-dev branch from 198a747 to 2e7024e Compare November 29, 2022 09:40

luc99hen mentioned this pull request Nov 30, 2022

add poolcoordinator cert manager #1083

Merged

huiwq1990 reviewed Nov 30, 2022

View reviewed changes

LindaYu17 force-pushed the pool-coordinator-dev branch from 2e7024e to 1f25094 Compare December 5, 2022 13:21

add poolcoordinator controller and webhooks to replace nodelifecycle …

1f25094

…controller

rambohe-ch merged commit 7fa0805 into openyurtio:pool-coordinator-dev Dec 7, 2022

gnunu mentioned this pull request Dec 26, 2022

make yurt-controller-manager take care of webhook configurations and … #1108

Merged

gnunu pushed a commit to gnunu/openyurt that referenced this pull request Jan 15, 2023

add poolcoordinator controller and webhooks to replace nodelifecycle …

c25c3ae

…controller (openyurtio#1040) make yurt-controller-manager take care of webhook configurations and certs add unit tests

gnunu mentioned this pull request Jan 15, 2023

add poolcoordinator controller and webhooks to replace nodelifecycle … #1151

Merged

gnunu pushed a commit to gnunu/openyurt that referenced this pull request Jan 15, 2023

add poolcoordinator controller and webhooks to replace nodelifecycle …

f54d6af

…controller (openyurtio#1040) make yurt-controller-manager take care of webhook configurations and certs add unit tests

gnunu pushed a commit to gnunu/openyurt that referenced this pull request Jan 17, 2023

add poolcoordinator controller and webhooks to replace nodelifecycle …

2ce179e

…controller (openyurtio#1040) make yurt-controller-manager take care of webhook configurations and certs add unit tests

gnunu pushed a commit to gnunu/openyurt that referenced this pull request Jan 17, 2023

add poolcoordinator controller and webhooks to replace nodelifecycle …

63b006a

…controller (openyurtio#1040) make yurt-controller-manager take care of webhook configurations and certs add unit tests

gnunu pushed a commit to gnunu/openyurt that referenced this pull request Jan 17, 2023

add poolcoordinator controller and webhooks to replace nodelifecycle …

909296a

…controller (openyurtio#1040) make yurt-controller-manager take care of webhook configurations and certs add unit tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add poolcoordinator controller and webhooks to replace nodelifecycle … #1040

add poolcoordinator controller and webhooks to replace nodelifecycle … #1040

LindaYu17 commented Oct 29, 2022

openyurt-bot commented Oct 29, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Oct 29, 2022

luc99hen commented Oct 30, 2022

rambohe-ch commented Oct 31, 2022

rambohe-ch Oct 31, 2022

rambohe-ch Oct 31, 2022

rambohe-ch Oct 31, 2022

rambohe-ch Oct 31, 2022

rambohe-ch Oct 31, 2022

codecov bot commented Oct 31, 2022 •

edited

Loading

rambohe-ch Nov 2, 2022

rambohe-ch Nov 2, 2022

rambohe-ch Nov 2, 2022

luc99hen Nov 2, 2022

LindaYu17 Nov 4, 2022

rambohe-ch Nov 11, 2022

rambohe-ch Nov 11, 2022

rambohe-ch Nov 11, 2022

rambohe-ch Nov 11, 2022

rambohe-ch Nov 11, 2022

huiwq1990 Nov 29, 2022

huiwq1990 Nov 29, 2022

huiwq1990 Nov 29, 2022

huiwq1990 Nov 29, 2022

huiwq1990 Nov 29, 2022

add poolcoordinator controller and webhooks to replace nodelifecycle … #1040

add poolcoordinator controller and webhooks to replace nodelifecycle … #1040

Conversation

LindaYu17 commented Oct 29, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Oct 29, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Oct 29, 2022

luc99hen commented Oct 30, 2022

rambohe-ch commented Oct 31, 2022

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

codecov bot commented Oct 31, 2022 • 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

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

codecov bot commented Oct 31, 2022 •

edited

Loading