Align federated CloneSet's observedGeneration semantics with its native #5057

veophi · 2024-06-17T10:08:44Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Part of #4870, #5021

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Mark `.status.observedGeneration` of CloneSet with `.metadata.generation` only when all members' statuses are algined with its resource template generation.

RainbowMango · 2024-06-17T10:14:30Z

/assign @yike21

codecov-commenter · 2024-06-17T10:22:52Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.22%. Comparing base (25f9fdd) to head (f2ad6e5).
Report is 4 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5057      +/-   ##
==========================================
- Coverage   28.22%   28.22%   -0.01%     
==========================================
  Files         632      632              
  Lines       43553    43553              
==========================================
- Hits        12293    12291       -2     
- Misses      30364    30366       +2     
  Partials      896      896

Flag	Coverage Δ
unittests	`28.22% <ø> (-0.01%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yike21

Thanks for the contribution! Can you help update the testdata files? Just like this:
This will allow the test data to characterize the new customizations.

# testdata/desired-cloneset-nginx.yaml
apiVersion: apps.kruise.io/v1alpha1
kind: CloneSet
metadata:
  labels:
    app: sample
  name: sample
  namespace: test-cloneset
  generation: 1
spec:
  replicas: 4
  selector:
    matchLabels:
      app: sample
      test: cloneset 
  template:
    metadata:
      labels:
        app: sample
        test: cloneset 
    spec:
      volumes:
        - name: configmap
          configMap:
            name: my-sample-config    
      containers:
        - name: nginx
          image: nginx:alpine
          env: 
          - name: logData
            valueFrom: 
              configMapKeyRef:
                name: mysql-config
                key: log
          - name: lowerData
            valueFrom:
              configMapKeyRef:
                name: mysql-config
                key: lower

# testdata/observed-cloneset-nginx.yaml
apiVersion: apps.kruise.io/v1alpha1
kind: CloneSet
metadata:
  annotations:
    resourcetemplate.karmada.io/generation: "1"
  labels:
    app: sample
  name: sample
  namespace: test-cloneset
  generation: 1
spec:
  replicas: 2
  selector:
    matchLabels:
      app: sample
      test: cloneset
  template:
    metadata:
      labels:
        app: sample
        test: cloneset
    spec:
      volumes:
        - name: configmap
          configMap:
            name: my-sample-config    
      containers:
        - name: nginx
          image: nginx:alpine
          env: 
          - name: logData
            valueFrom: 
              configMapKeyRef:
                name: mysql-config
                key: log
          - name: lowerData
            valueFrom:
              configMapKeyRef:
                name: mysql-config
                key: lower
status:
  availableReplicas: 2
  collisionCount: 0
  currentRevision: sample-59df6bd888
  expectedUpdatedReplicas: 2
  labelSelector: app=sample,test=cloneset
  observedGeneration: 1
  readyReplicas: 2
  replicas: 2
  updateRevision: sample-59df6bd888
  updatedReadyReplicas: 2
  updatedReplicas: 2

# status-file.yaml
applied: true
clusterName: member1
health: Healthy
status:
  availableReplicas: 2
  currentRevision: sample-59df6bd888
  expectedUpdatedReplicas: 2
  generation: 1
  labelSelector: app=sample,test=cloneset 
  observedGeneration: 1
  readyReplicas: 2
  replicas: 2
  resourceTemplateGeneration: 1
  updateRevision: sample-59df6bd888
  updatedReadyReplicas: 2
  updatedReplicas: 2
---
applied: true
clusterName: member3
health: Healthy
status:
  availableReplicas: 2
  currentRevision: sample-59df6bd888
  expectedUpdatedReplicas: 2
  generation: 1
  labelSelector: app=sample,test=cloneset 
  observedGeneration: 1
  readyReplicas: 2
  replicas: 2
  resourceTemplateGeneration: 1
  updateRevision: sample-59df6bd888
  updatedReadyReplicas: 2
  updatedReplicas: 2

yike21 · 2024-06-18T01:06:57Z

/lgtm
Maybe we can update these testdata files in another pr.

XiShanYongYe-Chang · 2024-06-18T01:30:14Z

Maybe we can update these testdata files in another pr.

It would be best if we could add this in the same PR. Other PRs can also follow this template to facilitate testing.

XiShanYongYe-Chang · 2024-06-18T01:55:01Z

...fault/thirdparty/resourcecustomizations/apps.kruise.io/v1alpha1/CloneSet/customizations.yaml

+
+          -- handle resource generation report
+          status.generation = observedObj.metadata.generation
+          if observedObj.metadata == nil or observedObj.metadata.annotations == nil then


Shouldn't this judgment observedObj.metadata == nil be placed two lines before the previous?

fixed @XiShanYongYe-Chang

veophi · 2024-06-25T02:18:32Z

Maybe we can update these testdata files in another pr.

It would be best if we could add this in the same PR. Other PRs can also follow this template to facilitate testing.

done. /cc @XiShanYongYe-Chang @yike21

XiShanYongYe-Chang

Thanks~
LGTM
Ask an agin review from @yike21

XiShanYongYe-Chang · 2024-06-25T02:45:10Z

Hi @veophi, would you mind changing your commit user to English? It might be a more common setting.

veophi · 2024-06-25T02:50:34Z

Hi @veophi, would you mind changing your commit user to English? It might be a more common setting.

ok

XiShanYongYe-Chang · 2024-06-25T08:32:55Z

/cc @yike21

yike21 · 2024-06-25T15:25:09Z

...esourcecustomizations/apps.kruise.io/v1alpha1/CloneSet/testdata/observed-cloneset-nginx.yaml

@@ -47,4 +49,4 @@ status:
  replicas: 2
  updateRevision: sample-59df6bd888
  updatedReadyReplicas: 2
-  updatedReplicas: 2
+  updatedReplicas: 2


Just a format error: there should be a blank line at the end of the file.

Here need a blank line at the end of the file.

yike21 · 2024-06-25T15:36:35Z

...resourcecustomizations/apps.kruise.io/v1alpha1/CloneSet/testdata/desired-cloneset-nginx.yaml

+              valueFrom:
+                configMapKeyRef:
+                  name: mysql-config
+                  key: lowerr


Note that the .status.observedGeneration of the federated resource has been value 1 when the scenario with the current test data is formed ( .status.observedGeneration of both member resource is value 1, which means that a previous update existed that made the field for federal resource value 1), so that the .status.observedGeneration of the test result obtained by executing karmadactl interpret -f customizations.yaml --operation aggregateStatus --desired-file testdata/desired-cloneset-nginx.yaml --status-file testdata/status-file.yaml will be value 1 as well. Otherwise we get value 0 by executing test-command.

fixed @yike21

yike21 · 2024-06-25T15:36:43Z

...fault/thirdparty/resourcecustomizations/apps.kruise.io/v1alpha1/CloneSet/customizations.yaml

-          currentRevision = ''
-          labelSelector = ''
+          if desiredObj.status.observedGeneration == nil then
+            desiredObj.status.observedGeneration = 0


Note that .status.observedGeneration of federated resource will be 0 when resourceTemplateGeneration == generation and memberGeneration == memberObservedGeneration is failed.

Note that .status.observedGeneration of federated resource will be 0 when resourceTemplateGeneration == generation and memberGeneration == memberObservedGeneration is failed.

I think it should be 0 if desiredObj.status.observedGeneration == nil and resourceTemplateGeneration == generation and memberGeneration == memberObservedGeneration is failed.

Signed-off-by: veophi <sunweixiang@xiaohongshu.com>

yike21 · 2024-07-08T13:10:17Z

Thanks for your contribution! @veophi
/lgtm

RainbowMango

/approve

karmada-bot · 2024-07-09T01:39:04Z

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

~~pkg/resourceinterpreter/default/thirdparty/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 Jun 17, 2024

karmada-bot requested review from Poor12 and yike21 June 17, 2024 10:08

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 17, 2024

karmada-bot assigned yike21 Jun 17, 2024

RainbowMango added this to the v1.11 milestone Jun 17, 2024

yike21 mentioned this pull request Jun 17, 2024

[Umbrella] Align generation and observedGeneration fields by reflectstatus and aggregatestatus operation #4870

Closed

20 tasks

yike21 reviewed Jun 17, 2024

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2024

XiShanYongYe-Chang reviewed Jun 18, 2024

View reviewed changes

veophi force-pushed the generation/cloneset branch from d0ce0e8 to c2dd8a0 Compare June 25, 2024 02:16

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 25, 2024

XiShanYongYe-Chang reviewed Jun 25, 2024

View reviewed changes

veophi force-pushed the generation/cloneset branch 2 times, most recently from 20adeb4 to 9abb88d Compare June 25, 2024 02:53

karmada-bot requested a review from yike21 June 25, 2024 08:32

yike21 reviewed Jun 25, 2024

View reviewed changes

veophi force-pushed the generation/cloneset branch from 9abb88d to d9648e3 Compare July 8, 2024 03:01

fix cloneset observed generation logic

f2ad6e5

Signed-off-by: veophi <sunweixiang@xiaohongshu.com>

veophi force-pushed the generation/cloneset branch from d9648e3 to f2ad6e5 Compare July 8, 2024 03:47

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024

RainbowMango approved these changes Jul 9, 2024

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2024

karmada-bot merged commit ffdc58c into karmada-io:master Jul 9, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align federated CloneSet's observedGeneration semantics with its native #5057

Align federated CloneSet's observedGeneration semantics with its native #5057

veophi commented Jun 17, 2024

RainbowMango commented Jun 17, 2024

codecov-commenter commented Jun 17, 2024 •

edited

Loading

yike21 left a comment •

edited

Loading

yike21 commented Jun 18, 2024 •

edited

Loading

XiShanYongYe-Chang commented Jun 18, 2024

XiShanYongYe-Chang Jun 18, 2024

veophi Jun 25, 2024

veophi commented Jun 25, 2024

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented Jun 25, 2024

veophi commented Jun 25, 2024

XiShanYongYe-Chang commented Jun 25, 2024

yike21 Jun 25, 2024

XiShanYongYe-Chang Jul 8, 2024

yike21 Jun 25, 2024 •

edited

Loading

veophi Jul 8, 2024

yike21 Jun 25, 2024 •

edited

Loading

veophi Jul 8, 2024

yike21 commented Jul 8, 2024 •

edited

Loading

RainbowMango left a comment

karmada-bot commented Jul 9, 2024

Align federated CloneSet's observedGeneration semantics with its native #5057

Align federated CloneSet's observedGeneration semantics with its native #5057

Conversation

veophi commented Jun 17, 2024

RainbowMango commented Jun 17, 2024

codecov-commenter commented Jun 17, 2024 • edited Loading

Codecov Report

yike21 left a comment • edited Loading

Choose a reason for hiding this comment

yike21 commented Jun 18, 2024 • edited Loading

XiShanYongYe-Chang commented Jun 18, 2024

XiShanYongYe-Chang Jun 18, 2024

Choose a reason for hiding this comment

veophi Jun 25, 2024

Choose a reason for hiding this comment

veophi commented Jun 25, 2024

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Jun 25, 2024

veophi commented Jun 25, 2024

XiShanYongYe-Chang commented Jun 25, 2024

yike21 Jun 25, 2024

Choose a reason for hiding this comment

XiShanYongYe-Chang Jul 8, 2024

Choose a reason for hiding this comment

yike21 Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

veophi Jul 8, 2024

Choose a reason for hiding this comment

yike21 Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

veophi Jul 8, 2024

Choose a reason for hiding this comment

yike21 commented Jul 8, 2024 • edited Loading

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jul 9, 2024

codecov-commenter commented Jun 17, 2024 •

edited

Loading

yike21 left a comment •

edited

Loading

yike21 commented Jun 18, 2024 •

edited

Loading

yike21 Jun 25, 2024 •

edited

Loading

yike21 Jun 25, 2024 •

edited

Loading

yike21 commented Jul 8, 2024 •

edited

Loading