search

openshift / assisted-service

Apache License 2.0
109 stars 209 forks source link

MGMT-18659: Local cluster name should be picked up from ACM ManagedCluster label #6696

Closed paul-maidment closed 2 weeks ago

paul-maidment commented 2 weeks ago

When performing a local cluster import - the local cluster name is hardwired to "local-cluster", this is not flexible enough and needs to be changed. We should be picking up the name from the ManagedCluster that is labelled as "local-cluster" - this should then be used wherever we use the present local-cluster name. An additional check is performed to ensure that the clusterID referenced in the labels of ManagedCluster matches the clusterID in the labels found in the clusterVersion of the hub on which the LocalClusterImport is running.

This should be sufficient at this stage as we do not need to handle scenarios such as a "rename", mainly because ACM will be enforcing some rules

From ACM-DDR-022:

To ensure backward compatibility, we will not directly allow renaming the local cluster from MCE or ACM installer at the first stage. The user can choose a customized local cluster name with the following step:
Disable local cluster management in MCE or ACM when install
Manually create a ManagedCluster resource with the label “local-cluster=true” on the hub

I have also added some entity cleanup that was missed in previous versions, mainly just making sure that unused secrets are deleted.

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

  1. Tested and verified to be correctly working in a dev-scripts environment with a custom operator bundle.
  2. AgentServiceConfig was created in a way that renders LocalClusterInstall active
  3. ManagedCluster was given the clusterID of the hub
  4. Expectation was that local managed cluster would have CR's created for it in the namespace specified by the name of ManagedCluster. This expectation was met.

Checklist

Reviewers Checklist

openshift-ci-robot commented 2 weeks ago

@paul-maidment: This pull request references MGMT-18563 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6696): >The present implementation of local-cluster-import only provides for a local cluster named "local-cluster" This is inflexible for users, especially in the face of recent ACM changes that allow a user to rename the local cluster ManagedCluster object. > >This PR gets rid of the hardwired default of "local-cluster" and instead will look for a ManagedCluster with the label "local-cluster", the name of this ManagedCluster will be used as the namespace and name of the imported local cluster. > >A an annotation will also be applied to any AgentClusterInstall, ClusterDeployment or InfraEnv that are created as part of this process, this is to allow auto deletion in the case that the ManagedCluster object is removed. > >Note: Legacy implementations will need to manually remove the entities named "local-cluster" post upgrade as these will not have been annotated under this scheme. > > > >## List all the issues related to this PR > >- [ ] New Feature >- [ ] Enhancement >- [ ] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [ ] Automation (CI, tools, etc) >- [ ] Cloud >- [ ] Operator Managed Deployments >- [x] 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) >- [x] 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? > >[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines >[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 2 weeks ago

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

openshift-ci-robot commented 2 weeks ago

@paul-maidment: This pull request references MGMT-18563 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6696): >The present implementation of local-cluster-import only provides for a local cluster named "local-cluster" This is inflexible for users, especially in the face of recent ACM changes that allow a user to rename the local cluster ManagedCluster object. > >This PR gets rid of the hardwired default of "local-cluster" and instead will look for a ManagedCluster with the label "local-cluster", the name of this ManagedCluster will be used as the namespace and name of the imported local cluster. > >An annotation will also be applied to any AgentClusterInstall, ClusterDeployment or InfraEnv that are created as part of this process, this is to allow auto deletion in the case that the ManagedCluster object is removed. > >Note: Legacy implementations will need to manually remove the entities named "local-cluster" post upgrade as these will not have been annotated under this scheme. > > > >## List all the issues related to this PR > >- [ ] New Feature >- [ ] Enhancement >- [ ] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [ ] Automation (CI, tools, etc) >- [ ] Cloud >- [ ] Operator Managed Deployments >- [x] 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) >- [x] 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? > >[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines >[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 2 weeks ago

@paul-maidment: This pull request references MGMT-18563 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6696): >When performing a local cluster import - the local cluster name is hardwired to "local-cluster", this is not flexible enough and needs to be changed. >We should be picking up the name from the ManagedCluster that is labelled as "local-cluster" - this should then be used wherever we use the present local-cluster name. >An additional check is performed to ensure that the clusterID referenced in the labels of ManagedCluster matches the clusterID in the labels found in the >clusterVersion of the hub on which the LocalClusterImport is running. > >This should be sufficient at this stage as we do not need to handle scenarios such as a "rename", mainly because ACM will be enforcing some rules > >From ACM-DDR-022: > >I have also added some entity cleanup that was missed in previous versions, mainly just making sure that unused secrets are deleted. >``` >To ensure backward compatibility, we will not directly allow renaming the local cluster from MCE or ACM installer at the first stage. The user can choose a customized local cluster name with the following step: >Disable local cluster management in MCE or ACM when install >Manually create a ManagedCluster resource with the label “local-cluster=true” on the hub >``` > >Additionally, I added cleanup for the secrets that we create in the local cluster namespace. > > > >## List all the issues related to this PR > >- [ ] New Feature >- [ ] Enhancement >- [x] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [x] Automation (CI, tools, etc) >- [x] Cloud >- [x] Operator Managed Deployments >- [x] None > >## How was this code tested? > > > >- [ ] assisted-test-infra environment >- [x] dev-scripts environment >- [ ] Reviewer's test appreciated >- [x] Waiting for CI to do a full test run >- [ ] Manual (Elaborate on how it was tested) >- [x] No tests needed > >## Checklist > >- [x] Title and description added to both, commit and PR. >- [x] Relevant issues have been associated (see [CONTRIBUTING] guide) >- [x] This change does not require a documentation update (docstring, `docs`, README, etc) >- [x] 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? > >[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines >[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 2 weeks ago

