-
Notifications
You must be signed in to change notification settings - Fork 65
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
Go back to latest payloads after release #377
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.
I was thinking about this, but it seemed weird to revert the fairly old change for enclave-cc (we forgot to update those on the last release). I don't really have a preference either way. You might want to comment on confidential-containers/confidential-containers#210 where we are fixing up the release checklist for next time. I think we will probably start using a script to make these changes in the future as well. |
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 also liked the git revert
idea. Maybe next release we can update the files so that it's git revert-able.
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.
@fitzthum, the PR itself is not wrong, but it'll need a follow-up commit (as part of the very same PR).
Once we revert to use the latest from kata-containers, Operator jobs will break, as on the Kata Containers side a change was made but not reflected here, and it's my fault, see:
- https://github.com/kata-containers/kata-containers/blob/9a6d8d83308870ca662b1b811b1bb7c42b96cfba/tools/packaging/kata-deploy/kata-deploy/base/kata-deploy.yaml#L61-L62
- https://github.com/kata-containers/kata-containers/blob/9a6d8d83308870ca662b1b811b1bb7c42b96cfba/tools/packaging/kata-deploy/kata-deploy/base/kata-deploy.yaml#L77-L79
That mount has to be propagated here:
operator/config/samples/ccruntime/base/ccruntime.yaml
Lines 18 to 43 in 0e8b344
installerVolumeMounts: - mountPath: /etc/crio/ name: crio-conf - mountPath: /etc/containerd/ name: containerd-conf - mountPath: /opt/kata/ name: kata-artifacts - mountPath: /usr/local/bin/ name: local-bin installerVolumes: - hostPath: path: /etc/crio/ type: "" name: crio-conf - hostPath: path: /etc/containerd/ type: "" name: containerd-conf - hostPath: path: /opt/kata/ type: DirectoryOrCreate name: kata-artifacts - hostPath: path: /usr/local/bin/ type: "" name: local-bin
Thanks, and let me know if you face any issue updating the file.
Since the release is done, move back to the latest payloads for enclave-cc and the reqs bundle. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
Since the release is done, point the Kata payloads back to the latest upstream bundles. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
kata-deploy now expects to access the host through /host. Update the ccruntime yaml to add this mount point. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
Added the host mount. I don't want this PR to become a catch-all for changes we pick up from Kata. Ideally it should be merged immediately after the release. |
I agree, but if the tests were not passing, we fix the minimum enough to get the CI green (which is the case at this point). |
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, thanks @fitzthum!
The release is done, so let's go back to the upstream bundles for enclave-cc, kata, and reqs-deploy