-
Notifications
You must be signed in to change notification settings - Fork 622
Move pgbackrest-restore test to Kyverno Chainsaw #4228
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
Conversation
This test has a number of scripts and jobs that pass and share data. Chainsaw's bindings and templates are a nice way to break this test up, and its "catch" operations provide good context when a step fails. Tested with Kyverno Chainsaw 0.2.12 See: https://kyverno.github.io/chainsaw/main
This helped me reproduce a race in "pg_ctl start" during Postgres recovery. Issue: PGO-1945
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
use: { template: '21-lose-data.yaml' } | ||
|
||
- name: 'Point-In-Time Restore' | ||
use: { template: '22-point-in-time-restore.yaml' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth it to regularize the file/template names? I always liked the numbered files because you can just read through a directory (well, could, before we started to use the subdir files
), but if Chainsaw uses this file to sequence the tests, maybe another convention is worth talking about (at a later date!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. I think I started with numbers close to the KUTTL ones to ease review/migration, but where I landed is not that!
I like the filenames going in a similar order as the test, too, but now they can get reused so... 🤷🏻
Maybe something more like "scenario one," "... two," 🤔 🌱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if the templates are being reused (hopefully!), then putting them in order doesn't make sense. Maybe descriptive titles could make it easier to read?
run: | | ||
curl -Lo /usr/local/bin/kubectl-kuttl https://github.com/kudobuilder/kuttl/releases/download/v0.13.0/kubectl-kuttl_0.13.0_linux_x86_64 | ||
chmod +x /usr/local/bin/kubectl-kuttl | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to install KUTTL any longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Make targets do go run ...@latest
by default. The binary download is faster, but not as easy to latest
.
I'm on the fence. Do you have a preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faster by how much? My guess is not that much, so this seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the run times of the Run make check-kuttl && exit
between this PR and another (with the older style), I see 1 worst case I wouldn't maybe want (abt 2 mins difference), but mostly I see 0-15 secs difference (and with these jobs, not sure where the time is really coming from).
|
||
This [chainsaw](https://github.com/kyverno/chainsaw) suite tests that CPK can clone and restore through pgBackRest backups. | ||
|
||
...other material here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What else should we put here to explain this to ourselves when we come back to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything specific to these tests that wouldn't go in general chainsaw docs? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of anything right now and the original test didn't have anything, so I'll leave this for now as a placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of questions but looks good!
|
||
This [chainsaw](https://github.com/kyverno/chainsaw) suite tests that CPK can clone and restore through pgBackRest backups. | ||
|
||
...other material here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything specific to these tests that wouldn't go in general chainsaw docs? 🤔
apiVersion: chainsaw.kyverno.io/v1alpha2 | ||
kind: Configuration | ||
metadata: | ||
name: end-to-end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this file?
I think the info in this file, mainly labels and timeouts, can be defined per test 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and as needed, I think we might make that change. A lot of this will depend on the second test that gets added/what further development we do to add chainsaw tests.
value: 'The name of the new PostgresCluster' | ||
|
||
try: | ||
- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we doing here? Why is there a newline after this -
instead of - description: >
on a single line 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this is a template. Is it related to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why Chris started with this pattern, but it goes through all the files.
curl -Lo /usr/local/bin/kubectl-kuttl https://github.com/kudobuilder/kuttl/releases/download/v0.13.0/kubectl-kuttl_0.13.0_linux_x86_64 | ||
chmod +x /usr/local/bin/kubectl-kuttl | ||
|
||
- run: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step is still named kuttl-k3d... should we make the name more generic?
Checklist:
Type of Changes:
Other Information:
This test failed in this other PR, and the KUTTL logs did not give me enough information to diagnose the cause. Chainsaw did much better and unblocked me over there.