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

Add Message to Suspend #4232

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add Message to Suspend #4232

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
479fc3c
Adds an optional `reason` cli flag to the `suspend` command that
haggishunk Sep 12, 2023
46552c5
Update cmd/flux/suspend.go
haggishunk Sep 13, 2023
59c759e
Update cmd/flux/suspend.go
haggishunk Sep 13, 2023
0ba9db5
Sets the flux cli `suspend` command `--reason` flag to an empty string
haggishunk Sep 13, 2023
8483b45
Removed comments
haggishunk Sep 13, 2023
b8a4a2f
Changed reason to more broadly applicable message.
haggishunk Nov 29, 2023
80f7350
Merge branch 'main' into add-reason-for-suspend
haggishunk May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/flux/resume_alert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ func (obj alertAdapter) getObservedGeneration() int64 {

func (obj alertAdapter) setUnsuspended() {
obj.Alert.Spec.Suspend = false
if _, ok := obj.Alert.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.Alert.Annotations, SuspendReasonAnnotation)
}
}

func (obj alertAdapter) successMessage() string {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_helmrelease.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ func (obj helmReleaseAdapter) getObservedGeneration() int64 {

func (obj helmReleaseAdapter) setUnsuspended() {
obj.HelmRelease.Spec.Suspend = false
if _, ok := obj.HelmRelease.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.HelmRelease.Annotations, SuspendReasonAnnotation)
}
}

func (obj helmReleaseAdapter) successMessage() string {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_image_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func (obj imageRepositoryAdapter) getObservedGeneration() int64 {

func (obj imageRepositoryAdapter) setUnsuspended() {
obj.ImageRepository.Spec.Suspend = false
if _, ok := obj.ImageRepository.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.ImageRepository.Annotations, SuspendReasonAnnotation)
}
}

func (a imageRepositoryListAdapter) resumeItem(i int) resumable {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_image_updateauto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ func init() {

func (obj imageUpdateAutomationAdapter) setUnsuspended() {
obj.ImageUpdateAutomation.Spec.Suspend = false
if _, ok := obj.ImageUpdateAutomation.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.ImageUpdateAutomation.Annotations, SuspendReasonAnnotation)
}
}

func (obj imageUpdateAutomationAdapter) getObservedGeneration() int64 {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_kustomization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ func (obj kustomizationAdapter) getObservedGeneration() int64 {

func (obj kustomizationAdapter) setUnsuspended() {
obj.Kustomization.Spec.Suspend = false
if _, ok := obj.Kustomization.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.Kustomization.Annotations, SuspendReasonAnnotation)
}
}

func (obj kustomizationAdapter) successMessage() string {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_receiver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ func (obj receiverAdapter) getObservedGeneration() int64 {

func (obj receiverAdapter) setUnsuspended() {
obj.Receiver.Spec.Suspend = false
if _, ok := obj.Receiver.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.Receiver.Annotations, SuspendReasonAnnotation)
}
}

func (obj receiverAdapter) successMessage() string {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_source_bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func (obj bucketAdapter) getObservedGeneration() int64 {

func (obj bucketAdapter) setUnsuspended() {
obj.Bucket.Spec.Suspend = false
if _, ok := obj.Bucket.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.Bucket.Annotations, SuspendReasonAnnotation)
}
}

func (a bucketListAdapter) resumeItem(i int) resumable {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_source_chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ func (obj helmChartAdapter) getObservedGeneration() int64 {

func (obj helmChartAdapter) setUnsuspended() {
obj.HelmChart.Spec.Suspend = false
if _, ok := obj.HelmChart.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.HelmChart.Annotations, SuspendReasonAnnotation)
}
}

func (obj helmChartAdapter) successMessage() string {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_source_git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func (obj gitRepositoryAdapter) getObservedGeneration() int64 {

func (obj gitRepositoryAdapter) setUnsuspended() {
obj.GitRepository.Spec.Suspend = false
if _, ok := obj.GitRepository.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.GitRepository.Annotations, SuspendReasonAnnotation)
}
}

func (a gitRepositoryListAdapter) resumeItem(i int) resumable {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_source_helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func (obj helmRepositoryAdapter) getObservedGeneration() int64 {

func (obj helmRepositoryAdapter) setUnsuspended() {
obj.HelmRepository.Spec.Suspend = false
if _, ok := obj.HelmRepository.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.HelmRepository.Annotations, SuspendReasonAnnotation)
}
}

