MGMT-15704 Assisted service should create Day2 import CR for hub cluster. #5459

paul-maidment · 2023-09-05T07:42:22Z

When using assisted-service in KubeAPI mode, we want to import the local cluster automatically, allowing it to be managed in a similar fashion to the spoke cluster.

The intent is to allow the user to perform day2 operations on the local cluster, to allow the addition of workers and so on.

Presently this is only possible via manual efforts and is not very customer friendly.

This PR aims to resolve this by adding functionality to import the local cluster as described above.

The behaviour during upgrade will be dealt with in another ticket.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci · 2023-09-05T07:42:48Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

filanov · 2023-09-05T09:53:06Z

Using ZTP in the commit message is incorrect, here you are referring to kube-api or the operator, have nothing to do with ZTP

filanov · 2023-09-05T09:53:31Z

/assign @nmagnezi

codecov · 2023-09-05T10:06:49Z

Codecov Report

Merging #5459 (eeca32d) into master (52448cd) will decrease coverage by 0.06%.
Report is 4 commits behind head on master.
The diff coverage is 63.72%.

❗ Current head eeca32d differs from pull request most recent head c2aabc3. Consider uploading reports for the commit c2aabc3 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5459      +/-   ##
==========================================
- Coverage   67.74%   67.68%   -0.06%     
==========================================
  Files         229      232       +3     
  Lines       33571    33968     +397     
==========================================
+ Hits        22741    22992     +251     
- Misses       8790     8929     +139     
- Partials     2040     2047       +7

Files Changed	Coverage Δ
...oller/controllers/clusterdeployments_controller.go	`71.23% <ø> (ø)`
...alclusterimport/local_cluster_import_operations.go	`0.00% <0.00%> (ø)`
...terimport/local_cluster_import_operations_mocks.go	`76.10% <76.10%> (ø)`
pkg/localclusterimport/import_local_cluster.go	`90.27% <90.27%> (ø)`

... and 1 file with indirect coverage changes

filanov

Please add in the commit message why did you had to add permissions to specific resources

General comment about if else
usually it's cleaner to handle the error first, then you will not need the else in most cases

cmd/main.go

pkg/localclusterimport/import_local_cluster.go

cmd/main.go

pkg/localclusterimport/import_local_cluster.go

nmagnezi · 2023-09-05T12:18:51Z

pkg/localclusterimport/import_local_cluster.go