@paul-maidment: This pull request references MGMT-18563 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6696): >When performing a local cluster import - the local cluster name is hardwired to "local-cluster", this is not flexible enough and needs to be changed. >We should be picking up the name from the ManagedCluster that is labelled as "local-cluster" - this should then be used wherever we use the present local-cluster name. >An additional check is performed to ensure that the clusterID referenced in the labels of ManagedCluster matches the clusterID in the labels found in the >clusterVersion of the hub on which the LocalClusterImport is running. > >This should be sufficient at this stage as we do not need to handle scenarios such as a "rename", mainly because ACM will be enforcing some rules > >From ACM-DDR-022: > >``` >To ensure backward compatibility, we will not directly allow renaming the local cluster from MCE or ACM installer at the first stage. The user can choose a customized local cluster name with the following step: >Disable local cluster management in MCE or ACM when install >Manually create a ManagedCluster resource with the label “local-cluster=true” on the hub >``` > >I have also added some entity cleanup that was missed in previous versions, mainly just making sure that unused secrets are deleted. > > > >## List all the issues related to this PR > >- [ ] New Feature >- [ ] Enhancement >- [x] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [x] Automation (CI, tools, etc) >- [x] Cloud >- [x] Operator Managed Deployments >- [x] None > >## How was this code tested? > > > >- [ ] assisted-test-infra environment >- [x] dev-scripts environment >- [ ] Reviewer's test appreciated >- [x] Waiting for CI to do a full test run >- [ ] Manual (Elaborate on how it was tested) >- [x] No tests needed > >## Checklist > >- [x] Title and description added to both, commit and PR. >- [x] Relevant issues have been associated (see [CONTRIBUTING] guide) >- [x] This change does not require a documentation update (docstring, `docs`, README, etc) >- [x] 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? > >[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines >[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 53.22581% with 29 lines in your changes missing coverage. Please review.

Project coverage is 68.63%. Comparing base (169d6fa) to head (0ab14ac). Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...ler/controllers/local_cluster_import_controller.go 53.22% 21 Missing and 8 partials :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/openshift/assisted-service/pull/6696/graphs/tree.svg?width=650&height=150&src=pr&token=YOR4NSSOXQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)](https://app.codecov.io/gh/openshift/assisted-service/pull/6696?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) ```diff @@ Coverage Diff @@ ## master #6696 +/- ## ========================================== - Coverage 68.64% 68.63% -0.01% ========================================== Files 246 246 Lines 36860 36897 +37 ========================================== + Hits 25301 25324 +23 - Misses 9307 9314 +7 - Partials 2252 2259 +7 ``` | [Files with missing lines](https://app.codecov.io/gh/openshift/assisted-service/pull/6696?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) | Coverage Δ | | |---|---|---| | [...ler/controllers/local\_cluster\_import\_controller.go](https://app.codecov.io/gh/openshift/assisted-service/pull/6696?src=pr&el=tree&filepath=internal%2Fcontroller%2Fcontrollers%2Flocal_cluster_import_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift#diff-aW50ZXJuYWwvY29udHJvbGxlci9jb250cm9sbGVycy9sb2NhbF9jbHVzdGVyX2ltcG9ydF9jb250cm9sbGVyLmdv) | `61.53% <53.22%> (-0.53%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/openshift/assisted-service/pull/6696/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)
openshift-ci-robot commented 2 weeks ago

@paul-maidment: This pull request references MGMT-18563 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6696): >When performing a local cluster import - the local cluster name is hardwired to "local-cluster", this is not flexible enough and needs to be changed. >We should be picking up the name from the ManagedCluster that is labelled as "local-cluster" - this should then be used wherever we use the present local-cluster name. >An additional check is performed to ensure that the clusterID referenced in the labels of ManagedCluster matches the clusterID in the labels found in the >clusterVersion of the hub on which the LocalClusterImport is running. > >This should be sufficient at this stage as we do not need to handle scenarios such as a "rename", mainly because ACM will be enforcing some rules > >From ACM-DDR-022: > >``` >To ensure backward compatibility, we will not directly allow renaming the local cluster from MCE or ACM installer at the first stage. The user can choose a customized local cluster name with the following step: >Disable local cluster management in MCE or ACM when install >Manually create a ManagedCluster resource with the label “local-cluster=true” on the hub >``` > >I have also added some entity cleanup that was missed in previous versions, mainly just making sure that unused secrets are deleted. > > > >## List all the issues related to this PR > >- [ ] New Feature >- [ ] Enhancement >- [x] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [x] Automation (CI, tools, etc) >- [x] Cloud >- [x] Operator Managed Deployments >- [x] None > >## How was this code tested? > > > >- [ ] assisted-test-infra environment >- [x] dev-scripts environment > >1. Tested and verified to be correctly working in a dev-scripts environment with a custom operator bundle. >2. AgentServiceConfig was created in a way that renders LocalClusterInstall active >3. ManagedCluster was given the clusterID of the hub >4. Expectation was that local managed cluster would have CR's created for it in the namespace specified by the name of ManagedCluster. This expectation was met. > >- [ ] Reviewer's test appreciated >- [x] Waiting for CI to do a full test run >- [ ] Manual (Elaborate on how it was tested) >- [x] No tests needed > >## Checklist > >- [x] Title and description added to both, commit and PR. >- [x] Relevant issues have been associated (see [CONTRIBUTING] guide) >- [x] This change does not require a documentation update (docstring, `docs`, README, etc) >- [x] 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? > >[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines >[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 2 weeks ago

