PR#2: PowerPC specific changes for createhdds - fedora-qa/createhdds

fedora-qa / createhdds

#2 PowerPC specific changes for createhdds

Merged 6 years ago by adamwill. Opened 6 years ago by michelmno.

fedora-qa/ michelmno/createhdds mno_working3 into master

Increase memory size from 2048 to 4096 for PowerPC

Michel Normand • 6 years ago

1f39c80

Add PowerPC support in createhdds script and json

Michel Normand • 6 years ago

eec1f1a

createhdds.py

file modified

+28 -6

		`@@ -31,6 +31,7 @@`
		`import fedfind.helpers`
		`import guestfs`
		`import libvirt`
		`+ import platform`

		`from six.moves.urllib.request import urlopen`

		`@@ -40,6 +41,7 @@`
		`# as the script itself. images are checked/created in the working`
		`# directory.`
		`SCRIPTDIR = os.path.abspath(os.path.dirname(sys.argv[0]))`
		`+ CPUARCH = platform.processor()`
		`logger = logging.getLogger('createhdds')`

		`def handle_size(size):`
		`@@ -230,20 +232,27 @@`
		`tmpfile = "{0}.tmp".format(self.filename)`
		`arch = self.arch`
		`try:`
		`+ # different url path between primary and secondary arch`
		`+ arch = self.arch`
		`+ if arch in ['ppc64','ppc64le']:`
		`+ fedoradir = 'fedora-secondary'`
		`+ memsize = '4096'`
		`+ else:`
		`+ fedoradir = 'fedora/linux'`
		`+ memsize = '2048'`
		`# this is almost complex enough to need fedfind but not`
		`# quite, I think. also fedfind can't find the 'transient'`
		`# rawhide and branched locations at present`
		`if self.release == 'rawhide':`
		`- loctmp = "https://dl.fedoraproject.org/pub/fedora/linux/development/rawhide/{1}/{2}/os/"`
		`+ loctmp = "https://dl.fedoraproject.org/pub/{0}/development/rawhide/{1}/{2}/os".format(fedoradir, self.variant, self.arch)`
		`elif int(self.release) > fedfind.helpers.get_current_release(branched=False):`
		`# branched`
		`- loctmp = "https://dl.fedoraproject.org/pub/fedora/linux/development/{0}/{1}/{2}/os/"`
		`+ loctmp = "https://dl.fedoraproject.org/pub/{3}/development/{0}/{1}/{2}/os/".format(self.release, self.variant, self.arch, fedoradir)`
		`else:`
		`if arch == 'i686' and int(self.release) > 25:`
		`# from F26 onwards, i686 is in fedora-secondary`
		`- loctmp = "https://download.fedoraproject.org/pub/fedora-secondary/releases/{0}/{1}/{2}/os/"`
		`- else:`
		`- loctmp = "https://download.fedoraproject.org/pub/fedora/linux/releases/{0}/{1}/{2}/os/"`
		`+ fedoradir = 'fedora-secondary'`
		`+ loctmp = "https://download.fedoraproject.org/pub/{3}/releases/{0}/{1}/{2}/os/".format(self.release, self.variant, self.arch, fedoradir)`
		`if arch == 'i686':`
		`arch = 'i386'`
		`xargs = "inst.ks=file:/{0}.ks".format(self.name)`
		`@@ -257,7 +266,7 @@`
		`"--os-variant", shortid, "-x", xargs, "--initrd-inject",`
		`"{0}/{1}.ks".format(SCRIPTDIR, self.name), "--location",`
		`loctmp.format(str(self.release), variant, arch), "--name", "createhdds",`
		`- "--memory", "2048", "--noreboot", "--wait", "-1"]`
		`+ "--memory", memsize , "--noreboot", "--wait", "-1"]`
		`if textinst:`
		`args.extend(("--graphics", "none", "--extra-args", "console=ttyS0"))`
		`else:`
		`@@ -397,6 +406,16 @@`
		`-2 means 'two releases lower than the "next" release', and so on.`
		`The values are the arches to build for that release.`
		`"""`
		`+ powerpc_arches = ['ppc64', 'ppc64le']`
		`+ intel_arches = ['i686', 'x86_64']`
		`+ if CPUARCH in powerpc_arches:`
		`+ supported_arches = powerpc_arches`
		`+ elif CPUARCH in intel_arches:`
		`+ supported_arches = intel_arches`
		`+ else:`
		`+ supported_arches = []`
		`+ logger.info("Need to add a list of supported arches for %s CPU", CPUARCH)`
		`+`
		`imgs = []`
		`# Set this here so if we need to calculate it, we only do it once`
		`if not nextrel:`
		`@@ -436,6 +455,9 @@`
		`# assume a single integer release number`
		`rels = [release]`
		`for arch in arches:`
		`+ if arch not in supported_arches:`
		`+ logger.debug("%s arch ignored on %s CPU machine", arch, CPUARCH)`
		`+ continue`
		`for rel in rels:`
		`imgs.append(`
		`VirtInstallImage(name, rel, arch, variant=variant, size=size, imgver=imgver,`

