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

Jump to bottom

✨ Support uninstall built-in addon #435

Merged
merged 2 commits into from
Jun 21, 2024
Merged

✨ Support uninstall built-in addon #435

merged 2 commits into from
Jun 21, 2024

Conversation

qiujian16
Copy link
Member

Summary

Related issue(s)

Fixes #339

@openshift-ci openshift-ci bot requested review from gparvin and itdove June 7, 2024 07:15
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jun 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved label Jun 7, 2024
@qiujian16
Copy link
Member Author

/assign @mikeshng @JustinKuli

@qiujian16 qiujian16 force-pushed the uninstall-addon branch 3 times, most recently from d0e5bfe to 5fe3ba4 Compare June 12, 2024 07:27
dhaiducek
dhaiducek suggested changes Jun 13, 2024
View reviewed changes
Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

Overall looks good! I've mentioned one important caveat around the PlacementRule CRD that will require an update.
I've also proposed adding an addon deployment map (though it'd result in refactoring some of the code).

pkg/cmd/install/hubaddon/scenario/resources.go Outdated
"addon/policy/propagator_serviceaccount.yaml",
"addon/policy/clustermanagementaddon_configpolicy.yaml",
"addon/policy/clustermanagementaddon_policyframework.yaml",
"addon/appmgr/crd_placementrule.yaml",
Copy link
Member

Choose a reason for hiding this comment

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

This PlacementRule CRD is a special case that needs to be handled and should only be cleaned up if both addons are uninstalled.

Copy link
Member Author

Choose a reason for hiding this comment

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

does policy still have a hard deps on placementRule, or we just do not create this crd when installing?

Copy link
Member

Choose a reason for hiding this comment

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

Currently the governance-policy-propagator controller won't start properly unless the PlacementRule CRD exists.

Copy link
Member Author

@qiujian16 qiujian16 Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

updated, will no delete any crd right now during uninstall.

Copy link
Member

Choose a reason for hiding this comment

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

FYI: In the next OCM release, Policy no longer has a dependency on PlacementRule, so addon CRD cleanup can proceed in clusteradm.

