Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

make package install idempotent #983

Merged
merged 2 commits into from
Oct 28, 2021

Conversation

maralavi
Copy link
Contributor

@maralavi maralavi commented Oct 26, 2021

What this PR does / why we need it:

Describe testing done for PR:
Tested manually in the cluster, also improved unit test coverage as part of the PR and tested with existing integration tests

Does this PR introduce a user-facing change?:
None

Release note:

 Changed to make "package install" and "package repository add" idempotent. Improved "package delete" experience by explicitly naming any related resources being deleted and removing the misleading resource deletion message

Example outputs:
When trying to install a package with invalid configuration:

tanzu package installed create test-pkg --package-name pkg.test.carvel.dev --version 2.0.0 --namespace test-ns --create-namespace --values-file config/values.yaml --wait=true
- Installing package 'pkg.test.carvel.dev'
- Getting package metadata for 'pkg.test.carvel.dev'
| Creating namespace 'test-ns'
| Creating service account 'test-pkg-test-ns-sa'
| Creating cluster admin role 'test-pkg-test-ns-cluster-role'
| Creating cluster role binding 'test-pkg-test-ns-cluster-rolebinding'
| Creating secret 'test-pkg-test-ns-values'

Error: failed to read from data values file 'config/values.yaml': open config/values.yaml: no such file or directory
Error: exit status 1

✖  exit status 1

Fixing the problem and retrying:

tanzu package installed create test-pkg --package-name pkg.test.carvel.dev --version 2.0.0 --namespace test-ns --create-namespace --values-file config/values.yaml --wait=true
- Installing package 'pkg.test.carvel.dev'
- Getting package metadata for 'pkg.test.carvel.dev'
| Creating namespace 'test-ns'
/ Creating service account 'test-pkg-test-ns-sa'
- Creating cluster admin role 'test-pkg-test-ns-cluster-role'
/ Creating cluster role binding 'test-pkg-test-ns-cluster-rolebinding'
| Creating secret 'test-pkg-test-ns-values'
/ Creating package resource
- Waiting for 'PackageInstall' reconciliation for 'test-pkg'
- 'PackageInstall' resource install status: Reconciling

Added installed package 'test-pkg' in namespace 'test-ns'

When trying to install an already existing package, if either of version or value-file be changed, those will get updated:

tanzu package installed create test-pkg --package-name pkg.test.carvel.dev --version 2.0.0 --namespace test-ns --create-namespace --values-file config/values.yaml --wait=true
/ Installing package 'pkg.test.carvel.dev'

| Getting package install for 'test-pkg'
- Getting package metadata for 'pkg.test.carvel.dev'
| Updating secret 'test-pkg-test-ns-values'
| Updating package install for 'test-pkg'
- Waiting for 'PackageInstall' reconciliation for 'test-pkg'


Updated installed package 'test-pkg' in namespace 'test-ns'

When trying to delete a package which was failed to be installed, any resources that is installed get deleted explicitly:

tanzu package installed delete test-pkg --namespace test-ns --poll-interval 20s --poll-timeout 10m0s -y
| Uninstalling package 'test-pkg' from namespace 'test-ns'
\ Getting package install for 'test-pkg'
/ Deleting role binding 'test-pkg-test-ns-cluster-rolebinding'
/ Deleting admin role 'test-pkg-test-ns-cluster-role'
/ Deleting service account 'test-pkg-test-ns-sa'

package 'test-pkg' is not installed in namespace 'test-ns'.

Deleting an existing the package:

tanzu package installed delete test-pkg --namespace test-ns --poll-interval 20s --poll-timeout 10m0s -y
| Uninstalling package 'test-pkg' from namespace 'test-ns'
/ Getting package install for 'test-pkg'
\ Deleting package install 'test-pkg' from namespace 'test-ns'
/ Deleting admin role 'test-pkg-test-ns-cluster-role'
/ Deleting role binding 'test-pkg-test-ns-cluster-rolebinding'
| Deleting secret 'test-pkg-test-ns-values'
| Deleting service account 'test-pkg-test-ns-sa'
Uninstalled package 'test-pkg' from namespace 'test-ns'

Trying to delete a non-existing package:

tanzu package installed delete test-pkg --namespace test-ns --poll-interval 20s --poll-timeout 10m0s -y
| Uninstalling package 'test-pkg' from namespace 'test-ns'
/ Getting package install for 'test-pkg'

package 'test-pkg' is not installed in namespace 'test-ns'.

New PR Checklist

  • Ensure PR contains only public links or terms
  • Use good commit messages
  • Squash the commits in this branch before merge to preserve our git history
  • If this PR is just an idea or POC, use a Draft PR instead of a full PR
  • Add appropriate kind label according to what type of issue is being addressed.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/983/20211026203025/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/983/20211026203817/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@vuil vuil added the ok-to-merge PRs should be labelled with this before merging label Oct 27, 2021
Copy link
Contributor

@shivaani0505 shivaani0505 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@danniel1205 danniel1205 left a comment

Choose a reason for hiding this comment

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

Have some minor comments, but they are not blockers. The changes look good to me. This PR has been reviewed and approved by component owner as well.

cmd/cli/plugin/package/repository_add.go Show resolved Hide resolved
pkg/v1/tkg/tkgpackageclient/package_install.go Outdated Show resolved Hide resolved
Signed-off-by: Marjan Alavi <malavi@vmware.com>
@maralavi maralavi force-pushed the make-package-install-idempotent branch from e7b5216 to 610abfa Compare October 28, 2021 00:39
Signed-off-by: Marjan Alavi <malavi@vmware.com>
@maralavi maralavi force-pushed the make-package-install-idempotent branch from 610abfa to be767d4 Compare October 28, 2021 00:46
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/983/20211028005146/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/983/20211028005227/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/983/20211028005903/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@maralavi maralavi merged commit aa516ff into vmware-tanzu:main Oct 28, 2021
@maralavi maralavi deleted the make-package-install-idempotent branch October 28, 2021 17:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
5 participants