-
Notifications
You must be signed in to change notification settings - Fork 519
docs: update sgx doc and sgx-test tag #3349
Conversation
/cc agowdamsft |
@salsal97: GitHub didn't allow me to request PR reviews from the following users: agowdamsft. Note that only Azure members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @agowdamsft |
@salsal97: GitHub didn't allow me to request PR reviews from the following users: agowdamsft. Note that only Azure members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @brendandburns |
/hold cancel |
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.
Looks prefect for the user flow and best practice recommendation.
@salsal97 the link to sgx documentation in https://github.com/Azure/aks-engine/blob/master/docs/topics/README.md also needs to be updated. |
/approve lgtm 👍 |
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 things would like to see updated.
/assign @jackfrancis |
@@ -7,8 +7,7 @@ spec: | |||
spec: | |||
containers: | |||
- name: sgxtest | |||
image: oeciteam/sgx-test | |||
command: ["/helloworld/host/helloworldhost", "/helloworld/enclave/helloworldenc.signed"] | |||
image: oeciteam/sgx-test:1.0 |
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.
Shall I assume this image has the equivalent "run something to validate basic SGX functionality and exit 0 if success" ?
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.
Yep! It has the same helloworld application, just moved the executing command to the image itself
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'm running tests to validate this now, then will merge, thanks!
Btw, do we know why we are only testing Kubernetes v1.15 and v1.16? Does the sgx-device-plugin
not work w/ v1.17+ for some reason (that would be weird), or do we need to enable tests for those versions of Kubernetes?
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.
Where exactly is this test happening? There's a different version of the yaml file for k8s v1.17 onwards because of changes from beta mode
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 use this config as input to the Jenkinsfile in the root of the project:
https://github.com/Azure/aks-engine/blob/master/test/e2e/test_cluster_configs/sgx.json
Essentially, by building an 18.04-LTS node pool w/ the Standard_DC2s
VM SKU, we expect that the drivers will be installed and that this sgx-test.yaml
will be properly scheduled and executed on a node in the cluster.
I'm building a 1.18 cluster now /w a 18.04 + Standard_DC2s
node pool, I'll report back...
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 sgx-test.yml mounts /dev/sgx so it essentially doesn't depend on the plugin...
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, paulcallen, salsal97, vtikoo 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 |
Reason for Change:
Rewrote parts of the sgx documentation to reflect current state, ordered it according to execution and added device-plugin yaml for deployment as a file instead of in the readme.
Linked the dockerfile for container referenced in the test to build helloworld oe
Updated test of updated docker image sgx-test:1.0
Issue Fixed:
Requirements:
Notes: