Skip to content
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

WIP: cmd-buildextrend-gcp, cosalib.build._Build, and YOU! #550

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ashcrow
Copy link
Collaborator

commented Jun 7, 2019

  • Move md5sum_file out of koji command and into cosalib.build
  • Update cmd-buildextrend-gcp to use and extend cosalib.build._Build
  • Restructure cmd-buildextrend-gcp to scope more variables out of global namespaces
src: Move md5sum_file from koji to build
Signed-off-by: Steve Milner <smilner@redhat.com>

@ashcrow ashcrow added the enhancement label Jun 7, 2019

@ashcrow ashcrow requested review from darkmuggle, dustymabe and miabbott Jun 7, 2019

Show resolved Hide resolved src/cmd-buildextend-gcp Outdated
@dustymabe

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

mostly LGTM other than @miabbott comment, not tested though

Show resolved Hide resolved src/cmd-buildextend-gcp Outdated

@ashcrow ashcrow force-pushed the ashcrow:build-class-gcp branch from 7d18de0 to 4adb7b6 Jun 12, 2019

Show resolved Hide resolved src/cmd-buildextend-gcp Outdated

@ashcrow ashcrow force-pushed the ashcrow:build-class-gcp branch from 4adb7b6 to cbc3afe Jun 12, 2019

Show resolved Hide resolved src/cmd-buildextend-gcp Outdated
Show resolved Hide resolved src/cmd-buildextend-gcp Outdated
shutil.rmtree(tmpdir)
# Update the meta to include our new output
build.meta['gcp'] = {
'image': f"{base_name}-{args.build.replace('.', '-')}",

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jun 12, 2019

Member

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?

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jun 12, 2019

Member

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?)

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jun 12, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jun 13, 2019

Author Collaborator

This comment has been minimized.

Copy link
@darkmuggle

darkmuggle Jun 19, 2019

Contributor

Sharing of registered compute images is not possible unless your images are "global." Per [1],

It is not possible to grant roles to allAuthenticatedUsers or allUsers that 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

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jun 19, 2019

Member

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 ashcrow force-pushed the ashcrow:build-class-gcp branch 2 times, most recently from 0240684 to fb72936 Jun 13, 2019

src/cmd-buildextend-gcp: Extend and use Build
Signed-off-by: Steve Milner <smilner@redhat.com>

@ashcrow ashcrow force-pushed the ashcrow:build-class-gcp branch from fb72936 to ff1c211 Jun 19, 2019

:param args: All non-keyword arguments
:type args: list
:param kwargs: All keyword arguments
:type kwargs: dict

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jul 1, 2019

Member

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)

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jul 1, 2019

Author Collaborator

Absolutely fair. I tend to do this more so because of sphinx documentation, but it's not required at all IMHO :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.