hdds.json

file modified

+5 -5

		`@@ -127,8 +127,8 @@`
		`{`
		`"name" : "minimal",`
		`"releases" : {`
		`- "-1" : ["x86_64"],`
		`- "-2" : ["x86_64"]`
		`+ "-1" : ["x86_64", "ppc64le", "ppc64"],`
		`+ "-2" : ["x86_64", "ppc64le", "ppc64"]`
		`},`
		`"size" : "6",`
		`"imgver": "2"`
		`@@ -155,8 +155,8 @@`
		`{`
		`"name" : "server",`
		`"releases" : {`
		`- "stable" : ["x86_64"],`
		`- "branched": ["x86_64"]`
		`+ "stable" : ["x86_64", "ppc64le", "ppc64"],`
		`+ "branched": ["x86_64", "ppc64le", "ppc64"]`
		`},`
		`"size" : "6",`
		`"imgver": "3",`
		`@@ -174,7 +174,7 @@`
		`{`
		`"name" : "support",`
		`"releases" : {`
		`- "current" : ["x86_64"]`
		`+ "current" : ["x86_64", "ppc64le", "ppc64"]`
		`},`
		`"size" : "6",`
		`"imgver" : "3"`

michelmno commented 6 years ago

limited now to only one commit !

Add PowerPC support in createhdds script and json

michelmno commented 6 years ago

This PR is part of the three required PR for openQA support for fedora PowerPC.
https://pagure.io/fedora-qa/os-autoinst-distri-fedora/pull-request/1
https://pagure.io/fedora-qa/createhdds/pull-request/2
https://pagure.io/fedora-qa/fedora_openqa/pull-request/1

adamwill commented on line 46 of createhdds.py 6 years ago

Please don't mix the %s and .format styles; just respect what's used already (i.e. use the .format style here, since it's what the line uses already).

adamwill commented on line 54 of createhdds.py 6 years ago

Obviously, we should merge this with your fedoradir change instead of having two different places where we handle the primary vs. secondary path issue.

adamwill commented on line 66 of createhdds.py 6 years ago

are you sure you didn't just need this because of the issue with the Workstation installer images for F26+ being way too big? See line 251 - 255 of current git master.

adamwill commented on line 80 of createhdds.py 6 years ago

I don't think this is how I want to deal with the arch stuff.

I think what I'd prefer is to stick with a single hdds.json which defines all the images we want, and make image creation more flexible. Perhaps we could add a new subcommand which works like all, but only creates virt-install images for arches that can be emulated at a reasonable speed on the host arch (so on an x86_64 system it would only create i686 and x86_64 images, on a ppc64(le) system it would only create ppc64 and ppc64le images; ideally we'd find some way to handily discover this information, if there isn't one, we'd just have to hard code it into a table or something). Or we could just make all do that, really, as offhand I can't think you'd really ever want to create the images for other arches.

