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

Migrate the underlying test framework from kuttl to chainsaw #15

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

sergenyalcin
Copy link
Member

@sergenyalcin sergenyalcin commented May 12, 2024

Description of your changes

This PR migrates the underlying test framework from kuttl to chainsaw. For further details about the design one pager about migration, please see: https://github.com/crossplane/uptest/blob/main/design/one-pager-considerations-for-changing-test-framework-of-uptest.md

This pull request updates our project's testing framework to align with the latest best practices for Kubernetes configurations and resource management. By transitioning from kuttl to chainsaw, we embrace a strategic shift toward advanced testing functionalities. This change requires updates across our configuration and templating setups to optimize our new testing framework.

Updated Testing Files, Templates, and Structure

  • Transition from kuttl to chainsaw:

    • Previously, the Apply, Update, Import, and Delete operations were part of a single end-to-end test in kuttl. Now, each operation is treated as a separate test in chainsaw, improving management and isolation. Each test consists of distinct steps, simplifying log tracking. Here’s the new test and step hierarchy:
      1. Apply Test
        • Run Setup Script
        • Apply Resources
        • Assert Status Conditions
      2. Update Test
        • Update Root Resource
        • Assert Updated Resource Status
      3. Import Test
        • Remove State
        • Assert Status Conditions and IDs
      4. Delete Test
        • Delete Resources
        • Assert Resource Deletion
  • Revamp of Test Templates:

    • Updated YAML templates to align with chainsaw requirements, ensuring compatibility with the framework’s features.
    • Removed outdated kuttl specific templates and references, decluttering the test setup.
    • Merged operation and assertion steps into single files in chainsaw, streamlining test execution.
  • Directory and Terminology Updates:

    • Renamed the test directory from kuttl to chainsaw to reflect the new testing framework.

Configuration and Execution Enhancements

  • New Flags Introduced:

    • log-collect-interval to set specific intervals for log collection during tests, enhancing debugging and monitoring capabilities.
    • render-only to render test configurations without execution, useful for verifying setups without running full tests.
  • Modified Timeout Settings:

    • Changed the default timeout flag to use Duration type, aligning with Go’s handling of timed durations for improved reliability in test execution timing.
  • Robust Condition Checks and Cleanup:

    • Enhanced condition checks and resource cleanup processes to be more robust and error-aware, promoting cleaner and safer test executions.

Logging Improvements

  • Updated Logging Practices:
    • Shifted from using kubectl get managed/claim to crossplane beta trace for logging, better integrating with the crossplane ecosystem and providing clearer logs.
    • The log-collect-interval now defaults to 30 seconds, allowing for flexible and effective log management during tests.

This comprehensive update not only refines our testing processes but also sets a new standard for future developments within our project’s testing practices.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested locally and in this PR

@sergenyalcin sergenyalcin force-pushed the chainsaw-migration branch 2 times, most recently from 5f6ccfd to a73bed8 Compare May 12, 2024 11:59
@sergenyalcin sergenyalcin marked this pull request as ready for review May 13, 2024 08:43
Copy link

@humoflife humoflife left a comment

Choose a reason for hiding this comment

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

Are we going to allow uptest users to set respective apply, assert, cleanup, delete, error, exec timeouts for chainsaw?

Copy link

@humoflife humoflife left a comment

Choose a reason for hiding this comment

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

Will chainsaw asset files cover all the resources that are expected to be created by a claim?

Copy link

@humoflife humoflife left a comment

Choose a reason for hiding this comment

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

Will external resources that were created by chainsaw be properly cleaned up also in the event of creation and assertion failures?

Copy link

@humoflife humoflife left a comment

Choose a reason for hiding this comment

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

Will this chainsaw integration work seamlessly with Upbound SaaS?

Copy link

@humoflife humoflife left a comment

Choose a reason for hiding this comment

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

Would it be useful if Uptest supported both KUTTL and Chainsaw for a couple of months to allow users a migration period after a proper announcement?

@sergenyalcin sergenyalcin force-pushed the chainsaw-migration branch 2 times, most recently from 329c0ed to 7d54801 Compare May 22, 2024 14:53
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.

Thanks @sergenyalcin for the chainsaw integration. Left some comments for us to consider & discuss.

cmd/uptest/main.go Outdated Show resolved Hide resolved
cmd/uptest/main.go Show resolved Hide resolved
cmd/uptest/main.go Show resolved Hide resolved
KindGroup string
YAML string
APIVersion string
Kind string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't Kind duplicated as I suspect it also appears in KindGroup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is duplicate. We need to access Kind and KindGroup in template files. I did not move the complexity to template file for doing concatenation inside the template files. Instead of this, I introduced both of them as separate fields.

internal/runner.go Outdated Show resolved Hide resolved
internal/tester.go Outdated Show resolved Hide resolved
internal/tester.go Show resolved Hide resolved
internal/tester.go Outdated Show resolved Hide resolved
internal/tester.go Outdated Show resolved Hide resolved
return err
}
}
return writer.Flush()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
return writer.Flush()
return nil

as the deferred file.Close will already flush any pending writes.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I changed this I noticed that the input file is being empty. So I preserved this line.

@ulucinar
Copy link
Collaborator

ulucinar commented May 28, 2024

Would it be useful if Uptest supported both KUTTL and Chainsaw for a couple of months to allow users a migration period after a proper announcement?

Hi all,
I personally feel it's okay to break things in this PR as the PR is replacing the underlying framework. We had better release the chainsaw support with a major version bump with proper announcement. My main motivation for suggesting that we don't keep supporting kuttle as an alternative engine in parallel to chainsaw is to keep things simple. What do you think regarding breaking things with this PR?

@ulucinar
Copy link
Collaborator

Hi @sergenyalcin,
Could you please include an example uptest invocation with the assumptions about its environment (e.g., crossplane CLI & chainsaw being installed, etc.) in the PR description?

@sergenyalcin
Copy link
Member Author

@ulucinar thank you for your detailed review, this is ready for the second cycle.

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 working on this PR. As we've discussed offline, the timeout models of kuttle and chainsaw do not match each other. But what you've implemented in this PR is a good tradeoff between the issues/goals we've discussed. We may need to revisit how we implement timesouts with chainsaw if users want tighter bounds for failing tests, or if we experience issues with signaling (killing) the chainsaw process. According to some preliminary tests, chainsaw seems to be handling SIGINT by propagating it to the child processes it forks (in exec steps), so we should be fine. Also if we decide to send a signal to the main chainsaw process, that means the test has timed out and the CI pipeline needs to terminate as soon as the resources are cleaned up, so any orphaned processes will not matter as long as the pipeline terminates. My understanding is that chainsaw itself is not responsible for the cleanup as we have a separate cleanup job in the pipeline. So, killing the chainsaw process looks safe. If these assumptions are invalidated, then we can consider implementing a different timeout scheme on top of chainsaw.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
- Use configurable timeout for all timeout types
- Use CommandContext for applying the global timeout
- For preventing memory leak extract the test function

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin merged commit 8a80a6a into crossplane:main Aug 22, 2024
6 checks passed
@sergenyalcin sergenyalcin deleted the chainsaw-migration branch August 22, 2024 22:17
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.

3 participants