feat: predefined configurableInterpreter #2768

chaunceyjiang · 2022-11-09T08:31:49Z

Signed-off-by: chaunceyjiang chaunceyjiang@gmail.com

What type of PR is this?
/kind feature

What this PR does / why we need it:

Predefine ConfigurableInterpreter for some well-known third-party projects.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Predefine ConfigurableInterpreter for some well-known third-party projects.

chaunceyjiang · 2022-11-09T14:42:12Z

/cc @ikaven1024 @RainbowMango @XiShanYongYe-Chang

What do you think?

pkg/resourceinterpreter/predefinedconfigurableinterpreter/configurable.go

ikaven1024 · 2022-11-15T06:39:50Z

I do't understand what scene need this feature. Could you show a user case?

chaunceyjiang · 2022-11-15T14:26:43Z

I do't understand what scene need this feature. Could you show a user case?

Currently, the built-in interpreter only maintains K8S resources. For some well-known third-party projects, Karmada may need to do some maintenance to reduce the cost of using the ConfigurableInterpreter.

For example, AggregateStatus may be a commonly used InterpreterOperation

@ikaven1024

ikaven1024 · 2022-11-16T03:23:49Z

I do't understand what scene need this feature. Could you show a user case?

Currently, the built-in interpreter only maintains K8S resources. For some well-known third-party projects, Karmada may need to do some maintenance to reduce the cost of using the ConfigurableInterpreter.

For example, AggregateStatus may be a commonly used InterpreterOperation

This interpreters are supplied by karmada, then why not implement them with go, just like built-in, rather than inefficient lua

chaunceyjiang · 2022-11-16T04:31:59Z

why not implement them with go, just like built-in

Because if you use go to implement it, you need to introduce third-party project APIs, which will lead to more and more dependencies being introduced into Karmada.

ikaven1024 · 2022-11-16T06:01:37Z

why not implement them with go, just like built-in

Because if you use go to implement it, you need to introduce third-party project APIs, which will lead to more and more dependencies being introduced into Karmada.

We can use unstructed, no need import real APIs

chaunceyjiang · 2022-11-16T06:51:51Z

We can use unstructed

This requires the maintainer of the third-party project to be familiar with the code of Karmada. On the other hand, this mechanism is convenient for the maintainer of the third-party project, not the maintainer of Kamrada.

ikaven1024 · 2022-11-16T08:10:50Z

We can use unstructed

This requires the maintainer of the third-party project to be familiar with the code of Karmada. On the other hand, this mechanism is convenient for the maintainer of the third-party project, not the maintainer of Kamrada.

That's say, these script files are committed by third-party project.

chaunceyjiang · 2022-11-16T08:15:30Z

That's say, these script files are committed by third-party project.

Yes, Karmada maintainer or third-party project maintainer.

Poor12 · 2023-03-09T01:46:09Z

Hi @chaunceyjiang, would you like to keep this pr going? We have plans to integrate with some well-known projects. I think this pr is needed.

chaunceyjiang · 2023-03-09T02:26:27Z

Hi @chaunceyjiang, would you like to keep this pr going?

Sure

chaunceyjiang · 2023-03-10T09:10:39Z

Directory Structure

resource_customizations
├── embed.go
├── webapp.my.domain                 # Group
│   └── v1                           # Version
│       └── Guestbook                # Kind
│           ├── customizations.yaml   # Fixed name
│           ├── customizations_test.yaml
│           └── testdata
└── webapp2.my.domain
    └── v1
        └── Guestbook
            ├── customizations.yaml
            ├── customizations_test.yaml
            └── testdata

customizations.yaml

# cat customizations.yaml 
apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: customization
spec:
  target:
    apiVersion: apps/v1
    kind: Deployment
  customizations:
    retention:
      luaScript: >
        function Retain(desiredObj, runtimeObj)
          desiredObj.metadata.annotations.cluster = runtimeObj.metadata.annotations.cluster
          return desiredObj
        end
    replicaResource:
      luaScript: >
        function GetReplicas(obj)
          replica = obj.spec.replicas
          requirement = {
            resourceRequest = obj.spec.template.spec.containers[1].resources.limits,
            nodeClaim = {
              nodeSelector = obj.spec.template.spec.nodeSelector,
              tolerations = obj.spec.template.spec.tolerations
            }
          }
          return replica, requirement
        end
    replicaRevision:
      luaScript: >
        function ReviseReplica(obj, desiredReplica)
          obj.spec.replicas = desiredReplica
          return obj
        end
    statusReflection:
      luaScript: >
        function ReflectStatus(observedObj)
          return observedObj.status
        end
    statusAggregation:
      luaScript: >
        function AggregateStatus(desiredObj, items)
          desiredObj.status.readyReplicas = 0
          for i = 1, #items do
            desiredObj.status.readyReplicas = desiredObj.status.readyReplicas + items[i].status.readyReplicas
          end
          return desiredObj
        end
    healthInterpretation:
      luaScript: >
        function InterpretHealth(observedObj)
          return observedObj.status.readyReplicas == observedObj.spec.replicas
        end
    dependencyInterpretation:
      luaScript: >
        function GetDependencies(desiredObj)
          dependencies = {}
          dependencies[1] = {
            apiVersion = "v1",
            kind       = "ServiceAccount",
            name       = desiredObj.metadata.name,
            namespace  = desiredObj.metadata.namespace,
          }
          return dependencies
        end