If we do that, we can handle actually creating the images in Fedora infra by running createhdds all on the ppc64 worker host as well as doing it on the (x86_64) server, I guess.

michelmno commented 6 years ago

PR updated today to address above @adamw comments.

rebased

6 years ago

adamwill commented 6 years ago

On "Tempo bypass to force fs of shrink images" - I don't like this, it's clearly wrong (it sabotages the mechanism by which we can have individual partitions with different filesystems from the 'overall' default filesystem for the image). If I understand your comment correctly, the problem you're actually trying to solve here is that, somehow, the shrink images that should have NTFS partitions are getting ext4 partitions instead. If so, that's definitely a bug, but we need to figure out why that's happening and fix it, not just implement an incorrect workaround.

It also doesn't sound at all PPC specific, nor should the existence of such a bug break PPC specifically in some way; so can we perhaps drop that commit from this PR, file a bug for the problem, and then look at fixing it separately from this PPC support merge?

adamwill commented 6 years ago

On "Add PowerPC support in createhdds script and json" - this mostly looks OK. But I'm still not clear on why you need to change the memory size, and I don't see an explanation. To be more explicit about my comment on that, I'm talking about this pair of commits:

https://pagure.io/fedora-qa/createhdds/c/f476e00bd6f84d6b9189b9d50fe9d0f2ac66ac7a?branch=master
https://pagure.io/fedora-qa/createhdds/c/6d997b2f807352e119cd3bd68cc2c02fc3f32dde?branch=master

I initially bumped the VM memory size to 4GiB because I found that creating Workstation images for F26 was failing with 2GiB. But then I looked into why it was failing and found the problem was that the Workstation stage2 image for Fedora 26 is wrong: it's the file from the OStree compose, not from the network install compose. See https://bugzilla.redhat.com/show_bug.cgi?id=1477916 for more about this. So I changed my 'fix' from 'increase the memory size' to 'use the Everything tree for F26 Workstation image builds'. What I'm saying is, perhaps you had to change the size for the same reason I did, and now with the Everything workaround, it's not actually needed any more? Can you say specifically why you needed to change the memory size, and/or check if things now work OK on PPC (with the 'Everything' commit) if you take that change out?

Aside from the that, and the 'force fs' thing, this looks fine, thanks again.

rebased

6 years ago

michelmno commented 6 years ago

On "Tempo bypass to force fs of shrink images" [CUT] ...

I am removing that commit from updated PR.

michelmno commented 6 years ago

... But I'm still not clear on why you need to change the memory size, [CUT]

Last February my trials with createhdds hungs on PowerPC while trying to create support images. I did not analysed the why but only search for a bypass.

Now it seems that this is not required anymore, so removing this memory size change in updated PR.

adamwill commented 6 years ago

OK, this mostly looks fine now. Just one thing I forgot to ask about: why do the support server and server image sizes need to be bumped? And do they need to go all the way from 6 to 10 - could we do 8 instead? Thanks!

michelmno commented 6 years ago

OK, this mostly looks fine now. Just one thing I forgot to ask about: why do the support server and server image sizes need to be bumped? And do they need to go all the way from 6 to 10 - could we do 8 instead? Thanks!

This is something I added while investigating server problems adding data capture on the server side (the now dropped patch https://pagure.io/fork/michelmno/fedora-qa/os-autoinst-distri-fedora/c/01e00aaa6460d327ae383d076d107b0a318a50fb?branch=tempo12_PR1_step1 )
I did not tried to tune between 6GB and 10GB :)
So I could remove completely this commit in createhdds as long as we are not addressing the other dropped commit.

1 new commit added

Add PowerPC support in createhdds script and json

6 years ago

1 new commit added

Increase memory size from 2048 to 4096 for PowerPC

6 years ago

michelmno commented 6 years ago

@adamw, I had to put back the increase memory size for PowerPC as per above commit after a trial to rebuild all images (--delete option) I added in commit header the signature of the problem.