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

cosalib/metal.py: migrate cmd-buildextend-metal to Python #1024

Open
wants to merge 1 commit into
base: master
from

Conversation

@darkmuggle
Copy link
Contributor

darkmuggle commented Jan 7, 2020

This builds on #946 and:

  • Migrates Qemu, Metal and DASD targets to Python
  • Introduces an OSTree class
  • Further code deduplication
  • Allows for using cmdlib.sh functions via Pythonic calls.
@darkmuggle darkmuggle force-pushed the darkmuggle:class-builds-metallic branch from 4fc8cf3 to 7edbabd Jan 8, 2020
@darkmuggle darkmuggle requested review from ashcrow, vrutkovs and cgwalters Jan 8, 2020
@darkmuggle darkmuggle force-pushed the darkmuggle:class-builds-metallic branch from 7edbabd to 64dc6bc Jan 8, 2020
REFACTOR: this moves the cmd-buildextend-{metal,qemu,dasd} into
cosalib.metal. The goal of this refactor is to remove code
duplication, use the common cosalib.build Class for handling the
meta-data and using a common CLI structure between all cmd-buildexend-*
commands.

cmd-artifact-disk: use cosalib.metal for previous metal targerts.

cmd-buildextend-{dasd,qemu,metal}:
 - Removed cmd-buildextend-metal
 - Re-target symlinks to cmd-artifact-disk. Since cmd-artifact-disk is
   idempotent, these commands will now gracefully exit when a build
   is already present.

cmdlib.sh: allow for "source cmdlib.sh".

cosalib/cmdlib.py: extend run_verbose to support sourcing cmdlib.sh
   Some cmdlib.sh functions (notably runvm) does not make much sense
   to migrate to Python. The change uses the kwarg 'cmdlib_sh=yes'
   to indicate that the command should be run in a wrapper with
   cmdlib.sh sources.

cosalib/ostree.py: introduce cosalib.ostree for interacting with OSTree
   commits. This is a port of many of the ostree-specific commands from
   cmd-buildextend-metal.

cosalib/metal.py: support the creation of whole-cloth disks images based
   on templates.
   - Supports variants disks using a template-like lookup. Adding new
     targets should be as easy as defining entries in the dict.
@darkmuggle darkmuggle force-pushed the darkmuggle:class-builds-metallic branch from 64dc6bc to c6cb031 Jan 11, 2020
@darkmuggle darkmuggle changed the title WIP: cosalib/metal.py: migrate cmd-buildextend-metal to Python cosalib/metal.py: migrate cmd-buildextend-metal to Python Jan 11, 2020
@darkmuggle darkmuggle marked this pull request as ready for review Jan 11, 2020
@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

darkmuggle commented Jan 11, 2020

Okay, this is ready for review.

DEFAULT_OSTREE_META_SIZE_PERCENT = 5
DEFAULT_OSTREE_PADDING_PERCENT = 35
DEFAULT_OSTREE_INODE_SIZE = 512
DEFAULT_OSTREE_BLOCK_SIZE = 4069

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

I'm not totally sure I agree with having estimate_tree_size as a method on an OSTree. Conceptually, an OSTree commit is an (abstract) filesystem tree, a bit distinct from one specific to a filesystem (instantiated tree).

I'm not objecting to having this here, but it feels like something more where we'd have:

def estimate_ostree_commit_size(repo, commit, filesystem):

And note the third filesystem parameter there which might be a Filesystem struct that has inode size, etc. An OSTree commit doesn't have an inode size, it's specific to a filesystem.

return self.ostree_meta.get("path")

@property
def ostree_sha256(self):

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

Given OSTree is all about sha256...this feels a bit too generically named. Something like ostree_tarball_sha256? And same for the size?

# Find the OStree repo
self._ostree_repo = kwargs.get(
"ostree_repo",
os.path.join(self._workdir, "cache", "repo")

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

Isn't it in tmp/repo? There's a big question here whether we should be using cache/pkgcache-repo instead - for estimating disk size it's going to be faster as we just need to stat() uncompressed objects rather than open()+read().

Get the default terminal based on the architecture
"""
if arch is None:
arch = get_basearch

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

Missing ()?

def rootfs_type(self):
rootfs = self.img_cfg.get("rootfs", DEFAULT_ROOTFS)
rootfs_luks = self.img_cfg.get("luks_rootfs", "None").lower()
if rootfs_luks in ("yes", "1", "true"):

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

Huh, we probably should also accept luks_rootfs: yes (without quotes, i.e. YAML boolean). Was there a reason you made it a string originally?

@property
def rootfs_type(self):
rootfs = self.img_cfg.get("rootfs", DEFAULT_ROOTFS)
rootfs_luks = self.img_cfg.get("luks_rootfs", "None").lower()

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

So the default here would be False not "None".

"ls", ref, "/usr/lib/modules",
],
check=True, capture_output=True
).stdout.decode('utf-8').splitlines()[-1]

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

Not a blocker, this is OK as is - just noting the OSTree API exposed by gobject-introspection supports this and basically everything else we're doing via a subprocess. Sometimes the subprocess is less verbose though.

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 14, 2020

Member

BTW for plenty of examples of using the API see https://github.com/ostreedev/ostree-releng-scripts which is actually already a git submodule here (though I don't think we use it).

f" rootfs to {self.image_name}"))

# Format the command
cmd = ['runvm']

This comment has been minimized.

Copy link
@cgwalters

cgwalters Jan 11, 2020

Member

This is a toplevel script/entrypoint now? I don't see it in master or this PR?

@@ -10,22 +10,28 @@ sys.path.insert(0, cosa_dir)
from cosalib.build import BuildExistsError
from cosalib.cli import BuildCli
import cosalib.qemuvariants as QVariants
import cosalib.metal as Metal

This comment has been minimized.

Copy link
@ashcrow
except subprocess.CalledProcessError:
fatal("Error running command " + args[0])
return process
os.environ["LIBGUESTFS_BACKEND"] = "direct"

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jan 13, 2020

Member

Should we honor the variable if it's already set or always force it to direct? EG:

os.environ['LIBGUESTFS_BACKEND'] = os.environ.get('LIBGUESTFS_BACKEND', 'direct')
if variant:
kwargs.update(VARIANTS.get(variant, {}))
self.arch = kwargs.pop("arch", get_basearch)
self.format = kwargs.pop("format")

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jan 13, 2020

Member

nit: Will raise KeyError if not defined. May be expected (and thus OK)

kwargs.update(VARIANTS.get(variant, {}))
self.arch = kwargs.pop("arch", get_basearch)
self.format = kwargs.pop("format")
self.platform = kwargs.pop("platform")

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jan 13, 2020

Member

nit: Will raise KeyError if not defined. May be expected (and thus OK)

self.size = kwargs.pop("size", "padded")
self.target_args = kwargs.pop("target_args", [])

OSTreeBuild.__init__(self, *args, **kwargs)

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jan 13, 2020

Member

Q: Why explicitly call __init__?

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