func (a helmRepositoryListAdapter) resumeItem(i int) resumable {
Expand Down
3 changes: 3 additions & 0 deletions cmd/flux/resume_source_oci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func (obj ociRepositoryAdapter) getObservedGeneration() int64 {

func (obj ociRepositoryAdapter) setUnsuspended() {
obj.OCIRepository.Spec.Suspend = false
if _, ok := obj.OCIRepository.Annotations[SuspendReasonAnnotation]; ok {
delete(obj.OCIRepository.Annotations, SuspendReasonAnnotation)
}
}

func (a ociRepositoryListAdapter) resumeItem(i int) resumable {
Expand Down
14 changes: 11 additions & 3 deletions cmd/flux/suspend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,25 @@ var suspendCmd = &cobra.Command{
}

type SuspendFlags struct {
all bool
all bool
reason string
}

var suspendArgs SuspendFlags

func init() {
suspendCmd.PersistentFlags().BoolVarP(&suspendArgs.all, "all", "", false,
"suspend all resources in that namespace")
suspendCmd.PersistentFlags().StringVarP(&suspendArgs.reason, "reason", "r", "suspended",
Copy link
Contributor

@darkowlzz darkowlzz Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to have some clarity about the intention here. This flag has a default value, which is always used to set the annotation on a suspended object. This results in the suspend command to always add an annotation on the object even when no reason is passed.

Name:         test-1
Namespace:    default
Labels:       <none>
Annotations:  suspend.fluxcd.io/reason: suspended
API Version:  source.toolkit.fluxcd.io/v1
Kind:         GitRepository

This makes every suspended object to have this annotation always. It touches a discussion we had a few times before about using annotation to suspend the object instead of a field in the spec. I think in the discussions, we usually conclude that it's too late now to remove suspend from the spec as some APIs are already GA.
Implementing this change such that there's a reason annotation on suspended object always, makes the API a little repetitive/redundant/open for multiple interpretation for everyone forever. Some users may assume that the annotation is how they can detect if the object is suspended. But if an object is resumed without using Flux CLI, the annotation will remain on the object.
We can avoid all these if this is implemented such that only when a reason is provided, the annotation is added, else it behaves as before. Users who know about this feature, will opt-in to have the annotation.
Is this an intentional design choice to always have the annotation with some benefits?

Copy link
Member

@stefanprodan stefanprodan Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We would add this annotation only if a reason is provided. So we really need to remove the default from the flag.

Copy link
Author

Choose a reason for hiding this comment

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

While I see it beneficial to support the suspend event and optional reason metadata percolate out through flux events, that's outside the scope of this PR.

The default of --reason suspended tightly coupling flux cli initiated suspensions to a resource annotation was an oversight of mine. I'm glad to have such a proper review @darkowlzz @stefanprodan :) and will change this.

Copy link
Member

Choose a reason for hiding this comment

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

During the community meeting today, I voiced concerns about this detachment and the possibility that the reason annotation would continue to exist when the .spec would move on. For example, when e.g. the .spec.suspend boolean is quickly changed in Git without making use of the Flux CLI.

Given this, I am wondering if we could not tackle two issues at once by using suspend.fluxcd.io/reason as another way to mark the resource as suspended. This would force a user to remove the annotation to continue the reconciliation of the object (avoiding any potential detachment), while also allowing the object to be suspended without creating a bump in the generation of the object (solving a historical regret around including the suspend field in the spec of the object).

This would raise some questions around how we would e.g. display this in a kubectl get column, but does prepare us for a better future in which we could eventually deprecate the .spec.suspend in a new major API version.

Copy link
Member

@hiddeco hiddeco Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking more about this, it may not even explicitly have to be /reason. The mechanics could also be that the presence of the key in the annotation map simply marks the resource as suspended, while the value can be arbitrary (including a reason string). To allow an explicit false, we could e.g. recognize false | 0 | False as "disabled" while any other value would mean "enabled".

Copy link
Author

Choose a reason for hiding this comment

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

During the community meeting today, I voiced concerns about this detachment and the possibility that the reason annotation would continue to exist when the .spec would move on. For example, when e.g. the .spec.suspend boolean is quickly changed in Git without making use of the Flux CLI.

Given this, I am wondering if we could not tackle two issues at once by using suspend.fluxcd.io/reason as another way to mark the resource as suspended.

After discussing a bit with @darkowlzz at yesterday's flux bug scrub, I'd like to start an RFC for implementing a suspend-by-annotation and deprecating .spec.suspend api. Perhaps we use suspend.toolkit.fluxcd.io/state or suspend.toolkit.fluxcd.io/suspended to signal to the controllers that the resource is suspended while allowing an optional reason under suspend.toolkit.fluxcd.io/reason.

If that path is supported by the maintainers I would ask that we consider this PR again without any control semantics for the suspend reason annotation.

Copy link
Member

Choose a reason for hiding this comment

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

Think reconcile.fluxcd.io/suspended would be sufficient to suspend and provide a reason to start with, and fit nicely together with the existing annotation to request a reconciliation (reconcile.fluxcd.io/requestedAt).

Copy link
Member

@makkes makkes Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think adding a suspend annotation is great and RFC is the most reasonable way to kick off a design around it. One thing to keep in mind is that the RFC should be very clear about co-existence with the existing .spec.suspend field which we cannot remove in the v1 APIs (e.g. kustomization.v1.kustomize.toolkit.fluxcd.io), e.g. regarding precedence.

Copy link
Author

Choose a reason for hiding this comment

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

I like the suggestion to use reconcile.fluxcd.io/suspend annotation for suspending a resource with support for a string that could be anything, including a reason-- maybe "message" is more open. However if we do want to have this eventually respected by controllers I'm concerned that adding it here will require some later migration. I'm not in a rush to get this feature PR incorporated and would be happy to get an RFC going to help the community get what they want with minimal churn.

Copy link
Author

Choose a reason for hiding this comment

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

After discussing the #4271 at the team meeting in September, 2023 @stefanprodan noted some complications with having suspended state set only as an annotation with a new version cli which would not be recognized by an old version controller. Possible long term options to support #4271 were suggested by @darkowlzz which included getting object status to indicate suspended state which would probably warrant a separate RFC.

I'd like to request another review of this PR which omits any controller interaction and simply focuses on metadata for human communication.

"set a reason for why the resource is suspended")
haggishunk marked this conversation as resolved.
Show resolved Hide resolved
rootCmd.AddCommand(suspendCmd)
}

type suspendable interface {
adapter
copyable
isSuspended() bool
setSuspended()
setSuspended(reason string)
}

type suspendCommand struct {
Expand All @@ -76,6 +79,7 @@ func (suspend suspendCommand) run(cmd *cobra.Command, args []string) error {
return err
}

// in case of all, get all in namespace and patch 'em
if len(args) < 1 && suspendArgs.all {
listOpts := []client.ListOption{
client.InNamespace(*kubeconfigArgs.Namespace),
Expand All @@ -88,6 +92,7 @@ func (suspend suspendCommand) run(cmd *cobra.Command, args []string) error {
return nil
}

// when not all, patch list of args
processed := make(map[string]struct{}, len(args))
for _, arg := range args {
if _, has := processed[arg]; has {
Expand Down Expand Up @@ -130,7 +135,7 @@ func (suspend suspendCommand) patch(ctx context.Context, kubeClient client.WithW

obj := suspend.list.item(i)
patch := client.MergeFrom(obj.deepCopyClientObject())
obj.setSuspended()
obj.setSuspended(suspendArgs.reason)
if err := kubeClient.Patch(ctx, obj.asClientObject(), patch); err != nil {
return err
}
Expand All @@ -140,3 +145,6 @@ func (suspend suspendCommand) patch(ctx context.Context, kubeClient client.WithW

return nil
}

// SuspendReasonAnnotation is the metadata key used to store the reason for resource suspension
const SuspendReasonAnnotation string = "suspend.fluxcd.io/reason"
haggishunk marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion cmd/flux/suspend_alert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj alertAdapter) isSuspended() bool {
return obj.Alert.Spec.Suspend
}

func (obj alertAdapter) setSuspended() {
func (obj alertAdapter) setSuspended(reason string) {
obj.Alert.Spec.Suspend = true
obj.Alert.Annotations[SuspendReasonAnnotation] = reason
}

func (a alertListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_helmrelease.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@ func (obj helmReleaseAdapter) isSuspended() bool {
return obj.HelmRelease.Spec.Suspend
}

func (obj helmReleaseAdapter) setSuspended() {
func (obj helmReleaseAdapter) setSuspended(reason string) {
obj.HelmRelease.Spec.Suspend = true
obj.HelmRelease.Annotations[SuspendReasonAnnotation] = reason
}

func (a helmReleaseListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_image_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj imageRepositoryAdapter) isSuspended() bool {
return obj.ImageRepository.Spec.Suspend
}

func (obj imageRepositoryAdapter) setSuspended() {
func (obj imageRepositoryAdapter) setSuspended(reason string) {
obj.ImageRepository.Spec.Suspend = true
obj.ImageRepository.Annotations[SuspendReasonAnnotation] = reason
}

func (a imageRepositoryListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_image_updateauto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (update imageUpdateAutomationAdapter) isSuspended() bool {
return update.ImageUpdateAutomation.Spec.Suspend
}

func (update imageUpdateAutomationAdapter) setSuspended() {
func (update imageUpdateAutomationAdapter) setSuspended(reason string) {
update.ImageUpdateAutomation.Spec.Suspend = true
update.ImageUpdateAutomation.Annotations[SuspendReasonAnnotation] = reason
}

func (a imageUpdateAutomationListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_kustomization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@ func (obj kustomizationAdapter) isSuspended() bool {
return obj.Kustomization.Spec.Suspend
}

func (obj kustomizationAdapter) setSuspended() {
func (obj kustomizationAdapter) setSuspended(reason string) {
obj.Kustomization.Spec.Suspend = true
obj.Kustomization.Annotations[SuspendReasonAnnotation] = reason
}

func (a kustomizationListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_receiver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj receiverAdapter) isSuspended() bool {
return obj.Receiver.Spec.Suspend
}

func (obj receiverAdapter) setSuspended() {
func (obj receiverAdapter) setSuspended(reason string) {
obj.Receiver.Spec.Suspend = true
obj.Receiver.Annotations[SuspendReasonAnnotation] = reason
}

func (a receiverListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_source_bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj bucketAdapter) isSuspended() bool {
return obj.Bucket.Spec.Suspend
}

func (obj bucketAdapter) setSuspended() {
func (obj bucketAdapter) setSuspended(reason string) {
obj.Bucket.Spec.Suspend = true
obj.Bucket.Annotations[SuspendReasonAnnotation] = reason
}

func (a bucketListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_source_chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj helmChartAdapter) isSuspended() bool {
return obj.HelmChart.Spec.Suspend
}

func (obj helmChartAdapter) setSuspended() {
func (obj helmChartAdapter) setSuspended(reason string) {
obj.HelmChart.Spec.Suspend = true
obj.HelmChart.Annotations[SuspendReasonAnnotation] = reason
}

func (a helmChartListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_source_git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj gitRepositoryAdapter) isSuspended() bool {
return obj.GitRepository.Spec.Suspend
}

func (obj gitRepositoryAdapter) setSuspended() {
func (obj gitRepositoryAdapter) setSuspended(reason string) {
obj.GitRepository.Spec.Suspend = true
obj.GitRepository.Annotations[SuspendReasonAnnotation] = reason
}

func (a gitRepositoryListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_source_helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj helmRepositoryAdapter) isSuspended() bool {
return obj.HelmRepository.Spec.Suspend
}

func (obj helmRepositoryAdapter) setSuspended() {
func (obj helmRepositoryAdapter) setSuspended(reason string) {
obj.HelmRepository.Spec.Suspend = true
obj.HelmRepository.Annotations[SuspendReasonAnnotation] = reason
}

func (a helmRepositoryListAdapter) item(i int) suspendable {
Expand Down
3 changes: 2 additions & 1 deletion cmd/flux/suspend_source_oci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (obj ociRepositoryAdapter) isSuspended() bool {
return obj.OCIRepository.Spec.Suspend
}

func (obj ociRepositoryAdapter) setSuspended() {
func (obj ociRepositoryAdapter) setSuspended(reason string) {
obj.OCIRepository.Spec.Suspend = true
obj.OCIRepository.Annotations[SuspendReasonAnnotation] = reason
}

func (a ociRepositoryListAdapter) item(i int) suspendable {
Expand Down