@paul-maidment: This pull request references MGMT-18659 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6696): >When performing a local cluster import - the local cluster name is hardwired to "local-cluster", this is not flexible enough and needs to be changed. >We should be picking up the name from the ManagedCluster that is labelled as "local-cluster" - this should then be used wherever we use the present local-cluster name. >An additional check is performed to ensure that the clusterID referenced in the labels of ManagedCluster matches the clusterID in the labels found in the >clusterVersion of the hub on which the LocalClusterImport is running. > >This should be sufficient at this stage as we do not need to handle scenarios such as a "rename", mainly because ACM will be enforcing some rules > >From ACM-DDR-022: > >``` >To ensure backward compatibility, we will not directly allow renaming the local cluster from MCE or ACM installer at the first stage. The user can choose a customized local cluster name with the following step: >Disable local cluster management in MCE or ACM when install >Manually create a ManagedCluster resource with the label “local-cluster=true” on the hub >``` > >I have also added some entity cleanup that was missed in previous versions, mainly just making sure that unused secrets are deleted. > > > >## List all the issues related to this PR > >- [ ] New Feature >- [ ] Enhancement >- [x] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [x] Automation (CI, tools, etc) >- [x] Cloud >- [x] Operator Managed Deployments >- [x] None > >## How was this code tested? > > > >- [ ] assisted-test-infra environment >- [x] dev-scripts environment > >1. Tested and verified to be correctly working in a dev-scripts environment with a custom operator bundle. >2. AgentServiceConfig was created in a way that renders LocalClusterInstall active >3. ManagedCluster was given the clusterID of the hub >4. Expectation was that local managed cluster would have CR's created for it in the namespace specified by the name of ManagedCluster. This expectation was met. > >- [ ] Reviewer's test appreciated >- [x] Waiting for CI to do a full test run >- [ ] Manual (Elaborate on how it was tested) >- [x] No tests needed > >## Checklist > >- [x] Title and description added to both, commit and PR. >- [x] Relevant issues have been associated (see [CONTRIBUTING] guide) >- [x] This change does not require a documentation update (docstring, `docs`, README, etc) >- [x] 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? > >[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines >[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
paul-maidment commented 2 weeks ago

/test edge-e2e-ai-operator-disconnected-capi

paul-maidment commented 2 weeks ago

/retest

openshift-ci[bot] commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, 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](https://github.com/openshift/assisted-service/blob/master/OWNERS)~~ [carbonin,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-robot commented 2 weeks ago

/retest-required

Remaining retests: 0 against base HEAD ff4c1f32cf271bd137330e3db486b1c013273382 and 2 for PR HEAD 0ab14ac33487acb8bad4a7c0653ef1269bc848fe in total

openshift-ci[bot] commented 2 weeks ago

@paul-maidment: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/edge-e2e-ai-operator-ztp-capi 0ab14ac33487acb8bad4a7c0653ef1269bc848fe link false /test edge-e2e-ai-operator-ztp-capi
ci/prow/edge-e2e-ai-operator-disconnected-capi 0ab14ac33487acb8bad4a7c0653ef1269bc848fe link false /test edge-e2e-ai-operator-disconnected-capi

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-bot commented 1 week ago

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-api-server This PR has been included in build ose-agent-installer-api-server-container-v4.18.0-202408301116.p0.g79bd2f1.assembly.stream.el9. All builds following this will include this PR.