Skip to content

Commit

Permalink
Merge pull request #95 from nokia/cli-retry-updates
Browse files Browse the repository at this point in the history 
 Retry 'Update' operations that are failing due to normal optimistic locking behavior in CLI commands
  • Loading branch information
@efiacor
efiacor authored Aug 29, 2024
2 parents 50e369e + 74aaa44 commit aff3f7a
Show file tree
Hide file tree
Showing 11 changed files with 121 additions and 111 deletions.
23 changes: 12 additions & 11 deletions internal/kpt/util/porch/approval.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,25 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

func UpdatePackageRevisionApproval(ctx context.Context, client client.Client, key client.ObjectKey, new v1alpha1.PackageRevisionLifecycle) error {

var pr v1alpha1.PackageRevision
if err := client.Get(ctx, key, &pr); err != nil {
return err
}
func UpdatePackageRevisionApproval(ctx context.Context, client client.Client, pr *v1alpha1.PackageRevision, new v1alpha1.PackageRevisionLifecycle) error {

switch lifecycle := pr.Spec.Lifecycle; lifecycle {
case v1alpha1.PackageRevisionLifecycleProposed, v1alpha1.PackageRevisionLifecycleDeletionProposed:
// ok
case v1alpha1.PackageRevisionLifecycleProposed:
// Approve - change the package revision kind to 'final'.
if new != v1alpha1.PackageRevisionLifecyclePublished && new != v1alpha1.PackageRevisionLifecycleDraft {
return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new)
}
case v1alpha1.PackageRevisionLifecycleDeletionProposed:
if new != v1alpha1.PackageRevisionLifecyclePublished {
return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new)
}
case new:
// already correct value
return nil
default:
return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new)
}
// Approve - change the package revision kind to "final".
pr.Spec.Lifecycle = new
return client.SubResource("approval").Update(ctx, &pr)

pr.Spec.Lifecycle = new
return client.SubResource("approval").Update(ctx, pr)
}
14 changes: 11 additions & 3 deletions pkg/cli/commands/rpkg/approve/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs"
"github.com/spf13/cobra"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -83,17 +84,24 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
namespace := *r.cfg.Namespace

for _, name := range args {
if err := porch.UpdatePackageRevisionApproval(r.ctx, r.client, client.ObjectKey{
key := client.ObjectKey{
Namespace: namespace,
Name: name,
}, v1alpha1.PackageRevisionLifecyclePublished); err != nil {
}
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
var pr v1alpha1.PackageRevision
if err := r.client.Get(r.ctx, key, &pr); err != nil {
return err
}
return porch.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, v1alpha1.PackageRevisionLifecyclePublished)
})
if err != nil {
messages = append(messages, err.Error())
fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err)
} else {
fmt.Fprintf(r.Command.OutOrStdout(), "%s approved\n", name)
}
}

if len(messages) > 0 {
return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n ")))
}
Expand Down
46 changes: 24 additions & 22 deletions pkg/cli/commands/rpkg/propose/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs"
"github.com/spf13/cobra"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -83,33 +84,34 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
namespace := *r.cfg.Namespace

