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

feat: add support for describe command #96

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

maanugh
Copy link
Contributor

@maanugh maanugh commented Aug 7, 2024

closes #94

Adding support to visualize kruise rollout using the command kruise describe rollout rollouts-2

image

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign furykerry for approval by writing /assign @furykerry in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@maanugh maanugh marked this pull request as draft August 7, 2024 07:56
@maanugh maanugh changed the title [WIP] feat: add support for describe command feat: add support for describe command Sep 22, 2024
@maanugh maanugh marked this pull request as ready for review September 22, 2024 14:05
Signed-off-by: maanugh <[email protected]>
pkg/cmd/describe/describe_rollout.go Outdated Show resolved Hide resolved
pkg/cmd/describe/describe_rollout.go Outdated Show resolved Hide resolved
fmt.Fprintf(o.Out, tableFormat, "Strategy:", "Canary")

if canary := rollout.Spec.Strategy.Canary; canary != nil {
fmt.Fprintf(o.Out, tableFormat, " Step:", canary.Steps[0].Replicas)
Copy link
Member

Choose a reason for hiding this comment

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

the content in these field is incorrect, and shall we display the current step or all canary steps? I think current step is required.

  1. step: the total number of canary steps can be get from the length of rollout.Spec.Strategy.Canary.steps, and current step can be calculated as rollout.status.canaryStatus.currentStepIndex + 1)
  2. SetWeight is very confusing, i prefer trafficWeight, and it can be get from the rollout.Spec.Strategy.Canary.steps[*].weight if available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

conclusion -> we would showing each step, and highlighting the currentStepIndex .

Strategy: Canary
 Step: 3/3
   Steps:
        
        - Replicas:  1
           traffic: 5%
        - Replicas:  50%
        - Replicas:  100%

pkg/cmd/describe/describe_rollout.go Show resolved Hide resolved
pkg/cmd/describe/describe_rollout.go Outdated Show resolved Hide resolved
pkg/cmd/describe/describe.go Outdated Show resolved Hide resolved
info.Replicas.Current = int32(status.FieldByName("Replicas").Int())
info.Replicas.Updated = int32(status.FieldByName("UpdatedReplicas").Int())
info.Replicas.Ready = int32(status.FieldByName("ReadyReplicas").Int())
info.Replicas.Available = int32(status.FieldByName("AvailableReplicas").Int())
Copy link
Member

Choose a reason for hiding this comment

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

plz check the rollout.status.ObservedGeneration against rollout.Metadata.generation, if two generation does not match, we shall not display status related fields.

@maanugh
Copy link
Contributor Author

maanugh commented Sep 30, 2024

Updated view - >
image

pkg/cmd/describe/describe_rollout.go Outdated Show resolved Hide resolved
pkg/cmd/describe/describe_rollout.go Outdated Show resolved Hide resolved
pkg/cmd/describe/describe_rollout.go Show resolved Hide resolved
pkg/cmd/describe/describe_rollout.go Show resolved Hide resolved
pkg/cmd/describe/describe_rollout.go Outdated Show resolved Hide resolved
BatchID: pod.Labels["rollouts.kruise.io/rollout-batch-id"],
Status: string(pod.Status.Phase),
Age: duration.HumanDuration(time.Since(pod.CreationTimestamp.Time)),
Restarts: fmt.Sprintf("%v (%v ago)", strconv.Itoa(int(pod.Status.ContainerStatuses[0].RestartCount)), duration.HumanDuration(time.Since(pod.Status.ContainerStatuses[0].LastTerminationState.Terminated.FinishedAt.Time))),
Copy link
Member

Choose a reason for hiding this comment

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

plz sum the restart count of all containers in the pod

pkg/cmd/describe/describe_rollout.go Show resolved Hide resolved
}

// Traffic Routings
if len(rollout.Spec.Strategy.Canary.TrafficRoutings) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

for end-to-end canary release, it is also possible for rollout to use rollout.spec.strategy.canary.trafficRoutingRef to specify traffic routing obj, plz refer to
https://openkruise.io/rollouts/user-manuals/strategy-end2end-canary-update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we have the ref of traffic routing then we have to fetch the detail from the api server. //TrafficRoutingRef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is problem in the api .

@furykerry
Copy link
Member

BatchID: pod.Labels["rollouts.kruise.io/rollout-batch-id"],
Status: string(pod.Status.Phase),
Age: duration.HumanDuration(time.Since(pod.CreationTimestamp.Time)),
Restarts: fmt.Sprintf("%v (%v ago)", strconv.Itoa(int(pod.Status.ContainerStatuses[0].RestartCount)), duration.HumanDuration(time.Since(pod.Status.ContainerStatuses[0].LastTerminationState.Terminated.FinishedAt.Time))),
Copy link
Member

Choose a reason for hiding this comment

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

this line will panic :

  1. pod.status.containerstatus may be empty for a newly created pod
  2. pod.status.containerstatus[0].LastTerminationState.Terminated may also be null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lastTerminationState unsure about what to do ?

return err
}

parts := strings.Split(args[0], "/")
Copy link
Member

Choose a reason for hiding this comment

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

the following command will panic due
kubectl kruise describe

panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
github.com/openkruise/kruise-tools/pkg/cmd/describe.(*DescribeRolloutOptions).Complete(0x1400023c360, {0x101f22bc0, 0x1400028e280}, {0x102d3d720, 0x0, 0x0?})
/Users/shouchen/git/kruise-tools/src/github.com/openkruise/kruise-tools/pkg/cmd/describe/describe_rollout.go:137 +0x324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualize Kruise Rollout progress with kruise-tools command
3 participants