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

add execute mod for interpret command #2824

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

ikaven1024
Copy link
Member

Signed-off-by: yingjinhui yingjinhui@didiglobal.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
add skeleton of interpreter command.

Which issue(s) this PR fixes:
part of #2371

Special notes for your reviewer:

Testing

Prepare a get-replica.yml

apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: get-replica
spec:
  target:
    apiVersion: apps/v1
    kind: Deployment
  customizations:
    retention:
    replicaResource:
      luaScript: >
        function GetReplicas(obj)
          replica = obj.spec.replicas + 1
          requirement = {}
          return replica, requirement
        end

and deploy-nginx.yml

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
spec:
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - image: nginx:alpine
        name: nginx
karmadactl interpret -f get-replica.yml --observed-file deploy-nginx.yml --operation=replicaResource

Output:

# replicaResource RESULTS:
---
# REPLICA
4
---
# REQUIRES
nodeclaim: null
resourcerequest: {}

Does this PR introduce a user-facing change?:

`karmadactl`: add execute mod for interpret command.

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 17, 2022
@ikaven1024 ikaven1024 changed the title add execute mod for interpret command [WIP] add execute mod for interpret command Nov 17, 2022
@karmada-bot karmada-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Merging #2824 (4423a0b) into master (9605609) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2824      +/-   ##
==========================================
- Coverage   37.84%   37.79%   -0.06%     
==========================================
  Files         190      191       +1     
  Lines       17696    17715      +19     
==========================================
- Hits         6697     6695       -2     
- Misses      10592    10612      +20     
- Partials      407      408       +1     
Flag Coverage Δ
unittests 37.79% <ø> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
pkg/util/worker.go 66.66% <0.00%> (-4.77%) ⬇️
pkg/scheduler/core/generic_scheduler.go 0.00% <0.00%> (ø)
pkg/scheduler/core/assignment.go 0.00% <0.00%> (ø)
pkg/search/proxy/store/util.go 92.89% <0.00%> (+0.47%) ⬆️

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

@XiShanYongYe-Chang
Copy link
Member

Hi @ikaven1024 as #2750 has been merged, dose this need to rebase the master branch?

@ikaven1024 ikaven1024 force-pushed the interpret-ctl-execute branch 2 times, most recently from ca4de7c to fb27426 Compare November 22, 2022 06:18
@karmada-bot karmada-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 22, 2022
@ikaven1024 ikaven1024 changed the title [WIP] add execute mod for interpret command add execute mod for interpret command Nov 22, 2022
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2022
@ikaven1024
Copy link
Member Author

@RainbowMango @XiShanYongYe-Chang this PR is ready for reviewing.

@ikaven1024 ikaven1024 force-pushed the interpret-ctl-execute branch 4 times, most recently from 06f246c to 4423a0b Compare November 22, 2022 07:55
@RainbowMango
Copy link
Member

/assign @XiShanYongYe-Chang

@XiShanYongYe-Chang
Copy link
Member

How about providing some test demos for each action in the library, otherwise it's too difficult for users to construct tests, such as aggregatedStatus.

@XiShanYongYe-Chang
Copy link
Member

How about providing some test demos for each action in the library, otherwise it's too difficult for users to construct tests, such as aggregatedStatus.

@RainbowMango How do you think?

@RainbowMango
Copy link
Member

How about providing some test demos for each action in the library,

What kind of demo we are taking about? And, to which library?

@ikaven1024 ikaven1024 force-pushed the interpret-ctl-execute branch 2 times, most recently from aefe334 to 87a47b6 Compare November 24, 2022 09:51
@XiShanYongYe-Chang
Copy link
Member

What kind of demo we are taking about?

Demo for karmadactl interpreter, specifically, the YAML file during execution.

And, to which library?

Current library, such as examples.

