Resource Interpreter implements the interfaces #2794

jameszhangyukun · 2022-11-14T01:21:14Z

Signed-off-by: zhangyukun 38148677+jameszhangyukun@users.noreply.github.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
Resource Interpreter interfaces implements
Which issue(s) this PR fixes:
part of #2371
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE (Part feature)

codecov-commenter · 2022-11-14T02:25:53Z

Codecov Report

Merging #2794 (a35b06b) into master (1005eb9) will increase coverage by 4.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2794      +/-   ##
==========================================
+ Coverage   33.47%   37.49%   +4.02%     
==========================================
  Files         200      189      -11     
  Lines       19642    17573    -2069     
==========================================
+ Hits         6575     6589      +14     
+ Misses      12673    10593    -2080     
+ Partials      394      391       -3

Flag	Coverage Δ
unittests	`37.49% <ø> (+4.02%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/karmadactl/util/idempotency.go	`8.39% <0.00%> (-3.26%)`	⬇️
...armadactl/cmdinit/karmada/webhook_configuration.go	`88.43% <0.00%> (-3.03%)`	⬇️
pkg/search/proxy/controller.go	`85.62% <0.00%> (-1.43%)`	⬇️
pkg/util/label.go	`100.00% <0.00%> (ø)`
pkg/detector/policy.go	`0.00% <0.00%> (ø)`
pkg/detector/detector.go	`0.00% <0.00%> (ø)`
pkg/util/helper/policy.go	`89.42% <0.00%> (ø)`
pkg/karmadactl/cmdinit/utils/format.go	`9.19% <0.00%> (ø)`
pkg/karmadactl/cmdinit/karmada/deploy.go	`0.00% <0.00%> (ø)`
pkg/karmadactl/cmdinit/kubernetes/deploy.go	`0.84% <0.00%> (ø)`
... and 26 more

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

jameszhangyukun · 2022-11-14T02:58:34Z

/retest

karmada-bot · 2022-11-14T02:58:54Z

@jameszhangyukun: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

XiShanYongYe-Chang · 2022-11-14T03:02:09Z

Thanks~ Let me take a look
/assign

RainbowMango

Generally looks good to me.

@jameszhangyukun I guess you have tested it on your side, please post the test here, so that other people can try it locally.

pkg/resourceinterpreter/interpreter.go

RainbowMango · 2022-11-14T03:47:44Z

@jameszhangyukun Please rebase your code with the latest code in master, I see the E2E test is falling and I'm not sure if it is due to the latest E2E changes.

RainbowMango · 2022-11-14T03:52:26Z

This approach is more straightforward than #2733, I also need to check on the progress of #2733.

And I tend to have a quick version for this feature just like I commented on #2733 (comment).

pkg/resourceinterpreter/configurableinterpreter/configurable.go

ikaven1024 · 2022-11-15T06:07:31Z

LGTM

pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go

RainbowMango · 2022-11-15T07:47:58Z

pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go

@@ -45,8 +45,7 @@ func (configManager *interpreterConfigManager) HasSynced() bool {
 	if configManager.initialSynced.Load().(bool) {
 		return true
 	}
-
-	if configManager.HasSynced() {
+	if configuration, err := configManager.lister.List(labels.Everything()); err == nil && len(configuration) == 0 {


len(configuration) == 0 means synced?

Maybe shall use informer.IsInformerSynced()

The relevant comments can be described here.

len(configuration) == 0 means synced?

karmada/pkg/resourceinterpreter/customizedinterpreter/configmanager/manager.go

Lines 50 to 58 in d3b19ed

if configuration, err := m.lister.List(labels.Everything()); err == nil && len(configuration) == 0 {

// the empty list we initially stored is valid to use.

// Setting initialSynced to true, so subsequent checks

// would be able to take the fast path on the atomic boolean in a

// cluster without any webhooks configured.

m.initialSynced.Store(true)

// the informer has synced, and we don't have any items

return true

}

But if informer is not synced, the lister.Lister also returns [] items and nil error. I test it as below:

func TestName2(t *testing.T) { // no server servers on this address config, _ := clientcmd.BuildConfigFromFlags("http://127.0.0.1", "") clientSet := kubernetes.NewForConfigOrDie(config) factory := informers.NewSharedInformerFactory(clientSet, 0) lister := factory.Core().V1().Nodes().Lister() stopCh := make(chan struct{}) defer close(stopCh) factory.Start(stopCh) go factory.WaitForCacheSync(stopCh) ticker := time.NewTicker(time.Second) for range ticker.C { nodes, err := lister.List(labels.Everything()) t.Log(nodes, err) } }

And output:

W1116 11:07:20.838592 95036 reflector.go:324] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused E1116 11:07:20.838731 95036 reflector.go:138] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: Failed to watch *v1.Node: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused cli_test.go:62: [] <nil> W1116 11:07:22.127046 95036 reflector.go:324] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused E1116 11:07:22.127648 95036 reflector.go:138] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: Failed to watch *v1.Node: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused cli_test.go:62: [] <nil> cli_test.go:62: [] <nil> cli_test.go:62: [] <nil> W1116 11:07:25.236868 95036 reflector.go:324] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused E1116 11:07:25.236924 95036 reflector.go:138] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: Failed to watch *v1.Node: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused cli_test.go:62: [] <nil> cli_test.go:62: [] <nil> cli_test.go:62: [] <nil> cli_test.go:62: [] <nil> cli_test.go:62: [] <nil> W1116 11:07:30.568426 95036 reflector.go:324] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused E1116 11:07:30.568490 95036 reflector.go:138] pkg/mod/k8s.io/client-go@v0.23.5/tools/cache/reflector.go:167: Failed to watch *v1.Node: failed to list *v1.Node: Get "http://127.0.0.1/api/v1/nodes?limit=500&resourceVersion=0": dial tcp 127.0.0.1:80: connect: connection refused cli_test.go:62: [] <nil>

We can see, informer is always not synced, but lister returns [] and nil

Just as the comment said, the empty list we initially stored is valid to use. There maybe not a problem?

Another word, when this function will return false?

When an error occurs in the list, the probability is low.

I agree with @ikaven1024, maybe we can hold the informer into interpreterConfigManager , not just the lister.
So, that we can check the sync state by IsInformerSynced or WaitForCacheSync.

// the empty list we initially stored is valid to use. // Setting initialSynced to true, so subsequent checks // would be able to take the fast path on the atomic boolean in a // cluster without any customization configured.

By the way, I think the comments didn't answer @ikaven1024's question.

Let's do it by the next PR.

XiShanYongYe-Chang · 2022-11-15T08:47:01Z

When I apply a ResourceInterpreterCustomization:

apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: examples-for-deploy
spec:
  target:
    apiVersion: apps/v1
    kind: Deployment
  customizations: 
    replicaResource:
      luaScript: | 
        function GetReplicas(desiredObj)
          nodeClaim = {}
          resourceRequest = {}
          result = {}
  
          replica = desiredObj.spec.replicas
          result.resourceRequest = desiredObj.spec.template.spec.containers[1].resources.limits
          nodeClaim.nodeSelector = desiredObj.spec.template.spec.nodeSelector
          nodeClaim.tolerations = desiredObj.spec.template.spec.tolerations
          result.nodeClaim = {}
          result.nodeClaim = nil
          return replica, {}
        end

A panic occurs in the karmada-controller-manager:

E1115 08:44:26.246144       1 runtime.go:77] Observed a panic: sync/atomic: store of inconsistently typed value into Value
goroutine 685 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x1c20800?, 0x2304b30})
	/root/go/src/github.com/karmada-io/karmada/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x203000?})
	/root/go/src/github.com/karmada-io/karmada/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75
panic({0x1c20800, 0x2304b30})
	/usr/local/go/src/runtime/panic.go:838 +0x207
sync/atomic.(*Value).Store(0xc000865640, {0x1ce76c0, 0xc000963c20})
	/usr/local/go/src/sync/atomic/value.go:78 +0xda
github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager.(*interpreterConfigManager).updateConfiguration(0xc0006e0f40)
	/root/go/src/github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go:92 +0x137
github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager.NewInterpreterConfigManager.func1({0xc000b2ae38?, 0x1?})
	/root/go/src/github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go:67 +0x1d
k8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnAdd(...)
	/root/go/src/github.com/karmada-io/karmada/vendor/k8s.io/client-go/tools/cache/controller.go:232
...

pkg/resourceinterpreter/configurableinterpreter/luavm/lua_test.go

pkg/resourceinterpreter/configurableinterpreter/luavm/lua.go

jameszhangyukun · 2022-11-16T01:57:52Z

When I apply a ResourceInterpreterCustomization:

apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: examples-for-deploy
spec:
  target:
    apiVersion: apps/v1
    kind: Deployment
  customizations: 
    replicaResource:
      luaScript: | 
        function GetReplicas(desiredObj)
          nodeClaim = {}
          resourceRequest = {}
          result = {}
  
          replica = desiredObj.spec.replicas
          result.resourceRequest = desiredObj.spec.template.spec.containers[1].resources.limits
          nodeClaim.nodeSelector = desiredObj.spec.template.spec.nodeSelector
          nodeClaim.tolerations = desiredObj.spec.template.spec.tolerations
          result.nodeClaim = {}
          result.nodeClaim = nil
          return replica, {}
        end

A panic occurs in the karmada-controller-manager:

E1115 08:44:26.246144       1 runtime.go:77] Observed a panic: sync/atomic: store of inconsistently typed value into Value
goroutine 685 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x1c20800?, 0x2304b30})
	/root/go/src/github.com/karmada-io/karmada/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x203000?})
	/root/go/src/github.com/karmada-io/karmada/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75
panic({0x1c20800, 0x2304b30})
	/usr/local/go/src/runtime/panic.go:838 +0x207
sync/atomic.(*Value).Store(0xc000865640, {0x1ce76c0, 0xc000963c20})
	/usr/local/go/src/sync/atomic/value.go:78 +0xda
github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager.(*interpreterConfigManager).updateConfiguration(0xc0006e0f40)
	/root/go/src/github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go:92 +0x137
github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager.NewInterpreterConfigManager.func1({0xc000b2ae38?, 0x1?})
	/root/go/src/github.com/karmada-io/karmada/pkg/resourceinterpreter/configurableinterpreter/configmanager/manager.go:67 +0x1d
k8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnAdd(...)
	/root/go/src/github.com/karmada-io/karmada/vendor/k8s.io/client-go/tools/cache/controller.go:232
...

fixed

pkg/resourceinterpreter/configurableinterpreter/luavm/lua.go

Signed-off-by: zhangyukun <38148677+jameszhangyukun@users.noreply.github.com>

XiShanYongYe-Chang · 2022-11-16T08:01:41Z

LGTM

RainbowMango

/lgtm
/approve

karmada-bot · 2022-11-16T09:43:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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~~ [RainbowMango]

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

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 14, 2022

karmada-bot requested review from iawia002 and XiShanYongYe-Chang November 14, 2022 01:21

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 14, 2022

jameszhangyukun force-pushed the pr-interpret-interfaces branch from 32d3d35 to a35b06b Compare November 14, 2022 02:08

karmada-bot assigned XiShanYongYe-Chang Nov 14, 2022

RainbowMango approved these changes Nov 14, 2022

View reviewed changes

pkg/resourceinterpreter/interpreter.go Outdated Show resolved Hide resolved

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2022

RainbowMango removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2022

XiShanYongYe-Chang reviewed Nov 14, 2022

View reviewed changes