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.

Sign up for GitHub

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

Jump to bottom

Add FullyApplied to binding #825

Merged
merged 1 commit into from
Oct 21, 2021
Merged

Add FullyApplied to binding #825

merged 1 commit into from
Oct 21, 2021

Conversation

lonelyCZ
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:
FullyApplied represents if the resource is applied to all of the scheduled clusters.

Which issue(s) this PR fixes:
Fixes #820

Special notes for your reviewer:
The process also can be moved to workstatus.go, but that is not a accurate place for resourcebinding. so I put them to ResourceBindingController.

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 16, 2021
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 16, 2021
@lonelyCZ lonelyCZ force-pushed the add_binding_fullyapplied branch 9 times, most recently from 5d88838 to 7dced83 Compare October 16, 2021 09:49
@lonelyCZ
Copy link
Member Author

ResourceBinding status is shown below.

...
status:
  aggregatedStatus:
  - applied: true
    clusterName: member68
    status:
      availableReplicas: 1
      conditions:
      - lastTransitionTime: "2021-10-16T15:14:13Z"
        lastUpdateTime: "2021-10-16T15:14:13Z"
        message: Deployment has minimum availability.
        reason: MinimumReplicasAvailable
        status: "True"
        type: Available
      - lastTransitionTime: "2021-10-16T15:14:04Z"
        lastUpdateTime: "2021-10-16T15:14:13Z"
        message: ReplicaSet "nginx-6799fc88d8" has successfully progressed.
        reason: NewReplicaSetAvailable
        status: "True"
        type: Progressing
      observedGeneration: 1
      readyReplicas: 1
      replicas: 1
      updatedReplicas: 1
  conditions:
  - lastTransitionTime: "2021-10-16T15:14:06Z"
    message: All works have been successfully applied.
    reason: FullyAppliedSuccess
    status: "True"
    type: FullyApplied

RainbowMango
RainbowMango reviewed Oct 19, 2021
View reviewed changes
pkg/controllers/binding/cluster_resource_binding_controller.go Outdated
return controllerruntime.Result{Requeue: true}, err
}
} else {
err := c.updateFullyAppliedCondition(binding, metav1.ConditionFalse, "FullyAppliedFalse", "Failed to apply all works.")
Copy link
Member

Choose a reason for hiding this comment

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

It's not a Failed state, it just means hasn't FullyApplied.
Failed to apply all works. will imply something bad happens, but it doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose to not setting the condition when FullyApplied==false.

Copy link
Member Author

@lonelyCZ lonelyCZ Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your reviews. @RainbowMango

Failed to apply all works.

Whether it can be changed to
Not all works have been deployed
If necessary, could count the successful applied works:
("Not all works(%v/%v) have been deployed", acutalAppliedWorksNum, total)

I suppose to not setting the condition when FullyApplied==false.

Ok, that is a good idea, reducing unnecessary update

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose to not setting the condition when FullyApplied==false.

I have a question, whether or not this is the case that the status is changed from FullyApplied==true to FullyApplied==false?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. This would probably happen when there are new clusters added.
No idea about this yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. This would probably happen when there are new clusters added. No idea about this yet.

Thanks for your reply! I just changed the code, that didnt set condition when FullyApplied==false. And if there have been added FullyApllied` condition, it need not to inspect works status, reducing the inspect times.

pkg/controllers/binding/cluster_resource_binding_controller.go Outdated
Status: status,
Reason: reason,
Message: message,
LastTransitionTime: metav1.Now(),
Copy link
Member

Choose a reason for hiding this comment

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

We should trigger the update only when the status changed (not FullyApplied --> FullyApplied).
Here will trigger the update operation every time since we have updated the LastTransitionTime here.

@lonelyCZ lonelyCZ force-pushed the add_binding_fullyapplied branch 2 times, most recently from ae4d59f to 2f445e2 Compare October 19, 2021 13:50
@lonelyCZ
Copy link
Member Author

Hi, @RainbowMango
I made some changes, please review them, thanks!

@lonelyCZ lonelyCZ force-pushed the add_binding_fullyapplied branch from 2f445e2 to f3eae22 Compare October 20, 2021 08:23
RainbowMango
RainbowMango reviewed Oct 21, 2021
View reviewed changes
pkg/controllers/binding/binding_controller.go
Type: workv1alpha2.FullyApplied,
Status: metav1.ConditionTrue,
Reason: FullyAppliedSuccessReason,
Message: FullyAppliedSuccessMessage,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder who set lastTransitionTime for us?

  - lastTransitionTime: "2021-10-21T02:16:18Z"
    message: All works have been successfully applied
    reason: FullyAppliedSuccess
    status: "True"
    type: FullyApplied

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/lonelyCZ/karmada/blob/master/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1412-L1418

It seem automatically set lastTransitionTime.

Now the new status is shown below:

...
conditions:
  - lastTransitionTime: "2021-10-21T03:58:22Z"
    message: the binding has been scheduled
    reason: BindingScheduled
    status: "True"
    type: Scheduled
  - lastTransitionTime: "2021-10-21T03:58:23Z"
    message: All works have been successfully applied
    reason: FullyAppliedSuccess
    status: "True"
    type: FullyApplied

Copy link
Member

Choose a reason for hiding this comment

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

karmada/vendor/k8s.io/apimachinery/pkg/api/meta/conditions.go

Lines 36 to 38 in 9c891f5

if newCondition.LastTransitionTime.IsZero() {
newCondition.LastTransitionTime = metav1.NewTime(time.Now())
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for pointing that out!

@RainbowMango
Copy link
Member

/lgtm
/approve

Thanks.

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2021
@RainbowMango RainbowMango added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2021
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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 merged commit 9eaba3e into karmada-io:master Oct 21, 2021
@lonelyCZ lonelyCZ deleted the add_binding_fullyapplied branch November 30, 2021 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RainbowMango RainbowMango RainbowMango left review comments

@Garrybest Garrybest Awaiting requested review from Garrybest

@mrlihanbo mrlihanbo Awaiting requested review from mrlihanbo

Assignees

@RainbowMango RainbowMango

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Propose 'FullyApplied' condition for RB/CRB
3 participants
@lonelyCZ @RainbowMango @karmada-bot