@ikaven1024 ikaven1024 force-pushed the interpret-ctl-execute branch 2 times, most recently from 25b54d4 to f70eafd Compare November 24, 2022 11:38
@RainbowMango RainbowMango added this to the v1.4 milestone Nov 25, 2022
@ikaven1024 ikaven1024 force-pushed the interpret-ctl-execute branch 3 times, most recently from 287f6a3 to c898140 Compare November 25, 2022 11:59
pkg/karmadactl/interpret/interpret.go Outdated Show resolved Hide resolved
pkg/karmadactl/interpret/interpret.go Outdated Show resolved Hide resolved
pkg/karmadactl/interpret/interpret.go Show resolved Hide resolved
pkg/karmadactl/interpret/interpret.go Outdated Show resolved Hide resolved
func(_ interface{}) { manager.updateConfiguration() })
inform.ForResource(resourceInterpreterCustomizationsGVR, configHandlers)

if inform != nil {
Copy link
Member

Choose a reason for hiding this comment

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

When will the inform be nil?

By the way, inform should be informer, it's a typo brought by the previous patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

When will the inform be nil?
In executing mode, config is not synced from informer, it's loaded by

interpreter := configurableinterpreter.NewConfigurableInterpreter(nil)

So informer is unused in this mode.

By the way, inform should be informer, it's a typo brought by the previous patch.

updated

Copy link
Member

Choose a reason for hiding this comment

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

Have you met a case that the informer is nil?

I'm still confused about why to add the if inform != nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

In interpret command, we don't use informer, so it's nil.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add some comments to explain the situation where informer is nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can add some comments to explain the situation where informer is nil.

Updated.

pkg/karmadactl/interpret/execute.go Outdated Show resolved Hide resolved
@XiShanYongYe-Chang
Copy link
Member

The function test is okay.

yaml preparation:

observed-deploy-nginx.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
  labels:
    app: nginx
spec:
  replicas: 3
  paused: true
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - image: nginx
        name: nginx
status:
  availableReplicas: 2
  observedGeneration: 1
  readyReplicas: 2
  replicas: 2
  updatedReplicas: 2
desired-deploy-nginx.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
  labels:
    app: nginx
spec:
  replicas: 3
  paused: false
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - image: nginx
        name: nginx
      serviceAccountName: test-sa
config.yaml
apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: get-replica
spec:
  target:
    apiVersion: apps/v1
    kind: Deployment
  customizations:
    retention:
    replicaResource:
      luaScript: >
        function GetReplicas(obj)
          replica = obj.spec.replicas
          requirement = {}
          return replica, requirement
        end
    replicaRevision:
      luaScript: >
        function ReviseReplica(obj, desiredReplica)
          obj.spec.replicas = desiredReplica
          return obj
        end
    retention:
      luaScript: >
        function Retain(desiredObj, observedObj)
          desiredObj.spec.paused = observedObj.spec.paused
          return desiredObj
        end
    statusAggregation:
      luaScript: >
        function AggregateStatus(desiredObj, statusItems)
          if statusItems == nil then
            return desiredObj
          end
          if desiredObj.status == nil then
            desiredObj.status = {}
          end
          replicas = 0
          for i = 1, #statusItems do
            if statusItems[i].status ~= nil and statusItems[i].status.replicas ~= nil  then
              replicas = replicas + statusItems[i].status.replicas
            end
          end
          desiredObj.status.replicas = replicas
          return desiredObj
        end
    statusReflection:
      luaScript: >
        function ReflectStatus (observedObj)
          if observedObj.status == nil then
            return nil
          end
        return observedObj.status
        end
    healthInterpretation:
      luaScript: >
        function InterpretHealth(observedObj)
          return observedObj.status.readyReplicas == observedObj.spec.replicas
        end
    dependencyInterpretation:
      luaScript: >
        function GetDependencies(desiredObj)
          dependentSas = {}
          refs = {}
          if desiredObj.spec.template.spec.serviceAccountName ~= '' and desiredObj.spec.template.spec.serviceAccountName ~= 'default' then
            dependentSas[desiredObj.spec.template.spec.serviceAccountName] = true
          end
          local idx = 1
          for key, value in pairs(dependentSas) do
            dependObj = {}
            dependObj.apiVersion = 'v1'
            dependObj.kind = 'ServiceAccount'
            dependObj.name = key
            dependObj.namespace = desiredObj.metadata.namespace
            refs[idx] = dependObj
            idx = idx + 1
          end
          return refs
        end
status-file.yaml
applied: true
clusterName: member1
health: Healthy
status:
  availableReplicas: 1
  readyReplicas: 1
  replicas: 1
  updatedReplicas: 1
---
applied: true
clusterName: member2
health: Healthy
status:
  availableReplicas: 1
  readyReplicas: 1
  replicas: 1
  updatedReplicas: 1

test command:

# karmadactl interpret -f config.yaml --observed-file observed-deploy-nginx.yaml --operation=reviseReplica --desired-replica 5
---
# [1/1] revised:
apiVersion: apps/v1
kind: Deployment
metadata:
    labels:
        app: nginx
    name: nginx
spec:
    paused: true
    replicas: 5
    selector:
        matchLabels:
            app: nginx
    template:
        metadata:
            labels:
                app: nginx
        spec:
            containers:
                - image: nginx
                  name: nginx
status:
    availableReplicas: 2
    observedGeneration: 1
    readyReplicas: 2
    replicas: 2
    updatedReplicas: 2
# karmadactl interpret -f config.yaml --observed-file observed-deploy-nginx.yaml --operation=InterpretReplica
---
# [1/2] replica:
3
---
# [2/2] requires:
{}
# karmadactl interpret -f config.yaml --desired-file desired-deploy-nginx.yaml --observed-file observed-deploy-nginx.yaml --operation retain
---
# [1/1] retained:
apiVersion: apps/v1
kind: Deployment
metadata:
    labels:
        app: nginx
    name: nginx
spec:
    paused: true
    replicas: 3
    selector:
        matchLabels:
            app: nginx
    template:
        metadata:
            labels:
                app: nginx
        spec:
            containers:
                - image: nginx
                  name: nginx
            serviceAccountName: test-sa
# karmadactl interpret -f config.yaml --observed-file observed-deploy-nginx.yaml --operation interpretStatus
---
# [1/1] status:
availableReplicas: 2
observedGeneration: 1
readyReplicas: 2
replicas: 2
updatedReplicas: 2
# karmadactl interpret -f config.yaml --observed-file observed-deploy-nginx.yaml --operation InterpretHealth
---
# [1/1] healthy:
false
# karmadactl interpret -f config.yaml --desired-file desired-deploy-nginx.yaml --operation interpretDependency
---
# [1/1] dependencies:
- apiVersion: v1
  kind: ServiceAccount
  name: test-sa
# karmadactl interpret -f config.yaml --desired-file desired-deploy-nginx.yaml --operation aggregateStatus --status-file status-file.yaml
---
# [1/1] aggregatedStatus:
apiVersion: apps/v1
kind: Deployment
metadata:
    labels:
        app: nginx
    name: nginx
spec:
    paused: false
    replicas: 3
    selector:
        matchLabels:
            app: nginx
    template:
        metadata:
            labels:
                app: nginx
        spec:
            containers:
                - image: nginx
                  name: nginx
            serviceAccountName: test-sa
status:
    replicas: 2

Signed-off-by: yingjinhui <yingjinhui@didiglobal.com>
@ikaven1024 ikaven1024 force-pushed the interpret-ctl-execute branch from 47cb8a6 to 4a37e9c Compare November 28, 2022 06:26
@XiShanYongYe-Chang
Copy link
Member

Thanks!
/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2022
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve
Thanks.

@karmada-bot
Copy link
Collaborator

[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:

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2022
@karmada-bot karmada-bot merged commit eb3763c into karmada-io:master Nov 28, 2022
@ikaven1024 ikaven1024 deleted the interpret-ctl-execute branch November 28, 2022 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants