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 description field to the migration plan and add support for execution from specified step #235

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sergenyalcin
Copy link
Member

@sergenyalcin sergenyalcin commented Jul 20, 2023

Description of your changes

This PR:

  • Adds description field to the migration plan for a better user interface.
  • Adds support for execution from specified step

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Generated and reviewed migration plan

- name: backup-managed-resources
  description: Backing up Managed Resources
  type: Exec
  exec:
    command: sh
    args:
        - -c
        - kubectl get managed -o yaml > backup/managed-resources.yaml
  manualExecution:
    - sh -c "kubectl get managed -o yaml > backup/managed-resources.yaml"
- name: backup-composite-resources
  description: Backing up Composite Resources
  type: Exec
  exec:
    command: sh
    args:
        - -c
        - kubectl get composite -o yaml > backup/composite-resources.yaml
  manualExecution:
    - sh -c "kubectl get composite -o yaml > backup/composite-resources.yaml"
- name: backup-claim-resources
  description: Backing up Claims
  type: Exec
  exec:
    command: sh
    args:
        - -c
        - kubectl get claim --all-namespaces -o yaml > backup/claim-resources.yaml
  manualExecution:
    - sh -c "kubectl get claim --all-namespaces -o yaml > backup/claim-resources.yaml"

@sergenyalcin sergenyalcin self-assigned this Jul 20, 2023
@sergenyalcin sergenyalcin changed the title Add description field to the migration plan Add description field to the migration plan and support for execution from specified step Jul 21, 2023
@sergenyalcin sergenyalcin changed the title Add description field to the migration plan and support for execution from specified step Add description field to the migration plan and add support for execution from specified step Jul 21, 2023
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @sergenyalcin for introducing the step descriptions that will help folks give more context into what the migrator is doing and for what purpose.

We may also consider having a verbose mode for the plan executor, in which we will dump the description and the outputs from the executed steps. If the executor is not verbose, then we may consider just dumping the description strings.

And as also mentioned in the review comments, we can also consider giving some more context in the description texts, such the the provider names being installed/waited, files being applied, image URLs being pushed/replaced, etc. I believe this also depends whether we will have a mode of execution where we just dump the descriptions (an option for a cleaner output in my opinion). Let's discuss.

@@ -46,6 +50,7 @@ spec:
manualExecution:
- "kubectl apply -f new-ssop/provider-family-aws.providers.pkg.crossplane.io_v1.yaml"
type: Apply
description: "Installing the new family provider"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "Installing the new family provider"
description: "Installing the new family config provider"

nit: We can also consider including the config provider's name in the description. This would require us to parse the references file in the manifest (and what about multiple manifests in a single file?) so we may do this if there's request for it. What do you think?

pkg/migration/plan_executor.go Show resolved Hide resolved
@@ -103,14 +111,15 @@ func NewPlanExecutor(plan Plan, executors []Executor, opts ...PlanExecutorOption

func (pe *PlanExecutor) Execute() error { //nolint:gocyclo // easier to follow this way
ctx := make(map[string]any)
for i := 0; i < len(pe.plan.Spec.Steps); i++ {
for i := pe.StartIndex; i < len(pe.plan.Spec.Steps); i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have access to a logger in migration.PlanExecutor? If so, we may consider adding at least a debug statement that will convey the step index we are starting execution at.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have access to a logger. One option can be adding a log to callback implementation.

var r CallbackResult
if pe.callback != nil {
r = pe.callback.StepToExecute(pe.plan.Spec.Steps[i], i)
switch r.Action {
case ActionCancel:
return nil
case ActionSkip:
pe.LastSuccessfulStep = i
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we use LastSucccessfulStep? If a step is skipped, is it still a successful one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left that up to the reviews to talk about. In family-migrator, we return the skip action for the steps that the user has run manually. We should consider successful steps that are run manually and that the user validates it.

However, the skip action is a much more general action. Therefore, we can specify another type of action for such situations. (For manually performed steps)

Copy link
Member Author

@sergenyalcin sergenyalcin Jul 24, 2023

Choose a reason for hiding this comment

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

Or the another option can be that changing the name of the LastSucccessfulStep to LastPerformedStep.

This field is used for caching mechanism for plan. Please see this PR

I think, other approach can be moving this logic (storing the last performed step) to the migrator side.

executors []Executor
plan Plan
callback ExecutorCallback
LastSuccessfulStep int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do we read LastSuccessfulStep?

Copy link
Member Author

Choose a reason for hiding this comment

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

We read this value in the family-migrator. Please see this PR.

@sergenyalcin
Copy link
Member Author

@ulucinar I made the changes you suggested about the wording in the descriptions. However, I would like to say a few things about the fact that it contains more information.

It may be good to provide as much information as possible in the Description, but I think it is sufficient to set the summary information as a title in its current form. An example output is in this PR. (Of course, this is an older version, the wording has been improved.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants