Add Docker-based compatibility test kit for external validation - #431
Add Docker-based compatibility test kit for external validation#431glehmann wants to merge 3 commits into
Conversation
d40d6e1 to
a04eb08
Compare
33fec9a to
54e648b
Compare
071af66 to
d195fe7
Compare
d195fe7 to
eebadce
Compare
54e648b to
a3afb27
Compare
a3afb27 to
9daefa6
Compare
eebadce to
815e691
Compare
9daefa6 to
bc754e8
Compare
2619b93 to
4018ea6
Compare
bc754e8 to
789db8a
Compare
c1989ab to
55de4c6
Compare
| contents: read | ||
| packages: write # Required to push packages to GHCR | ||
| steps: | ||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 |
There was a problem hiding this comment.
nitpicking: pining hash is a good practice, but I tent to add versions in comments then can track what should be updated (or maybe dependabot can figure this out)
| curl \ | ||
| git \ | ||
| iputils \ | ||
| netcat \ |
There was a problem hiding this comment.
| netcat \ | |
| netcat \ | |
| make \ |
There was a problem hiding this comment.
why make? I don't think we need it in the image
There was a problem hiding this comment.
Well there are a makefile helper in repo, i thought we could use it to collect tasks that can be useful for developers (like the setup steps in the docker files)
| if num_hosts != 1: | ||
| raise ValueError( | ||
| f"Pool constraint violated: Master host's pool contains {num_hosts} host(s), " | ||
| f"but only 1 host was provided. The pool must contain only the specified hosts. " |
There was a problem hiding this comment.
| f"but only 1 host was provided. The pool must contain only the specified hosts. " | |
| f"but only 1 host was provided. The pool must contain only the specified host. " |
unclear to me, is it "only one host expected in pool" ?
There was a problem hiding this comment.
The pool must contain only the hosts provided by the user, or the script would fail.
I'll try to reformulate
| if num_hosts != 1: | ||
| raise ValueError( | ||
| f"Pool constraint violated: Master host's pool contains {num_hosts} host(s), " | ||
| f"but only the master host should be present before joining the second host. " |
There was a problem hiding this comment.
| f"but only the master host should be present before joining the second host. " | |
| f"but only the master host should be present before the second host is joining the pool. " |
| sys.stdout.buffer.write(line) | ||
| sys.stdout.buffer.flush() | ||
|
|
||
| # 2. Strip the codes and write the clean text to the file |
There was a problem hiding this comment.
question: Wouldn't it be better to make color output optional in the first place ?
| run_pytest(2, test_args, "test_kit_2.log") | ||
|
|
||
|
|
||
| def set_hvm_fep(enabled: bool) -> None: |
| logging.warning(f"Failed to disable hvm_fep and reboot: {e}") | ||
|
|
||
|
|
||
| def print_summary() -> None: |
There was a problem hiding this comment.
suggestion: would add default param to make colors/utf-8 optional
| sys.exit(1) | ||
| except Exception as e: | ||
| logging.error(f"Error: {e}", exc_info=True) | ||
| sys.exit(1) |
There was a problem hiding this comment.
would use a different exit code
|
|
||
| def main() -> None: | ||
| """Parse arguments and run the compatibility test kit.""" | ||
| default_vm_url = "https://nextcloud.vates.tech/index.php/s/MmEjo8qYo7Ccs2B/download" |
There was a problem hiding this comment.
is it public ? if not this has to be changed to something stable over time. or it has to be generated on the fly (With a script? ie vagrant?)
There was a problem hiding this comment.
yes it is. I agree we should use something stable—ideally on the same GH repository—but we don't have it yet
| parser.add_argument('--master-host', help="IP or hostname of the pool master") | ||
| parser.add_argument("--second-host", help="IP or hostname of the second host (optional, for pool tests)") | ||
| parser.add_argument("--password", help="SSH root password (if not provided, will be prompted)") | ||
| parser.add_argument("--log-dir", default="/app/logs", |
There was a problem hiding this comment.
I would rather use a relative path (in HOME?) to allow out of docker use.
| @@ -0,0 +1,7 @@ | |||
| -----BEGIN OPENSSH PRIVATE KEY----- | |||
There was a problem hiding this comment.
Can't we put this in a secret ? maybe not if images have to be shared, I would generate it on the fly (like vm?) if possible, or add a comment that/why it is intentional and there is no security impact
There was a problem hiding this comment.
This must match the key in the VM image. I would prefer to generate it on the fly, but I don't think we have a way to start a VM and specify an authorized ssh key for now, as we would with cloudinit
There was a problem hiding this comment.
yea i figured this out, I would just add a comment with a reference to the image (and explicitly state it's not secure) file can be also renamed with unsecure suffix.
|
|
||
| ## Security Notes | ||
|
|
||
| The test VM is configured with a known access key and is **strictly reserved for testing purposes only**. |
There was a problem hiding this comment.
| The test VM is configured with a known access key and is **strictly reserved for testing purposes only**. | |
| The test VM is configured with a known access key and is **strictly reserved for testing purposes only**. | |
| Test VM should not be exposed to untrusted networks. |
rzr
left a comment
rzr
left a comment
There was a problem hiding this comment.
lgtm, I work prefer that we have a re-producible vm instead of a fixed asset. Has this been considered ? there is a risk it could be blocked in user's infra.
It's been considered, but not done yet, as it's a significant increase in complexity |
8d6a533 to
519e972
Compare
Introduces a containerized test kit that enables external teams to easily validate xcp-ng compatibility on their own hardware without complex setup requirements. The compat_kit module provides a straightforward, self-contained environment for running compatibility tests, lowering the barrier to entry for third-party validation. Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
When importing a VM from an HTTPS URL, xe vm-import fails because it doesn't properly support HTTPS URLs. This change downloads the image to a temporary file on the host before importing it. HTTP URLs remain unchanged and continue to use the url= parameter directly, avoiding unnecessary local storage of potentially large images. Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
519e972 to
04d74e1
Compare
| @@ -0,0 +1,7 @@ | |||
| -----BEGIN OPENSSH PRIVATE KEY----- | |||
There was a problem hiding this comment.
| -----BEGIN OPENSSH PRIVATE KEY----- | |
| This key is unsecure, it should be only used to connect to prebuild-demo vm images. | |
| -----BEGIN OPENSSH PRIVATE KEY----- |
rzr
left a comment
rzr
left a comment
There was a problem hiding this comment.
Please consider the key comment (we never know) we could create a issue for removal or this key when a better solution is in place.
6 participants
Introduces a containerized test kit that enables external teams to easily validate xcp-ng compatibility on their own hardware without complex setup requirements.
The compat_kit module provides a straightforward, self-contained environment for running compatibility tests, lowering the barrier to entry for third-party validation.
This PR is part of a tree containing 3 PRs:
mastermaster