for _, name := range args {
pr := &v1alpha1.PackageRevision{}
if err := r.client.Get(r.ctx, client.ObjectKey{
key := client.ObjectKey{
Namespace: namespace,
Name: name,
}, pr); err != nil {
return errors.E(op, err)
}

switch pr.Spec.Lifecycle {
case v1alpha1.PackageRevisionLifecycleDraft:
// ok
case v1alpha1.PackageRevisionLifecycleProposed:
fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed\n", name)
continue
default:
msg := fmt.Sprintf("cannot propose %s package", pr.Spec.Lifecycle)
messages = append(messages, msg)
fmt.Fprintln(r.Command.ErrOrStderr(), msg)
continue
}

pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleProposed
if err := r.client.Update(r.ctx, pr); err != nil {
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
var pr v1alpha1.PackageRevision
if err := r.client.Get(r.ctx, key, &pr); err != nil {
return err
}

switch pr.Spec.Lifecycle {
case v1alpha1.PackageRevisionLifecycleDraft:
pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleProposed
err := r.client.Update(r.ctx, &pr)
if err == nil {
fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed\n", name)
}
return err
case v1alpha1.PackageRevisionLifecycleProposed:
fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed\n", name)
return nil
default:
return fmt.Errorf("cannot propose %s package", pr.Spec.Lifecycle)
}
})
if err != nil {
messages = append(messages, err.Error())
fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err)
} else {
fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed\n", name)
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/commands/rpkg/propose/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestCmd(t *testing.T) {
lc: porchapi.PackageRevisionLifecycleDraft,
},
"Cannot propose package": {
output: "cannot propose Published package\n",
output: pkgRevName + " failed (cannot propose Published package)\n",
lc: porchapi.PackageRevisionLifecyclePublished,
wantErr: true,
},
Expand Down
48 changes: 25 additions & 23 deletions pkg/cli/commands/rpkg/proposedelete/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs"
"github.com/spf13/cobra"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -83,39 +84,40 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
namespace := *r.cfg.Namespace

for _, name := range args {
pr := &v1alpha1.PackageRevision{}
if err := r.client.Get(r.ctx, client.ObjectKey{
key := client.ObjectKey{
Namespace: namespace,
Name: name,
}, pr); err != nil {
return errors.E(op, err)
}

switch pr.Spec.Lifecycle {
case v1alpha1.PackageRevisionLifecyclePublished:
// ok
case v1alpha1.PackageRevisionLifecycleDeletionProposed:
fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed for deletion\n", name)
continue
default:
msg := fmt.Sprintf("can only propose published packages for deletion; package %s is not published", name)
messages = append(messages, msg)
fmt.Fprintln(r.Command.ErrOrStderr(), msg)
continue
}

pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleDeletionProposed
if err := r.client.Update(r.ctx, pr); err != nil {
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
var pr v1alpha1.PackageRevision
if err := r.client.Get(r.ctx, key, &pr); err != nil {
return err
}

switch pr.Spec.Lifecycle {
case v1alpha1.PackageRevisionLifecyclePublished:
pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleDeletionProposed
err := r.client.Update(r.ctx, &pr)
if err == nil {
fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed for deletion\n", name)
}
return err
case v1alpha1.PackageRevisionLifecycleDeletionProposed:
fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed for deletion\n", name)
return nil
default:
return fmt.Errorf("can only propose published packages for deletion; package %s is not published", name)
}

})
if err != nil {
messages = append(messages, err.Error())
fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err)
} else {
fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed for deletion\n", name)
}
}

if len(messages) > 0 {
return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n ")))
}

return nil
}
3 changes: 2 additions & 1 deletion pkg/cli/commands/rpkg/proposedelete/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,12 @@ func TestCmd(t *testing.T) {
ns string
}{
"Package not found in ns": {
output: pkgRevName + " failed (packagerevisions.porch.kpt.dev \"" + pkgRevName + "\" not found)\n",
ns: "doesnotexist",
wantErr: true,
},
"Package not published": {
output: "can only propose published packages for deletion; package " + pkgRevName + " is not published\n",
output: pkgRevName + " failed (can only propose published packages for deletion; package " + pkgRevName + " is not published)\n",
ns: "ns",
wantErr: true,
},
Expand Down
51 changes: 24 additions & 27 deletions pkg/cli/commands/rpkg/reject/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs"
"github.com/spf13/cobra"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -85,41 +86,37 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
var messages []string

namespace := *r.cfg.Namespace

var proposedFor string
for _, name := range args {
pr := &v1alpha1.PackageRevision{}
if err := r.client.Get(r.ctx, client.ObjectKey{
key := client.ObjectKey{
Namespace: namespace,
Name: name,
}, pr); err != nil {
return errors.E(op, err)
}
switch pr.Spec.Lifecycle {
case v1alpha1.PackageRevisionLifecycleProposed:
if err := porch.UpdatePackageRevisionApproval(r.ctx, r.client, client.ObjectKey{
Namespace: namespace,
Name: name,
}, v1alpha1.PackageRevisionLifecycleDraft); err != nil {
messages = append(messages, err.Error())
fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err)
} else {
fmt.Fprintf(r.Command.OutOrStdout(), "%s rejected\n", name)
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
var pr v1alpha1.PackageRevision
if err := r.client.Get(r.ctx, key, &pr); err != nil {
return err
}
case v1alpha1.PackageRevisionLifecycleDeletionProposed:
pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecyclePublished
if err := r.client.Update(r.ctx, pr); err != nil {
messages = append(messages, err.Error())
fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err)
} else {
fmt.Fprintf(r.Command.OutOrStdout(), "%s no longer proposed for deletion\n", name)
switch pr.Spec.Lifecycle {
case v1alpha1.PackageRevisionLifecycleProposed:
proposedFor = "approval"
return porch.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, v1alpha1.PackageRevisionLifecycleDraft)
case v1alpha1.PackageRevisionLifecycleDeletionProposed:
proposedFor = "deletion"
// NOTE(kispaljr): should we use UpdatePackageRevisionApproval() here?
pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecyclePublished
return r.client.Update(r.ctx, &pr)
default:
return fmt.Errorf("cannot reject %s with lifecycle '%s'", name, pr.Spec.Lifecycle)
}
default:
msg := fmt.Sprintf("cannot reject %s with lifecycle '%s'", name, pr.Spec.Lifecycle)
messages = append(messages, msg)
fmt.Fprintln(r.Command.ErrOrStderr(), msg)
})
if err != nil {
messages = append(messages, err.Error())
fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err)
} else {
fmt.Fprintf(r.Command.OutOrStdout(), "%s no longer proposed for %s\n", name, proposedFor)
}
}

