Migrating PGO CLI kuttl tests to chainsaw #138
Conversation
We can use Kyverno policies to facilitate testing with different image repos.
These are helpful templates to use when testing the PGO CLI
Instructions for how to run the tests
The chainsaw-show Test should remove resources after it completes.
tjmoore4
left a comment
tjmoore4
left a comment
There was a problem hiding this comment.
Looks good overall. A few questions.
| - name: Sleep for 10s | ||
| try: | ||
| - sleep: | ||
| duration: 10s |
There was a problem hiding this comment.
🤔 Is 10s enough for these sleeps? I wonder if the step length or something else is still coming into play here.
There was a problem hiding this comment.
Can we set timeouts on different steps to give longer timeouts for the confirm steps or whatever that is taking so long.
There was a problem hiding this comment.
Yes, I would still like to investigate getting away from Sleeps and use some form of Get/Wait or Get/Assert to give underlying Resources enough time to be created before checking if they have completed.
| apiVersion: chainsaw.kyverno.io/v1alpha1 | ||
| kind: Test | ||
| metadata: | ||
| name: check-backup-command-longer-options |
There was a problem hiding this comment.
This test feels like it's testing that we pass through the options from the command line to the spec. If that's true, I wonder if we could do this as a unit test in the future, and skip the actual backup.
There was a problem hiding this comment.
Yes - I think several of the kuttl tests are candidates for other implementations. But I still wanted to migrate them to chainsaw for completeness and discussion.
| - description: Compare annotations | ||
| assert: | ||
| timeout: 30s | ||
| resource: | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: compare-annotations | ||
| data: | ||
| (key1 != key2): true | ||
| (key2 != 'not-found'): true |
There was a problem hiding this comment.
Didn't know you could do that with Chainsaw, that's interesting.
🤔 I wonder what a step would look like that just compared the two strings in bash...
There was a problem hiding this comment.
yes that would be another way to do it - using ConfigMaps to pass test data around is an interesting paradigm I've seen used with Chainsaw. I wrote this test this way to learn more about it. Open to changing the format though.
There was a problem hiding this comment.
Do you like it better this way?
I'm open to either -- the cm way was a bit of a shock, but happy to use it going forward. But I'd like being a little consistent if we can. I wonder if there's anything we can't do with the CM version or if the bash version looks messier
There was a problem hiding this comment.
I kind of like this way because it makes the assertion more clear (it's not in a bash script...although this would be a simple one).
| apiVersion: chainsaw.kyverno.io/v1alpha1 | ||
| kind: Test | ||
| metadata: | ||
| name: check-backup-command-no-flags |
There was a problem hiding this comment.
What is this test demonstrating?
There was a problem hiding this comment.
This is the conversion of 08--backup-with-just-trigger.yaml which is triggering a backup from the PGO CLI with no options and checking the annotations
| use: | ||
| template: '../templates/confirm-created.yaml' | ||
|
|
||
| - name: Sleep for 30s |
There was a problem hiding this comment.
If we need these sleep steps, I think it would be fine to name them all Sleep or something without the time -- it's easy enough to look at the duration to see how long and it avoids drift when we change those durations.
More interesting to me would be why we chose those durations.
There was a problem hiding this comment.
Most of the Sleep 30s started at Sleep 10. I bumped them up because running all the test simultaneously was causing issues. I left the descriptive "Sleep for 30s" as I was debugging the tests to know how much time I should wait before the next step.
| apiVersion: chainsaw.kyverno.io/v1alpha1 | ||
| kind: Test | ||
| metadata: | ||
| name: check-restore-confirm-yes |
There was a problem hiding this comment.
Would it be faster to create one cluster and run several tests on that -- restore without a name, restore no, restore yes...?
There was a problem hiding this comment.
Yes it is, but each "test" would then be a separate "step". It becomes an issue of clarity and preference. I.e. do we have a single Restore Test that does multiple things. Or multiple Restore Tests, each doing one single thing.
One thing I like about Chainsaw is that each test is namespace-scoped. So it's easy to separate out tests into discrete units.
| (contains($stderr, 'Apply failed')): true | ||
| (contains($stderr, '2 conflicts')): true | ||
|
|
||
| - name: run 'restore' with confirm 'yes' and --force-conflicts |
There was a problem hiding this comment.
Could you break this up into a few steps? Maybe
- set the annotation
- get the annotation (maybe the same step as the one above)
- issue command
- check annotation differs
?
There was a problem hiding this comment.
yes, I will look into this. I think I cribbed more of the original kuttl test on this particular one than the others.
|
|
||
| - name: 'Confirm cluster is created' | ||
| use: | ||
| template: '../templates/confirm-created.yaml' |
There was a problem hiding this comment.
So the idea is that we can use the same template here because the command did nothing?
There was a problem hiding this comment.
yes that's correct
| - sleep: | ||
| duration: 30s | ||
|
|
||
| - name: run 'stop cluster' with confirm 'y' and force-conflicts |
There was a problem hiding this comment.
if the start test runs the stop command, I think we can collapse this into one test, yeah?
There was a problem hiding this comment.
If memory serves, there are some interesting behaviors that happen when you try to stop a cluster that was created from a manifest rather than the PGO CLI. I think the --force-conflicts is really getting tested here to make sure the cluster will stop and then be started.
The original kuttl test (and this conversion) may be worth revisiting.
| name: version | ||
| spec: | ||
| steps: | ||
| - name: step-00 |
There was a problem hiding this comment.
can we get a more descriptive step name?
| verbs: | ||
| - create | ||
| - update | ||
| - delete |
There was a problem hiding this comment.
I was having some issues getting it to work. This Github comment suggested delete.
The way we defined the selector for backups was quite terse. This refactor simplifies the logic.
tjmoore4
left a comment
tjmoore4
left a comment
There was a problem hiding this comment.
Updates look good to me. Not sure if @benjaminjb is still tracking any blockers, but I think this is good to merge.
| - name: postgresVersion | ||
| value: 16 |
There was a problem hiding this comment.
Don't we have a central source for pg version?
There was a problem hiding this comment.
Yes we do; but the bash for this particular test has already hard-coded 16 in it. I didn't want to refactor to interrupt migrating the tests.
There was a problem hiding this comment.
Yeah, this is not a blocker for me; I just hope when we're testing in the far future, when 16 is a problem or dropped, we remember to change it here too (if we're not pulling from that central location)
| metadata: | ||
| name: kuttl-support-monitoring-cluster | ||
| spec: | ||
| postgresVersion: 16 |
There was a problem hiding this comment.
and if we have a central location for the pg version (or even a local version), better to use it
| metadata: | ||
| name: kuttl-support-instrumentation | ||
| spec: | ||
| postgresVersion: 16 |
| requests: | ||
| storage: 1Gi | ||
| replicas: 1 | ||
| postgresVersion: 16 |
benjaminjb
left a comment
benjaminjb
left a comment
There was a problem hiding this comment.
Really no blockers, but re-reading these tests -- many of which I probably had a hand in -- makes me want to revise these tests. Please make a ticket so we don't forget to!
(What I really want is to shift tests left, e.g., run as much as we can as unit tests) and also combine a few of these tests if we can to run them faster. I know I have created a few tickets re: separating out the cluster work from some CLI work, so it won't just be test changes I'm envisioning in the future.)
3 participants
These are the first round of kuttl tests migrated to chainsaw. The structure is a single test for each kubectl-pgo command