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.

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

Add metalctl command line interface #162

Merged
merged 17 commits into from
Dec 5, 2024
Merged

Conversation

SzymonSAP
Copy link
Contributor

@SzymonSAP SzymonSAP commented Nov 4, 2024

Proposed Changes

This PR adds metaclt move command with it's flags described in a issue #146.

To discuss

  • How to ensure the CRDs and CRs already existing in the target cluster are the same as in the source targets?
    done
  • How to ensure the CRDs and CRs have been successfully created in the target cluster?
    done

Fixes #146

@github-actions github-actions bot added the size/L label Nov 4, 2024
@afritzler
Copy link
Member

Thanks @SzymonSAP. Can you open the PR directly against the main branch?

@SzymonSAP SzymonSAP force-pushed the metalctl branch 3 times, most recently from c0f7ff0 to 441f587 Compare November 6, 2024 10:25
@github-actions github-actions bot added size/XXL api-change documentation Improvements or additions to documentation and removed size/L labels Nov 6, 2024
@SzymonSAP SzymonSAP changed the base branch from metalctl to main November 6, 2024 10:26
@github-actions github-actions bot added size/L and removed size/XXL labels Nov 6, 2024
@SzymonSAP
Copy link
Contributor Author

Thanks @SzymonSAP. Can you open the PR directly against the main branch?

Sure, done it. Please let me know if the changes are on the right track and share any ideas what edge cases should also be covered.
I wonder about scenario in which there are already some CRDs or CRs in target cluster that are identical to source cluster. In such scenario metalctl move could be run successfully but it requires some code changes. Is it a real world use case?

Copy link
Contributor

@Nuckal777 Nuckal777 left a comment

Choose a reason for hiding this comment

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

Thanks, for looking into this. 👍 Besides some small stuff, I think to larger topics are missing.

Firstly, the status of custom resources need to be copied over. This is especially important for Servers, where the status contains the lifecycle phase. Not copying the status here, would re-provision running servers (due to the reset into the discovery phase). One needs to use cl.Status().Update() to set the status after creation of a resource.

Secondly, the metal-operator uses owner references on many resources. They cannot be moved directly as they contain the UID of the owner, which is assigned on creation at random. To move these the owner needs to be looked up in the target cluster by name to patch the UID. E.g.

var endpoint metalv1alphav1.Endpoint
if err := clients.target.Get(ctx, types.NamespacedName{Name: owner.Name}, &endpoint); err != nil {
	return err
}
cr.OwnerReferences[0].UID = endpoint.UID

As a side effect this requires ordering the creation of CRs properly. E.g. Endpoints need to be handled first, then BMC and BMCSecrets and so on. In that regard it may be worth thinking about an "integration" test, which creates an Endpoint against the redfish simulator in source cluster with the metal-operator attached and then try moving.

cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
cmd/metalctl/app/suite_test.go Outdated Show resolved Hide resolved
cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
@SzymonSAP SzymonSAP requested a review from Nuckal777 November 13, 2024 14:59
Copy link
Contributor

@Nuckal777 Nuckal777 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the handling of owner references. The implementation seems fine on a quick glance. 👍 I've added a few comments on stuff I'm unsure about.

cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
cmd/metalctl/app/move.go Outdated Show resolved Hide resolved
@SzymonSAP SzymonSAP force-pushed the metalctl branch 2 times, most recently from 43b8a6a to 90166fe Compare November 14, 2024 09:50
@afritzler
Copy link
Member

Can you please rebase this PR branch and use #179 to add the license headers to all net-new go files?

@SzymonSAP
Copy link
Contributor Author

Can you please rebase this PR branch and use #179 to add the license headers to all net-new go files?

I have pushed requested changes and rebased the PR. Please have a look ;)

@Nuckal777
Copy link
Contributor

Nuckal777 commented Nov 18, 2024

I had a short alignment with @afritzler today. Here are the 2 main outcomes:

  • We don't like to move CRDs with the move command. It is expected, that they are present in the target cluster beforehand, either via make install or helm install. You can remove the related code (Creation of CRs without the repsective CRD fails automatically). Already done. 👍
  • We keep the unstructured.Unstructured approach for moving CRs, because we would like to re-use it for the boot-operator. Could you encapsulate a Move(ctx context.Context, clients Clients, crsToBeMoved apimachinery.GroupVersionKind) (or similar) function into a dedicated package/directory, please? After merging here, we can move that package to ironcore-dev/controllerutils.

Comment on lines 184 to 187
if err = cl.Create(ctx, crsTree.Cr); err != nil {
err = fmt.Errorf("CR %s couldn't be created in the target cluster: %w", crName(crsTree.Cr), err)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the status subresource creation is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added status update after getting CR however in tests cl.Create creates object with status so I'm not sure if it will work on real environment. It would be good to test using clusters.

@afritzler
Copy link
Member

Also it would make sense to add new documentation section Usage/metalctl and describe the use of the new CLI.

var mgrCtx context.Context
mgrCtx, cancel := context.WithCancel(context.Background())
DeferCleanup(cancel)
registryServer := registry.NewServer("localhost:30000")
Copy link
Member

Choose a reason for hiding this comment

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

I would leave out the registry server out of this test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ;)

@afritzler
Copy link
Member

afritzler commented Nov 28, 2024

@SzymonSAP can you add above this line https://github.com/ironcore-dev/metal-operator/blob/main/internal/controller/bmc_controller_test.go#L14

- Usage:
    - metalctl: usage/metalctl.md

And describe in the dosc/usage/metalctl.md how to install and use the metalctl command line tool?

@SzymonSAP
Copy link
Contributor Author

Also it would make sense to add new documentation section Usage/metalctl and describe the use of the new CLI.

I added documentation strongly inspired by https://cluster-api.sigs.k8s.io/clusterctl/commands/move.html

@SzymonSAP SzymonSAP requested a review from afritzler November 29, 2024 15:47
docs/usage/metalctl.md Outdated Show resolved Hide resolved
@afritzler afritzler merged commit 790e54c into ironcore-dev:main Dec 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change documentation Improvements or additions to documentation enhancement New feature or request size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metalctl CLI with move Command to Migrate CRDs and CRs Between Clusters
3 participants