pkg/cmd/install/hubaddon/scenario/resources.go Outdated
Comment on lines 11 to 12
var (
PolicyFrameworkConfigFiles = []string{
Copy link
Member

Choose a reason for hiding this comment

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

I really like this new configuration file. What would you think of creating a struct and a map above it that also has the addon name as a key so that the install/uninstall doesn't need to specify names, something like:

var addonDeployments = map[string]AddonDeployment{
	"governance-policy-framework": {
		ConfigFiles: PolicyFrameworkConfigFiles,
		DeploymentFiles: PolicyFrameworkDeploymentFiles,
	},
	"application-manager": {
		ConfigFiles: AppManagerConfigFiles,
		DeploymentFiles: AppManagerDeploymentFiles,
	},
}

type AddonDeployment struct {
	ConfigFiles []string
	DeploymentFiles []string
}

@qiujian16 qiujian16 force-pushed the uninstall-addon branch 3 times, most recently from 0d7590c to 00a17fd Compare June 18, 2024 08:12
zhujian7
zhujian7 reviewed Jun 21, 2024
View reviewed changes
pkg/cmd/uninstall/cmd.go Outdated
genericclioptionsclusteradm "open-cluster-management.io/clusteradm/pkg/genericclioptions"
)

// NewCmd provides a cobra command wrapping NewCmdImportCluster
Copy link
Member

Choose a reason for hiding this comment

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

what does the NewCmdImportCluster mean here?

@zhujian7
Copy link
Member

when I run clusteradm uninstall hub-addon --help, I got, not sure if these can be optimized?

╰─# clusteradm uninstall hub-addon --help
Install specific built-in add-on(s) to the hub cluster

Examples:

# Install built-in add-ons to the hub cluster
clusteradm install hub-addon --names application-manager
clusteradm install hub-addon --names governance-policy-framework

zhujian7
zhujian7 reviewed Jun 21, 2024
View reviewed changes
pkg/cmd/uninstall/cmd.go Outdated
import (
"github.com/spf13/cobra"
"k8s.io/cli-runtime/pkg/genericclioptions"
hubaddon "open-cluster-management.io/clusteradm/pkg/cmd/install/hubaddon"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hubaddon "open-cluster-management.io/clusteradm/pkg/cmd/install/hubaddon"
hubaddon "open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon"

?

Copy link
Member

Choose a reason for hiding this comment

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

@qiujian16 I think this must be fixed.

pkg/cmd/uninstall/hubaddon/cmd.go Outdated

var example = `
# Uninstall built-in add-ons from the hub cluster
%[1]s install hub-addon --names application-manager
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
%[1]s install hub-addon --names application-manager
%[1]s uninstall hub-addon --names application-manager

pkg/cmd/uninstall/hubaddon/cmd.go Outdated
var example = `
# Uninstall built-in add-ons from the hub cluster
%[1]s install hub-addon --names application-manager
%[1]s install hub-addon --names governance-policy-framework
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
%[1]s install hub-addon --names governance-policy-framework
%[1]s uninstall hub-addon --names governance-policy-framework

pkg/cmd/uninstall/hubaddon/cmd.go Outdated

cmd := &cobra.Command{
Use: "hub-addon",
Short: "install hub-addon",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Short: "install hub-addon",
Short: "uninstall hub-addon",

pkg/cmd/uninstall/hubaddon/cmd.go Outdated
cmd := &cobra.Command{
Use: "hub-addon",
Short: "install hub-addon",
Long: "Install specific built-in add-on(s) to the hub cluster",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Long: "Install specific built-in add-on(s) to the hub cluster",
Long: "Uninstall specific built-in add-on(s) to the hub cluster",

pkg/cmd/uninstall/hubaddon/cmd.go Outdated
},
}

cmd.Flags().StringVar(&o.names, "names", "", "Names of the built-in add-on to install (comma separated). The built-in add-ons are: application-manager, governance-policy-framework")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd.Flags().StringVar(&o.names, "names", "", "Names of the built-in add-on to install (comma separated). The built-in add-ons are: application-manager, governance-policy-framework")
cmd.Flags().StringVar(&o.names, "names", "", "Names of the built-in add-on to uninstall (comma separated). The built-in add-ons are: application-manager, governance-policy-framework")

pkg/cmd/uninstall/hubaddon/cmd.go Outdated
}

cmd.Flags().StringVar(&o.names, "names", "", "Names of the built-in add-on to install (comma separated). The built-in add-ons are: application-manager, governance-policy-framework")
cmd.Flags().StringVar(&o.values.Namespace, "namespace", "open-cluster-management", "Namespace of the built-in add-on to install. Defaults to open-cluster-management")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd.Flags().StringVar(&o.values.Namespace, "namespace", "open-cluster-management", "Namespace of the built-in add-on to install. Defaults to open-cluster-management")
cmd.Flags().StringVar(&o.values.Namespace, "namespace", "open-cluster-management", "Namespace of the built-in add-on to uninstall. Defaults to open-cluster-management")

@zhujian7
Copy link
Member

I think we need to do more tests on the new uninstall command.

╰─# clusteradm uninstall hub-addon --names application-manager
panic: template: addon/appmgr/deployment_channel.yaml:22:95: executing "addon/appmgr/deployment_channel.yaml" at <.BundleVersion.AppAddon>: can't evaluate field BundleVersion in type hubaddon.Values

goroutine 1 [running]:
github.com/openshift/library-go/pkg/assets.MustCreateAssetFromTemplate(...)
        /home/go/src/open-cluster-management.io/clusteradm/vendor/github.com/openshift/library-go/pkg/assets/assets.go:108
open-cluster-management.io/clusteradm/pkg/helpers/reader.(*ResourceReader).Delete(0xc0002a15e0, {0xc000118150?}, {0x33c35c0, 0xc000d25ec0}, {0xc00015e000?, 0x4, 0xc000132020?})
        /home/go/src/open-cluster-management.io/clusteradm/pkg/helpers/reader/reader.go:145 +0x6a7
open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon.(*Options).runWithClient(0xc0002524d0)
        /home/go/src/open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon/exec.go:79 +0x28d
open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon.(*Options).run(0xc0002524d0)
        /home/go/src/open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon/exec.go:58 +0x348
open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon.NewCmd.func2(0xc0009f8b00?, {0xc000a18380?, 0x4?, 0x3695cd9?})
        /home/go/src/open-cluster-management.io/clusteradm/pkg/cmd/uninstall/hubaddon/cmd.go:42 +0x4f
github.com/spf13/cobra.(*Command).execute(0xc000a0b200, {0xc000a18360, 0x2, 0x2})
        /home/go/src/open-cluster-management.io/clusteradm/vendor/github.com/spf13/cobra/command.go:983 +0xabc
github.com/spf13/cobra.(*Command).ExecuteC(0xc00072cf00)
        /home/go/src/open-cluster-management.io/clusteradm/vendor/github.com/spf13/cobra/command.go:1115 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
        /home/go/src/open-cluster-management.io/clusteradm/vendor/github.com/spf13/cobra/command.go:1039
main.main()
        /home/go/src/open-cluster-management.io/clusteradm/cmd/clusteradm/clusteradm.go:130 +0xc73

zhujian7
zhujian7 reviewed Jun 21, 2024
View reviewed changes
pkg/cmd/uninstall/hubaddon/exec.go
return fmt.Errorf("invalid add-on name %s", n)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

we can set a default latest bundleversion here to fix #435 (comment)

Suggested change
versionBundle, err := version.GetVersionBundle("latest")
if err != nil {
return err
}
o.values.BundleVersion = versionBundle

@zhujian7
Copy link
Member

LGTM

@zhujian7
Copy link
Member

/lgtm
/hold

@openshift-ci openshift-ci bot added the lgtm label Jun 21, 2024
@qiujian16
Copy link
Member Author

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 261c92a into open-cluster-management-io:main Jun 21, 2024
8 checks passed
@qiujian16 qiujian16 deleted the uninstall-addon branch July 19, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhaiducek dhaiducek dhaiducek requested changes

@zhujian7 zhujian7 zhujian7 left review comments

@gparvin gparvin Awaiting requested review from gparvin

@itdove itdove Awaiting requested review from itdove

Assignees

@dhaiducek dhaiducek

@zhujian7 zhujian7

@JustinKuli JustinKuli

@mikeshng mikeshng

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

no command to remove addon application-manager in hub cluster
5 participants
@qiujian16 @zhujian7 @dhaiducek @JustinKuli @mikeshng