if len(messages) > 0 {
return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n ")))
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/cli/commands/rpkg/reject/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ func TestCmd(t *testing.T) {
fakeclient client.WithWatch
}{
"Package not found in ns": {
output: pkgRevName + " failed (packagerevisions.porch.kpt.dev \"" + pkgRevName + "\" not found)\n",
wantErr: true,
fakeclient: fake.NewClientBuilder().WithScheme(scheme).
WithObjects(&porchapi.PackageRevision{
Expand Down Expand Up @@ -93,7 +94,7 @@ func TestCmd(t *testing.T) {
},
"Reject draft package": {
wantErr: true,
output: "cannot reject " + pkgRevName + " with lifecycle 'Draft'\n",
output: pkgRevName + " failed (cannot reject " + pkgRevName + " with lifecycle 'Draft')\n",
fakeclient: fake.NewClientBuilder().WithScheme(scheme).
WithObjects(&porchapi.PackageRevision{
TypeMeta: metav1.TypeMeta{
Expand All @@ -111,7 +112,7 @@ func TestCmd(t *testing.T) {
},
"Reject published package": {
wantErr: true,
output: "cannot reject " + pkgRevName + " with lifecycle 'Published'\n",
output: pkgRevName + " failed (cannot reject " + pkgRevName + " with lifecycle 'Published')\n",
fakeclient: fake.NewClientBuilder().WithScheme(scheme).
WithObjects(&porchapi.PackageRevision{
TypeMeta: metav1.TypeMeta{
Expand All @@ -128,7 +129,7 @@ func TestCmd(t *testing.T) {
}}).Build(),
},
"Reject proposed package": {
output: pkgRevName + " rejected\n",
output: pkgRevName + " no longer proposed for approval\n",
fakeclient: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{
//fake subresourceupdate
SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error {
Expand Down
11 changes: 10 additions & 1 deletion pkg/cli/commands/rpkg/update/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs"
"github.com/spf13/cobra"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/util/retry"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -114,7 +115,15 @@ func (r *runner) runE(cmd *cobra.Command, args []string) error {
if pr == nil {
return errors.E(op, fmt.Errorf("could not find package revision %s", args[0]))
}
if err := r.doUpdate(pr); err != nil {
key := client.ObjectKeyFromObject(pr)
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
var pr porchapi.PackageRevision
if err := r.client.Get(r.ctx, key, &pr); err != nil {
return err
}
return r.doUpdate(&pr)
})
if err != nil {
return errors.E(op, err)
}
if _, err := fmt.Fprintf(cmd.OutOrStdout(), "%s updated\n", pr.Name); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion scripts/cleanup-after-tests.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ rm -rf "$self_dir/../.cache"

kubectl get packagerev -A --no-headers | awk '{ print "-n", $1, $2 }' | xargs -L 1 --no-run-if-empty kubectl patch packagerev --type='json' -p='[{"op": "remove", "path": "/metadata/finalizers"}]'

kubectl get ns -o name --no-headers | grep test | xargs -L 1 --no-run-if-empty kubectl delete
kubectl get ns -o name --no-headers | egrep '(test|rpkg-|repo-)' | xargs -L 1 --no-run-if-empty kubectl delete
Loading

0 comments on commit aff3f7a

Please sign in to comment.