---

apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: customization-2
spec:
  target:
    apiVersion: apps/v1
    kind: Deployment
  customizations:
    dependencyInterpretation:
      luaScript: >
        function GetDependencies(desiredObj)
          dependencies = {}
          dependencies[1] = {
            apiVersion = "v1",
            kind       = "ConfigMap",
            name       = desiredObj.metadata.name,
            namespace  = desiredObj.metadata.namespace,
          }
          return dependencies
        end
---

Q: Why is such a directory structure set up, and what are the benefits of doing so?
A: First, it is easier to classify ResourceInterpreterCustomization for easy reference. Second, we can verify the content in customizations.yaml, just like karmada-webhook.

chaunceyjiang · 2023-03-29T08:36:24Z

Does 1000 YAML files mean 1000 resources?

No, there will be some YAML files for testing one type of resource, and each type of resource has about 10 test files. So there are about 100 types of resources.

RainbowMango · 2023-03-29T09:08:24Z

No, there will be some YAML files for testing one type of resource, and each type of resource has about 10 test files. So there are about 100 types of resources.

100 resources VS 1MB, is totally acceptable I guess.

chaunceyjiang · 2023-03-29T09:13:28Z

100 resources VS 1MB, is totally acceptable I guess.

yes, i think so.

chaunceyjiang · 2023-03-29T10:24:17Z

Can we organize the code like this?

Can the luavm directory be separated from configureinterpreter?

RainbowMango · 2023-03-31T01:32:49Z

Can the luavm directory be separated from configureinterpreter?

I think keeping the luavm in pkg/resourceinterpreter/customized/declarative would be fine, at least it won't block this PR. Let's revisit during or after this PR.

Poor12 · 2023-04-04T06:45:54Z

Good job! Could you please post the latest test result(whether the built-in configurableInterpret takes effect and whether users can override the default third-party crds)?

chaunceyjiang · 2023-04-04T09:17:12Z

build-in third-party configurableInterpreter:

users can override the third-party configurableInterpreter:

    statusAggregation:
      luaScript: >
        function AggregateStatus(desiredObj, statusItems)
          if statusItems == nil then
            return desiredObj
          end
          if desiredObj.status == nil then
            desiredObj.status = {}
          end
          if desiredObj.generation == nil then
            desiredObj.generation = 0
          end
          generation = desiredObj.generation
          replicas = 0
          updatedReplicas = 0 
          readyReplicas = 0
          availableReplicas = 0
          updatedReadyReplicas = 0
          expectedUpdatedReplicas = 0
          updateRevision = ''
          currentRevision = ''
          for i = 1, #statusItems do
            if statusItems[i].status ~= nil and statusItems[i].status.replicas ~= nil then
              replicas = replicas + statusItems[i].status.replicas
            end

            if statusItems[i].status ~= nil and statusItems[i].status.availableReplicas ~= nil then
              availableReplicas = availableReplicas + statusItems[i].status.availableReplicas
            end

          end
          desiredObj.status.observedGeneration = generation
          desiredObj.status.replicas = replicas
          desiredObj.status.updatedReplicas = updatedReplicas
          desiredObj.status.readyReplicas = readyReplicas
          desiredObj.status.availableReplicas = availableReplicas
          desiredObj.status.updatedReadyReplicas = updatedReadyReplicas
          desiredObj.status.expectedUpdatedReplicas = expectedUpdatedReplicas
          desiredObj.status.updateRevision = updateRevision
          desiredObj.status.currentRevision = currentRevision
          return desiredObj
        end

krmada control plane:

➤ k get clone -oyaml

Poor12 · 2023-04-06T01:40:22Z

/lgtm

XiShanYongYe-Chang

Thanks~

XiShanYongYe-Chang · 2023-04-06T02:39:25Z

pkg/resourceinterpreter/default/thirdparty/manager.go