+		return false, err
+	}
+	if agentClusterInstall != nil {
+		i.log.Infof("hubClusterDay2Importer: Found AgentClusterInstall for hub cluster, assuming that hub has been correctly registered for ZTP day 2 operations: %s", agentClusterInstall.Name)


Are there any checks we can do here to ensure that cluster is actually healthy?
Also, why is ZTP mentioned here?

I'm not sure that this is possible at this stage.

The one thing we do perform is a check of the cluster-version history, which will show up as completed if the last update was applied.

i'm not sure which tests can be done in this case, domain resolution and valid kubeconfig, but if you are taking them from the cluster and even if they are invalid, what can be done?

pkg/localclusterimport/import_local_cluster.go

nmagnezi · 2023-09-05T12:31:03Z

pkg/localclusterimport/import_local_cluster.go

+	return true, nil
+}
+
+func (i *LocalClusterImport) ImportLocalCluster() error {


Few things about this function:

It does to many things, which should be split to sub functions for both readability and tests.

It can potentially fail in too many places (which is related to the first point, in part). I also think that for many things here it is worth to use multiErr to aggregate a much of the problems and reflect them to the user as early as possible.

1: Done
2: Done

paul-maidment · 2023-09-06T07:57:58Z

Using ZTP in the commit message is incorrect, here you are referring to kube-api or the operator, have nothing to do with ZTP

Fixed now

Using ZTP in the commit message is incorrect, here you are referring to kube-api or the operator, have nothing to do with ZTP

Fixed

filanov · 2023-09-06T08:39:57Z

cmd/main.go

@@ -205,6 +208,23 @@ func maxDuration(dur time.Duration, durations ...time.Duration) time.Duration {
 	return ret
 }

+func importLocalCluster(ctrlMgr manager.Manager, log *logrus.Logger) {
+	if !Options.EnableLocalClusterImport {
+		log.Debug("EnableLocalClusterImport disabled in options, skipping...")


can be info, it's not something periodic

filanov · 2023-09-06T08:40:30Z

cmd/main.go

@@ -162,6 +163,8 @@ var Options struct {
 	AllowConvergedFlow                   bool          `envconfig:"ALLOW_CONVERGED_FLOW" default:"true"`
 	PreprovisioningImageControllerConfig controllers.PreprovisioningImageControllerConfig
 	BMACConfig                           controllers.BMACConfig
+	EnableLocalClusterImport             bool   `envconfig:"ENABLE_LOCAL_CLUSTER_IMPORT" default:"true"`
+	LocalClusterImportNamespace          string `envconfig:"LOCAL_CLUSTER_IMPORT_NAMESPACE" defualt:"local-cluster"`


please make sure with ACM/MCE that they are not using the same namespace

filanov · 2023-09-06T08:44:04Z

pkg/localclusterimport/local_cluster_import_operations.go

+}
+
+func (o *LocalClusterImportOperations) CreateClusterImageSet(clusterImageSet *hivev1.ClusterImageSet) (*hivev1.ClusterImageSet, error) {
+	err := o.cachedApiClient.Create(o.context, clusterImageSet)


what will happen is it's already exists?
same question to all the resources.

It would fail to write the cluster image set and the old one would remain in place.
Not sure what the best thing to do is, on one hand, we could delete anything we find when performing this write, but seems to me that this would not be the smart choice until we have looked into the impact of the update ticket.

until we handle upgrade it should be good enough

filanov · 2023-09-06T09:39:20Z

pkg/localclusterimport/import_local_cluster.go

+
+func (i *LocalClusterImport) ImportLocalCluster() error {
+
+	shouldImportLocalCluster, err := i.shouldImportLocalCluster()


As we talked, this function can be removed and you will try to create all the objects and if they are already exist you will ignore them, using k8serrors.IsAlreadyExists(err) to check the error

filanov · 2023-09-06T12:52:42Z

pkg/localclusterimport/import_local_cluster.go

+	}
+	// Fetch the recently stored AgentClusterInstall so that we can obtain the UID
+	aci, err := i.clusterImportOperations.GetAgentClusterInstall(i.localClusterNamespace, i.localClusterNamespace+"-cluster-install")
+	if err != nil && !k8serrors.IsAlreadyExists(err) {


get cannot return already exist

filanov · 2023-09-06T12:53:28Z

pkg/localclusterimport/import_local_cluster.go

+	agentClusterInstall.Namespace = i.localClusterNamespace
+	agentClusterInstall.Name = i.localClusterNamespace + "-cluster-install"
+	err := i.clusterImportOperations.CreateAgentClusterInstall(agentClusterInstall)
+	if err != nil {


it can return already exist

filanov · 2023-09-06T12:56:28Z

pkg/localclusterimport/import_local_cluster.go

+	}
+
+	err = i.createNamespace(i.localClusterNamespace)
+	if err != nil && !k8serrors.IsAlreadyExists(err) {


minor comment - you already handling already exist error inside createNamespace

I actually changed this, I need to be able to mock this so I moved the handling of this stuff into import_local_cluster

filanov · 2023-09-06T12:56:48Z

pkg/localclusterimport/import_local_cluster.go

+
+	if kubeConfigSecret != nil {
+		err = i.createAdminKubeConfig(kubeConfigSecret)
+		if err != nil && !k8serrors.IsAlreadyExists(err) {


minor comment - you already handling already exist error inside createAdminKubeConfig

I actually changed this, I need to be able to mock this so I moved the handling of this stuff into import_local_cluster

…ster. When using assisted-service in KubeAPI mode, we want to import the local cluster automatically, allowing it to be managed in a similar fashion to the spoke cluster. The intent is to allow the user to perform day2 operations on the local cluster, to allow the addition of workers and so on. Presently this is only possible via manual efforts and is not very customer friendly. This PR aims to resolve this by adding functionality to import the local cluster as described above.

paul-maidment · 2023-09-06T20:20:29Z

/retest

openshift-ci · 2023-09-06T21:27:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: filanov, paul-maidment

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:

~~OWNERS~~ [filanov,paul-maidment]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-09-06T21:47:16Z

@paul-maidment: all tests passed!

Full PR test history. Your PR dashboard.

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.

paul-maidment · 2023-09-10T10:47:53Z

/cherry-pick release-ocm-2.8

openshift-cherrypick-robot · 2023-09-10T10:48:49Z

@paul-maidment: #5459 failed to apply on top of branch "release-ocm-2.8":

Applying: MGMT-15704: Assisted service should create Day2 import CR for hub cluster.
Using index info to reconstruct a base tree...
M	cmd/main.go
M	deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml
M	go.mod
M	internal/controller/controllers/clusterdeployments_controller.go
Falling back to patching base and 3-way merge...
Auto-merging internal/controller/controllers/clusterdeployments_controller.go
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml
Auto-merging cmd/main.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 MGMT-15704: Assisted service should create Day2 import CR for hub cluster.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-ocm-2.8

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.

…ster. (openshift#5459) When using assisted-service in KubeAPI mode, we want to import the local cluster automatically, allowing it to be managed in a similar fashion to the spoke cluster. The intent is to allow the user to perform day2 operations on the local cluster, to allow the addition of workers and so on. Presently this is only possible via manual efforts and is not very customer friendly. This PR aims to resolve this by adding functionality to import the local cluster as described above.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2023

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 5, 2023

paul-maidment mentioned this pull request Sep 5, 2023

MGMT-15362: Import local cluster into ZTP #5433

Closed

20 tasks

paul-maidment force-pushed the MGMT-15704 branch 7 times, most recently from 1060b2c to f01d801 Compare September 5, 2023 09:43

paul-maidment marked this pull request as ready for review September 5, 2023 09:49

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2023

openshift-ci bot requested review from avishayt and eranco74 September 5, 2023 09:49

openshift-ci bot assigned nmagnezi Sep 5, 2023

filanov reviewed Sep 5, 2023

View reviewed changes

pkg/localclusterimport/import_local_cluster.go Outdated Show resolved Hide resolved

paul-maidment force-pushed the MGMT-15704 branch 4 times, most recently from 3d6c930 to 85f69f1 Compare September 5, 2023 12:27

nmagnezi suggested changes Sep 5, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-15704 branch 3 times, most recently from 8d0af57 to eeca32d Compare September 5, 2023 21:19

paul-maidment force-pushed the MGMT-15704 branch 8 times, most recently from 9a2f924 to 23bc5d3 Compare September 6, 2023 07:56

paul-maidment force-pushed the MGMT-15704 branch from 23bc5d3 to f638b37 Compare September 6, 2023 08:07

filanov reviewed Sep 6, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-15704 branch 4 times, most recently from 9210a94 to 7d5ff3d Compare September 6, 2023 12:43

filanov reviewed Sep 6, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-15704 branch from 7d5ff3d to 8ee1b33 Compare September 6, 2023 13:51

paul-maidment force-pushed the MGMT-15704 branch from 8ee1b33 to c2aabc3 Compare September 6, 2023 14:45

filanov approved these changes Sep 6, 2023

View reviewed changes

openshift-ci bot assigned filanov Sep 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023

openshift-merge-robot merged commit 05bc6d8 into openshift:master Sep 6, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-15704 Assisted service should create Day2 import CR for hub cluster. #5459

MGMT-15704 Assisted service should create Day2 import CR for hub cluster. #5459

paul-maidment commented Sep 5, 2023 •

edited

openshift-ci bot commented Sep 5, 2023

filanov commented Sep 5, 2023

filanov commented Sep 5, 2023

codecov bot commented Sep 5, 2023 •

edited

filanov left a comment

nmagnezi Sep 5, 2023

paul-maidment Sep 5, 2023

filanov Sep 6, 2023

nmagnezi Sep 5, 2023

paul-maidment Sep 6, 2023

paul-maidment commented Sep 6, 2023

filanov Sep 6, 2023

filanov Sep 6, 2023

filanov Sep 6, 2023

paul-maidment Sep 6, 2023

filanov Sep 6, 2023

filanov Sep 6, 2023

filanov Sep 6, 2023

filanov Sep 6, 2023

filanov Sep 6, 2023

paul-maidment Sep 6, 2023

filanov Sep 6, 2023

paul-maidment Sep 6, 2023

paul-maidment commented Sep 6, 2023

openshift-ci bot commented Sep 6, 2023

openshift-ci bot commented Sep 6, 2023

paul-maidment commented Sep 10, 2023

openshift-cherrypick-robot commented Sep 10, 2023


		func (i *LocalClusterImport) ImportLocalCluster() error {

		shouldImportLocalCluster, err := i.shouldImportLocalCluster()

MGMT-15704 Assisted service should create Day2 import CR for hub cluster. #5459

MGMT-15704 Assisted service should create Day2 import CR for hub cluster. #5459

Conversation

paul-maidment commented Sep 5, 2023 • edited

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Sep 5, 2023

filanov commented Sep 5, 2023

filanov commented Sep 5, 2023

codecov bot commented Sep 5, 2023 • edited

Codecov Report

filanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-maidment commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-maidment commented Sep 6, 2023

openshift-ci bot commented Sep 6, 2023

openshift-ci bot commented Sep 6, 2023

paul-maidment commented Sep 10, 2023

openshift-cherrypick-robot commented Sep 10, 2023

paul-maidment commented Sep 5, 2023 •

edited

codecov bot commented Sep 5, 2023 •

edited