WIP: cmd-buildextrend-gcp, cosalib.build._Build, and YOU! #550
Conversation
Signed-off-by: Steve Milner <smilner@redhat.com>
|
mostly LGTM other than @miabbott comment, not tested though |
| shutil.rmtree(tmpdir) | ||
| # Update the meta to include our new output | ||
| build.meta['gcp'] = { | ||
| 'image': f"{base_name}-{args.build.replace('.', '-')}", |
There was a problem hiding this comment.
As I understand it, GCP is quite different from AWS in that a bootable image name is global. It is however namespaced to a project, based on this doc.
So we probably want to include the project name here as well?
There was a problem hiding this comment.
A good reference here is to think about what the cluster/machine API needs. The installer today injects the AMIs into the cluster API machineset objects.
I was looking at a GCP example object and it seems to just use os which is a public image. It's not even clear to me that the GCP provider has support for starting an image...
Hum, maybe what the installer will need to do is copy the image from the project we own into the user's project.
(Have you tested booting an image that was uploaded using this code?)
There was a problem hiding this comment.
No, this is all still WIP 😄. The majority of the code is the same (use of stat, name of the image, etc..). This PR attempts to push that code into using a Build class.
I'll hopefully context switch back to this later this week.
There was a problem hiding this comment.
Sharing of registered compute images is not possible unless your images are "global." Per [1],
It is not possible to grant roles to
allAuthenticatedUsersorallUsersthat allow access to images or snapshots.
Sharing between organizations is allowed, but that seems to be an unsustainable path. What can be done, short of working with GCP, is to copy/upload the tarball then have the installer registered the object as a compute image.
This leads me to conclude that including the GCP section should have a bucket path and we should apply an ACL to allow copying.
[1] https://cloud.google.com/compute/docs/images/sharing-images-across-projects
There was a problem hiding this comment.
It sounds like they're basically discouraging easy use of non-official images, which I totally understand. Having the installer copy the image into the target project seems fine to me. It's effectively the same as what we're doing with the AMIs in EC2 for a different reason (encryption).
From my PoV we don't need to have this bit finalized before merge but it'd be nice to have for sure (test case is uploading using one account and documenting the flow/steps to boot the image in another account with the data from the meta.json).
ashcrow
force-pushed
the
build-class-gcp
branch
2 times, most recently
from
0240684
to
fb72936
Compare
Signed-off-by: Steve Milner <smilner@redhat.com>
| :param args: All non-keyword arguments | ||
| :type args: list | ||
| :param kwargs: All keyword arguments | ||
| :type kwargs: dict |
There was a problem hiding this comment.
This is totally my personal feeling but...I feel like part of the value of Python is that it's not too verbose, one can write things quickly. These "types in doc comments" make it more of a mixed bag. If I was writing a major Python library I'd probably do it, but in this case it should be totally obvious to anyone who knows Python at all that **kwargs is a dict of args.
Not saying we should drop this at all, but hope no one objects if I don't add similar doc comments myself 😉 (If we are going to go for verbosity I'd vote for a static language instead)
There was a problem hiding this comment.
Absolutely fair. I tend to do this more so because of sphinx documentation, but it's not required at all IMHO :-)
|
@ashcrow given the refactor and the commit history, I think this is stale. I'm going to close it...but if you disagree, please feel free to reopen. |
|
@ashcrow: PR needs rebase. 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. |
2 similar comments
|
@ashcrow: PR needs rebase. 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. |
|
@ashcrow: PR needs rebase. 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. |
|
@ashcrow: PR needs rebase. 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. |
md5sum_fileout ofkojicommand and intocosalib.buildcmd-buildextrend-gcpto use and extendcosalib.build._Buildcmd-buildextrend-gcpto scope more variables out of global namespaces