+		if d.IsDir() {
+			return nil
+		}
+		if strings.Contains(d.Name(), "testdata") {


When is this going to happen?
Should we add a new document to describe how to use this feature?

This is just to filter out test files.

pkg/resourceinterpreter/default/thirdparty/manager.go

pkg/resourceinterpreter/interpreter.go

RainbowMango

I'm sorry, I missed submitting my comments/questions...

resourcecustomizations/embed.go

…party projects. Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>

RainbowMango

Just one question from me.
/assign @XiShanYongYe-Chang

RainbowMango · 2023-04-10T10:23:28Z

pkg/resourceinterpreter/interpreter.go

+	replica, requires, hookEnabled, err = i.thirdpartyInterpreter.GetReplicas(object)
+	if err != nil {
+		return
+	}
+	if hookEnabled {
+		return
+	}


Should we put the thirdpartyInterpreter after the defaultInterpreter for better efficiency?
I think K8s native resources are used more often than third-party ones.

Same as below.

The resources interpreted by thirdpartyInterpreter and thirdpartyInterpreter maybe not overlap, and I agree with this point.

The resources interpreted by thirdpartyInterpreter and thirdpartyInterpreter maybe not overlap

Yes, that's exactly what I was thinking.
@chaunceyjiang What do you think?

In the ReflectStatus of defaultInterpreter, if there is no built-in handler, it will attempt to collect the whole status from '.status' filed.

Refter to: https://github.com/karmada-io/karmada/blob/master/pkg/resourceinterpreter/default/native/default.go#L134-L135

If thirdpartyInterpreter is placed after defaultInterpreter, then the ReflectStatus of thirdpartyInterpreter will not take effect.

@RainbowMango @XiShanYongYe-Chang What do you think?

If thirdpartyInterpreter is placed after defaultInterpreter, then the ReflectStatus of thirdpartyInterpreter will not take effect.

Good point!

I missed that, thank you.

/approve

XiShanYongYe-Chang

Generally LGTM.

RainbowMango · 2023-04-11T02:49:01Z

/lgtm
Leave approve to @XiShanYongYe-Chang

karmada-bot · 2023-04-11T02:50:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

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:

~~pkg/resourceinterpreter/OWNERS~~ [XiShanYongYe-Chang]

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 9, 2022

karmada-bot requested review from Garrybest and iawia002 November 9, 2022 08:31

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

chaunceyjiang changed the title ~~Predefined ConfigurableInterpreter~~ [WIP] Predefined ConfigurableInterpreter Nov 9, 2022

karmada-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch from fa2cb96 to b639f25 Compare November 9, 2022 10:35

karmada-bot requested review from ikaven1024, RainbowMango and XiShanYongYe-Chang November 9, 2022 14:42

ikaven1024 reviewed Nov 15, 2022

View reviewed changes

chaunceyjiang mentioned this pull request Dec 8, 2022

refactor resourceinterpreter framework #2766

Closed

RainbowMango added this to the v1.5 milestone Dec 8, 2022

RainbowMango modified the milestones: v1.5, v1.6 Mar 7, 2023

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch from b639f25 to 7b86e50 Compare March 10, 2023 09:04

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch 2 times, most recently from 9665531 to a2e4d6f Compare March 10, 2023 09:19

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch from 73e4d20 to 5de9942 Compare April 4, 2023 03:14

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 4, 2023

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch from 5de9942 to abf20ee Compare April 4, 2023 06:29

karmada-bot assigned Poor12 Apr 6, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023

XiShanYongYe-Chang reviewed Apr 6, 2023

View reviewed changes

RainbowMango mentioned this pull request Apr 10, 2023

add customizations for apps.krusie.io/v1alpha1/BroadcastJob #3366

Merged

RainbowMango reviewed Apr 10, 2023

View reviewed changes

resourcecustomizations/embed.go Outdated Show resolved Hide resolved

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch from abf20ee to c4d3726 Compare April 10, 2023 07:05

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2023

Add a third-party configurable interpreter for some well-known third-…

538d8f1

…party projects. Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>

chaunceyjiang force-pushed the predefinedConfigurableInterpreter branch from c4d3726 to 538d8f1 Compare April 10, 2023 07:22

RainbowMango reviewed Apr 10, 2023

View reviewed changes

karmada-bot assigned XiShanYongYe-Chang Apr 10, 2023

XiShanYongYe-Chang reviewed Apr 10, 2023

View reviewed changes

karmada-bot assigned RainbowMango Apr 11, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2023

karmada-bot merged commit 4133d59 into karmada-io:master Apr 11, 2023
12 checks passed

feat: predefined configurableInterpreter #2768

feat: predefined configurableInterpreter #2768

Conversation

chaunceyjiang commented Nov 9, 2022

chaunceyjiang commented Nov 9, 2022

ikaven1024 commented Nov 15, 2022

chaunceyjiang commented Nov 15, 2022

ikaven1024 commented Nov 16, 2022

chaunceyjiang commented Nov 16, 2022

ikaven1024 commented Nov 16, 2022

chaunceyjiang commented Nov 16, 2022

ikaven1024 commented Nov 16, 2022

chaunceyjiang commented Nov 16, 2022

Poor12 commented Mar 9, 2023

chaunceyjiang commented Mar 9, 2023

chaunceyjiang commented Mar 10, 2023 • edited

chaunceyjiang commented Mar 29, 2023

RainbowMango commented Mar 29, 2023

chaunceyjiang commented Mar 29, 2023

chaunceyjiang commented Mar 29, 2023

RainbowMango commented Mar 31, 2023

Poor12 commented Apr 4, 2023

chaunceyjiang commented Apr 4, 2023 • edited

Poor12 commented Apr 6, 2023

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango left a 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

chaunceyjiang Apr 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango commented Apr 11, 2023

karmada-bot commented Apr 11, 2023

chaunceyjiang commented Mar 10, 2023 •

edited

chaunceyjiang commented Apr 4, 2023 •

edited

chaunceyjiang Apr 11, 2023 •

edited