-
Notifications
You must be signed in to change notification settings - Fork 168
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
kola/tests/luks: Use separate machine as Tang server #1407
Conversation
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 (other than the build failure) but I'd like a second review.
This is a non-blocking comment: I'd like do fast-fail on these test. @cgwalters work around using the virtio-serial would be an interesting experiment (that is, lets look at the console for a failure message) so we don't have to wait 10min for a failure to encrypt).
if err := util.Retry(10, 2*time.Second, func() error { | ||
_, err := os.Stat(xinetdPidFile) | ||
// Wait a little bit for the container to start | ||
if err := util.Retry(10, time.Second, func() error { |
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.
Any chance we could do a loop instead to reduce the 10s wait? I can envision a situation where 10s is not enough, and too long.
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 Retry
function should return early if the func
passed in doesn't return an error. Is this different from what you are suggesting? There is also a WaitUntilReady
function that takes a timeout instead of a number of attempts.
@@ -68,7 +50,6 @@ func init() { | |||
Name: `rhcos.luks.tang`, | |||
Flags: []register.Flag{}, | |||
Distros: []string{"rhcos"}, | |||
Platforms: []string{"qemu-unpriv"}, |
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.
These ClusterSize: 0
tests are really "coreos-assmber local tests" - we still require a qemu image and that should be reflected somewhere.
Why did you remove this? Was it breaking something?
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.
@cgwalters The Tang test will also run on other platforms--I tested on AWS. The only difference is how we create the Tang server which is handled in a switch statement. The SSS tests use TPM2 so they are qemu-unpriv only, although I could technically use two Tang servers.
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.
Aside: as of recently anything using TPM2 can also run in GCP: openshift/installer#2921
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.
Overall this is a big improvement!
One of the limitations of unprivileged qemu is there is no machine to machine networking. Host port forwarding allows the host machine to communicate to the guest machine through the local host address. It also allows the guest machine to communicate with the host machine through the gateway address. With two guest machines, we can bridge the two guests through the host machine with port forwarding. For example, Host A can run a webserver on port 8080 and Host B can access Host A's webserver through its gateway (host machine) address http://10.0.2.2:8080. This is useful for creating test services (such as tang) for use by the test machine.
With multiple host port forwarding enabled and machine to machine communication possible, Tang is no longer needed in coreos-assembler. The Tang server is created as a container running inside a machine as a part of the test.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, mike-nguyen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR builds on top of #1405Closed in favor of this PR.Two main things in this PR:
Enable forwarding of multiple host ports. Host port forwarding allows the host machine to communicate to the guest machine through the local host address. It also allows the guest machine to communicate with the host machine through the gateway address. With two guest machines, we can bridge the two guests through the host machine with port forwarding. For example, Host A can run a webserver on port 8080 and Host B can access Host A's webserver through its gateway (host machine) address http://10.0.2.2:8080.
Changing the Tang test to use host port forwarding to run Tang inside a contain on a separate machine. This eliminates the need to have Tang in coreos-assembler itself and fixes races/issues with sharing the Tang server in core-assembler.