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

✨ Clusterctl alpha rollout status #8569

Closed
wants to merge 2 commits into from

Conversation

hiromu-a5a
Copy link
Contributor

@hiromu-a5a hiromu-a5a commented Apr 25, 2023

What this PR does / why we need it:

In the current implementation, it's hard to track the status of rollout for a specific resource.
The basic behavior of this status command is similar to the kubectl rollout status which shows a message ever time rollout progress. A few differences are: i) only the machinedeployment/kubeadmcontrolplane are supported; ii) --revision=<revision> option is omitted since when setting this option in kubectl, it just shows an error which is not informative; iii) As a business logic, codes in this PR directly GET resources whereas kubectl uses informer, because using the informer only for this command is too much; iv) timeout is hard coded whereas kubeclt use .spec.progressDeadlineSeconds, because .spec.progressDeadlineSeconds in MachineDeployment doesn't have any effect.

In other words, this PR is the minimum implementation to satisfy most usecases of rollout status command.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Tracking Issue:
#3439

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 25, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 25, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @hiromu-a5a. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the area/clusterctl Issues or PRs related to clusterctl label Apr 25, 2023
@killianmuldoon
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 25, 2023
@k8s-ci-robot
Copy link
Contributor

[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 ykakarap for approval. 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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2023
@Jont828
Copy link
Contributor

Jont828 commented Nov 1, 2023

@hiromu-a5a Hey, are you still working on this? It looks like it needs a rebase and I'm happy to give you a review once that's done.

@hiromu-a5a
Copy link
Contributor Author

@hiromu-a5a Hey, are you still working on this? It looks like it needs a rebase and I'm happy to give you a review once that's done.

Yes. thank you for reminding me. I'll do and let you know.

@vincepri
Copy link
Member

@hiromu-a5a Any updates on this PR?

@k8s-ci-robot
Copy link
Contributor

@hiromu-a5a: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-apidiff-main 47fef46 link false /test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@hiromu-a5a
Copy link
Contributor Author

@vincepri @Jont828 My apologies for the late reply. I've rebased it. Could you review it again?

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023
Copy link
Contributor

@Jont828 Jont828 left a comment

Choose a reason for hiding this comment

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

Overall seems pretty straightforward. I'm wondering if we can find a way to reuse some of the conditions + messages in the status to generate the rollout info.

// MachineDeploymentStatusWatcher implements the StatusViewer interface.
type MachineDeploymentStatusWatcher struct{}

// ObjectStatusWatcher will issue a view on the specified cluster-api resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ObjectStatusWatcher will issue a view on the specified cluster-api resource.
// ObjectStatusWatcher will issue a view on the specified Cluster API resource.

}

oldStatus := ""
timeout := 600 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a shared timeout defined in a const somewhere?

return "", false, errors.Wrapf(err, "failed to get kubeadmcontrolplane/%v", name)
}
if newKcp.Spec.Replicas != nil && newKcp.Status.UpdatedReplicas < *newKcp.Spec.Replicas {
return fmt.Sprintf("Waiting for kubeadmcontrolplane %q rollout to finish: %d out of %d new replicas have been updated...\n", newKcp.Name, newKcp.Status.UpdatedReplicas, *newKcp.Spec.Replicas), false, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Is there a way to reuse something like the ready condition in the status? That should already provide a summary of the resource as well as info on if it's an error, warning, or informational message.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 25, 2024
@Jont828
Copy link
Contributor

Jont828 commented Jun 11, 2024

@hiromu-a5a Are you still working on this? If not we might want to close this PR as it's been without activity for almost 6 months.

@sbueringer
Copy link
Member

At this point it's very very likely that we will deprecate and remove the revision management, see #10479

@hiromu-a5a
Copy link
Contributor Author

I am really sorry for the late reply. We are shifting to GitOps and lost motivation to clusterctl.

@hiromu-a5a hiromu-a5a closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

None yet

7 participants