[gentoo-catalyst] Mounts and Mountmap completions, fixes. Try 2:

[gentoo-catalyst] Mounts and Mountmap completions, fixes. Try 2:

[Brian Dolbec]

Sorry, the previous email missed one of the patches.
These 2 patches complete the mounts and mountmap rewrite.
It includes a target_mounts dictionary which I put at the top of the generic_stage_target.py for now.
It will be moved to defaults in later commits once defaults.py is created.
They should be applied next, and soon to complete what was missed in the previous batch of commits.

[gentoo-catalyst] [PATCH 1/2] modules/generic_stage_target.py: USE portdir, distdir,... instead of paths for keys

[Brian Dolbec]

If snapcache is enabled, self.mounts and self.mountmap had not been modified
like they were if it was not enabled.
---
 modules/generic_stage_target.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index 3e60f72..d7cffa9 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -174,10 +174,10 @@ class generic_stage_target(generic_target):
 
 		""" Setup our mount points """
 		if "SNAPCACHE" in self.settings:
-			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
-			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
-				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
-				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
+			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
+			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
+				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
+				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 1/2] modules/generic_stage_target.py: USE portdir, distdir,... instead of paths for keys

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On Tue, Dec 17, 2013 at 05:07:26PM -0800, Brian Dolbec wrote:
>  		if "SNAPCACHE" in self.settings:
> -			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
> -			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
> -				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
> -				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
> +			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> +			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> +				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> +				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
>  		else:
>  			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
>  			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",

Where does /usr/portage come from if SNAPCACHE is not enabled?  Is it
just bind-mounted from the host's /usr/portage?

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [gentoo-catalyst] [PATCH 1/2] modules/generic_stage_target.py: USE portdir, distdir,... instead of paths for keys

[Brian Dolbec]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Tue, 2013-12-17 at 19:10 -0800, W. Trevor King wrote:
> On Tue, Dec 17, 2013 at 05:07:26PM -0800, Brian Dolbec wrote:
> >  		if "SNAPCACHE" in self.settings:
> > -			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
> > -			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
> > -				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
> > -				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
> > +			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> > +			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> > +				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> > +				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> >  		else:
> >  			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
> >  			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> 
> Where does /usr/portage come from if SNAPCACHE is not enabled?  Is it
> just bind-mounted from the host's /usr/portage?
> 
> Cheers,
> Trevor
> 

It comes from the snapshot tarball created during a snapshot run.
SnapCache just keeps an unpacked copy ready to reuse.  That way it can
skip the unpack stage.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] [PATCH 1/2] modules/generic_stage_target.py: USE portdir, distdir,... instead of paths for keys

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Tue, Dec 17, 2013 at 07:15:07PM -0800, Brian Dolbec wrote:
> On Tue, 2013-12-17 at 19:10 -0800, W. Trevor King wrote:
> > On Tue, Dec 17, 2013 at 05:07:26PM -0800, Brian Dolbec wrote:
> > >  		if "SNAPCACHE" in self.settings:
> > > -			self.mounts=["/proc","/dev","/usr/portage","/usr/portage/distfiles","/var/tmp/portage"]
> > > -			self.mountmap={"/proc":"/proc","/dev":"/dev","devpts":"/dev/pts",
> > > -				"/usr/portage":self.settings["snapshot_cache_path"]+"/portage",\
> > > -				"/usr/portage/distfiles":self.settings["distdir"],"/var/tmp/portage":"tmpfs"}
> > > +			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> > > +			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> > > +				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> > > +				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> > >  		else:
> > >  			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
> > >  			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> > 
> > Where does /usr/portage come from if SNAPCACHE is not enabled?  Is it
> > just bind-mounted from the host's /usr/portage?
> 
> It comes from the snapshot tarball created during a snapshot run.
> SnapCache just keeps an unpacked copy ready to reuse.  That way it can
> skip the unpack stage.

In that case, this patch is acked-by me.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

Cleanup all self.mounts, self.mountmap usage.
Replace multiple path additions with one instance at the beginning of the function, reuse the result multiple times.
Add some extra debug prints
---
 modules/generic_stage_target.py | 97 +++++++++++++++++++++++++----------------
 modules/stage1_target.py        |  5 ++-
 2 files changed, 62 insertions(+), 40 deletions(-)

diff --git a/modules/generic_stage_target.py b/modules/generic_stage_target.py
index d7cffa9..d9cc499 100644
--- a/modules/generic_stage_target.py
+++ b/modules/generic_stage_target.py
@@ -4,6 +4,21 @@ from generic_target import *
 from stat import *
 import catalyst_lock
 
+target_mounts = {
+	"proc": "/proc",
+	"dev": "/dev",
+	"devpts": "/dev/pts",
+	"portdir": "/usr/portage",
+	"distdir": "/usr/portage/distfiles",
+	"packagedir": "/usr/portage/packages",
+	"port_tmpdir": "/var/tmp/portage",
+	"kerncache": "/tmp/kerncache",
+	"ccache": "/var/tmp/ccache",
+	"icecream": "/var/cache/icecream",
+	"port_logdir": "/var/log/portage",
+	}
+
+
 class generic_stage_target(generic_target):
 	"""
 	This class does all of the chroot setup, copying of files, etc. It is
@@ -173,11 +188,12 @@ class generic_stage_target(generic_target):
 			file_locate(self.settings,["portage_confdir"],expand=0)
 
 		""" Setup our mount points """
+		self.target_mounts = target_mounts.copy()
 		if "SNAPCACHE" in self.settings:
 			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
 			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
-				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
-				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
+				"portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]),
+ 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
 		else:
 			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
 			self.mountmap = {"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
@@ -218,17 +234,17 @@ class generic_stage_target(generic_target):
 			self.mounts.append("ccache")
 			self.mountmap["ccache"] = ccdir
 			""" for the chroot: """
-			self.env["CCACHE_DIR"]="/var/tmp/ccache"
+			self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
 
 		if "ICECREAM" in self.settings:
-			self.mounts.append("/var/cache/icecream")
-			self.mountmap["/var/cache/icecream"]="/var/cache/icecream"
+			self.mounts.append("icecream")
+			self.mountmap["icecream"] = self.target_mounts["icecream"]
 			self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
 
 		if "port_logdir" in self.settings:
-			self.mounts.append("/var/log/portage")
-			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
-			self.env["PORT_LOGDIR"]="/var/log/portage"
+			self.mounts.append("port_logdir")
+			self.mountmap["port_logdir"]=self.settings["port_logdir"]
+			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
 			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
 
 	def override_cbuild(self):
@@ -602,33 +618,34 @@ class generic_stage_target(generic_target):
 				"kill-chroot-pids script failed.",env=self.env)
 
 	def mount_safety_check(self):
-		mypath=self.settings["chroot_path"]
-
 		"""
 		Check and verify that none of our paths in mypath are mounted. We don't
 		want to clean up with things still mounted, and this allows us to check.
 		Returns 1 on ok, 0 on "something is still mounted" case.
 		"""
 
-		if not os.path.exists(mypath):
+		if not os.path.exists(self.settings["chroot_path"]):
 			return
 
+		print "self.mounts =", self.mounts
 		for x in self.mounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			print "mount_safety_check() x =", x, target
+			if not os.path.exists(target):
 				continue
 
-			if ismount(mypath + self.mountmap[x]):
+			if ismount(target):
 				""" Something is still mounted "" """
 				try:
-					print self.mountmap[x] + " is still mounted; performing auto-bind-umount...",
+					print target + " is still mounted; performing auto-bind-umount...",
 					""" Try to umount stuff ourselves """
 					self.unbind()
-					if ismount(mypath + self.mountmap[x]):
-						raise CatalystError, "Auto-unbind failed for " + self.mountmap[x]
+					if ismount(target):
+						raise CatalystError, "Auto-unbind failed for " + target
 					else:
 						print "Auto-unbind successful..."
 				except CatalystError:
-					raise CatalystError, "Unable to auto-unbind " + self.mountmap[x]
+					raise CatalystError, "Unable to auto-unbind " + target
 
 	def unpack(self):
 		unpack=True
@@ -896,32 +913,37 @@ class generic_stage_target(generic_target):
 
 	def bind(self):
 		for x in self.mounts:
-			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
-				os.makedirs(self.settings["chroot_path"]+x,0755)
+			#print "bind(); x =", x
+			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
+			if not os.path.exists(target):
+				os.makedirs(target, 0755)
 
 			if not os.path.exists(self.mountmap[x]):
 				if not self.mountmap[x] == "tmpfs":
-					os.makedirs(self.mountmap[x],0755)
+					os.makedirs(self.mountmap[x], 0755)
 
 			src=self.mountmap[x]
-			if "SNAPCACHE" in self.settings and x == "/usr/portage":
+			#print "bind(); src =", src
+			if "SNAPCACHE" in self.settings and x == self.settings["portdir"]:
 				self.snapshot_lock_object.read_lock()
 			if os.uname()[0] == "FreeBSD":
 				if src == "/dev":
-					retval = os.system("mount -t devfs none " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount -t devfs none " + target
+					retval=os.system(cmd)
 				else:
-					retval = os.system("mount_nullfs " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount_nullfs " + src + " " + target
+					retval=os.system(cmd)
 			else:
 				if src == "tmpfs":
 					if "var_tmpfs_portage" in self.settings:
-						retval=os.system("mount -t tmpfs -o size="+\
-							self.settings["var_tmpfs_portage"]+"G "+src+" "+\
-							self.settings["chroot_path"]+x)
+						cmd = "mount -t tmpfs -o size=" + \
+							self.settings["var_tmpfs_portage"] + "G " + \
+							src + " " + target
+						retval=os.system(cmd)
 				else:
-					retval = os.system("mount --bind " + src + " " +
-						self.settings["chroot_path"] + src)
+					cmd = "mount --bind " + src + " " + target
+					#print "bind(); cmd =", cmd
+					retval=os.system(cmd)
 			if retval!=0:
 				self.unbind()
 				raise CatalystError,"Couldn't bind mount " + src
@@ -933,26 +955,25 @@ class generic_stage_target(generic_target):
 		myrevmounts.reverse()
 		""" Unmount in reverse order for nested bind-mounts """
 		for x in myrevmounts:
-			if not os.path.exists(mypath + self.mountmap[x]):
+			target = normpath(mypath + self.target_mounts[x])
+			if not os.path.exists(target):
 				continue
 
-			if not ismount(mypath + self.mountmap[x]):
+			if not ismount(target):
 				continue
 
-			retval=os.system("umount "+\
-				os.path.join(mypath, self.mountmap[x].lstrip(os.path.sep)))
+			retval=os.system("umount " + target)
 
 			if retval!=0:
-				warn("First attempt to unmount: " + mypath +
-					self.mountmap[x] +" failed.")
+				warn("First attempt to unmount: " + target + " failed.")
 				warn("Killing any pids still running in the chroot")
 
 				self.kill_chroot_pids()
 
-				retval2 = os.system("umount " + mypath + self.mountmap[x])
+				retval2 = os.system("umount " + target)
 				if retval2!=0:
 					ouch=1
-					warn("Couldn't umount bind mount: " + mypath + self.mountmap[x])
+					warn("Couldn't umount bind mount: " + target)
 
 			if "SNAPCACHE" in self.settings and x == "/usr/portage":
 				try:
diff --git a/modules/stage1_target.py b/modules/stage1_target.py
index aa43926..1e0d874 100644
--- a/modules/stage1_target.py
+++ b/modules/stage1_target.py
@@ -88,8 +88,9 @@ class stage1_target(generic_stage_target):
 			os.makedirs(self.settings["stage_path"]+"/proc")
 
 		# alter the mount mappings to bind mount proc onto it
-		self.mounts.append("/tmp/stage1root/proc")
-		self.mountmap["/tmp/stage1root/proc"]="/proc"
+		self.mounts.append("stage1root/proc")
+		self.target_mounts["stage1root/proc"] = "/tmp/stage1root/proc"
+		self.mountmap["stage1root/proc"]="/proc"
 
 def register(foo):
 	foo.update({"stage1":stage1_target})
-- 
1.8.3.2

Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> +target_mounts = {
> +	"proc": "/proc",
> + …
> +	"port_logdir": "/var/log/portage",
> +	}
> …
> @@ -173,11 +188,12 @@ class generic_stage_target(generic_target):
>  			file_locate(self.settings,["portage_confdir"],expand=0)
>  
>  		""" Setup our mount points """
> +		self.target_mounts = target_mounts.copy()
>  		if "SNAPCACHE" in self.settings:
>  			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
>  			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> -				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> +				"portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]),
> + 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}

Can we do this for the mountmap defaults too, and just override the
mountmap special cases in the SNAPCACHE branch?  I think this would be
a good commit for that (or we can do it in a commit after this).

I don't think the 'portage' → self.settings["repo_name"] replacement
should go in this commit.  It also looks like there is some leading
non-tab whitespace at the beginning of your new distdir line, which
was otherwise unchanged.

> -			self.mounts.append("/var/log/portage")
> -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> -			self.env["PORT_LOGDIR"]="/var/log/portage"
> +			self.mounts.append("port_logdir")
> +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]

I don't know where we stand on 'x=y' vs 'x = y', but I'd prefer the
latter here.  I also think that the PORT_LOGDIR environment variable
should be:

  self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]

Nothing in the chroot should care where the source comes from ;).  I'd
considering renaming mountmap → source_mounts for clarity, using
existing settings to override source_mounts at initialization, and
using source_mounts thereafter.

>  	def bind(self):
>  		for x in self.mounts:
> -			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
> -				os.makedirs(self.settings["chroot_path"]+x,0755)
> +			#print "bind(); x =", x
> +			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
> +			if not os.path.exists(target):
> +				os.makedirs(target, 0755)
>  
>  			if not os.path.exists(self.mountmap[x]):
>  				if not self.mountmap[x] == "tmpfs":
> -					os.makedirs(self.mountmap[x],0755)
> +					os.makedirs(self.mountmap[x], 0755)
>  
>  			src=self.mountmap[x]

Might as well shift this src declaration up and use it instead of
self.mountmap[x] earlier.  Not exactly required for this commit, but
you are defining target, so it's not too much of a stretch ;).
 
> -			if "SNAPCACHE" in self.settings and x == "/usr/portage":
> +			#print "bind(); src =", src
> +			if "SNAPCACHE" in self.settings and x == self.settings["portdir"]:

Oops.  x is the mountmap key, so I think this should be 'portdir'.

> +					#print "bind(); cmd =", cmd

If it's not useful enough to print, I don't think we should commit it
;).  I'll be happy once we're using subprocess :p.

A few typos, but I think this makes mount handling much more sane :).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

[-- Attachment #1: Type: text/plain, Size: 6659 bytes --]

On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote:
> On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> > +target_mounts = {
> > +	"proc": "/proc",
> > + …
> > +	"port_logdir": "/var/log/portage",
> > +	}
> > …
> > @@ -173,11 +188,12 @@ class generic_stage_target(generic_target):
> >  			file_locate(self.settings,["portage_confdir"],expand=0)
> >  
> >  		""" Setup our mount points """
> > +		self.target_mounts = target_mounts.copy()
> >  		if "SNAPCACHE" in self.settings:
> >  			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> >  			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> > -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> > -				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> > +				"portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]),
> > + 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> 
> Can we do this for the mountmap defaults too, and just override the
> mountmap special cases in the SNAPCACHE branch?  I think this would be
> a good commit for that (or we can do it in a commit after this).
> 
> I don't think the 'portage' → self.settings["repo_name"] replacement
> should go in this commit.  

repo_name variable is already defined. This one got missed in the
change.  You want it in the next commit by itself?  Can do.


> It also looks like there is some leading
> non-tab whitespace at the beginning of your new distdir line, which
> was otherwise unchanged.


Ah, yes, an errant space got in at the beginning of the line.  I have
rebased it away.  I will repost the fixed patch when any other dust
settles.


> 
> > -			self.mounts.append("/var/log/portage")
> > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > +			self.mounts.append("port_logdir")
> > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> 
> I don't know where we stand on 'x=y' vs 'x = y', but I'd prefer the
> latter here. 

I do too, but I thought it was agreed to fix all that separately
later :/

>  I also think that the PORT_LOGDIR environment variable
> should be:
> 
>   self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]
> 

meh, but I also prefer some uniformity.  I prefer all-caps for
constants.  In this case, the end result later in a new defaults.py
file...  since it is internal, why mix case in keys.  They are also
pushed into the bash environment for the chroot scripts to use.  So,
again, uniformity can be your friend when making changes.  Especially
with a large complex app like catalyst.

code snipit from defaults.py to come later.

# please try to avoid using "-", "/", "." in key names
# due to them not being compatible in the bash environment
confdefaults={
	"storedir": "/var/tmp/catalyst",
	"sharedir": "/usr/lib/catalyst",
	"shdir": "%(sharedir)s/targets",
	"PythonDir": "./catalyst",
	"archdir": "%(PythonDir)s/arch",
	"distdir": "/usr/portage/distfiles",
	"repo_name": "portage",
	"portdir": "/usr/portage",
	"packagedir": "/usr/portage/packages",
	"port_tmpdir": "/var/tmp/portage",
	"local_overlay": "/usr/local/portage",
	"port_conf": "/etc/portage",
	"make_conf": "%(port_conf)s/make.conf",
	"options": set(),
	"snapshot_name": "portage-",
	"snapshot_cache": "/var/tmp/catalyst/snapshot_cache",
	"hash_function": "crc32",
	}


target_mounts = {
	"proc": "/proc",
	"dev": "/dev",
	"pts": "/dev/pts",
	"portdir": "/usr/portage",
	"distdir": "/usr/portage/distfiles",
	"packagedir": "/usr/portage/packages",
	"port_tmpdir": "/var/tmp/portage",
	"kerncache": "/tmp/kerncache",
	"ccache": "/var/tmp/ccache",
	"icecream": "/var/cache/icecream",
	"port_logdir": "/var/log/portage",
	}



> Nothing in the chroot should care where the source comes from ;).  I'd
> considering renaming mountmap → source_mounts for clarity, using
> existing settings to override source_mounts at initialization, and
> using source_mounts thereafter.

mounts and mountmap were existing variable names.  With some of the
flack I've gotten over my choice of variable name already, now you want
me to go through even more???  Do it in a separate commit.  (after the
rewrite patches are done for.)

> 
> >  	def bind(self):
> >  		for x in self.mounts:
> > -			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
> > -				os.makedirs(self.settings["chroot_path"]+x,0755)
> > +			#print "bind(); x =", x
> > +			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
> > +			if not os.path.exists(target):
> > +				os.makedirs(target, 0755)
> >  
> >  			if not os.path.exists(self.mountmap[x]):
> >  				if not self.mountmap[x] == "tmpfs":
> > -					os.makedirs(self.mountmap[x],0755)
> > +					os.makedirs(self.mountmap[x], 0755)
> >  
> >  			src=self.mountmap[x]
> 
> Might as well shift this src declaration up and use it instead of
> self.mountmap[x] earlier.  Not exactly required for this commit, but
> you are defining target, so it's not too much of a stretch ;).

Yeah, I did target when trying to get all the path issues straightened
out.  I never really looked at src to be honest.  That code was not
causing me grief.  So, yeah, do it later in a separate commit.

>  
> > -			if "SNAPCACHE" in self.settings and x == "/usr/portage":
> > +			#print "bind(); src =", src
> > +			if "SNAPCACHE" in self.settings and x == self.settings["portdir"]:
> 
> Oops.  x is the mountmap key, so I think this should be 'portdir'.

Good catch, I missed this one when converting from paths as keys.

> 
> > +					#print "bind(); cmd =", cmd
> 
> If it's not useful enough to print, I don't think we should commit it
> ;).  

There are various of these added to the code.  They are EXTREMELY
helpful and even necessary to debug major code changes.  Of which the
mounts, mountmap changes were one of the toughest to debug.

Both you and I have already stated that we want to convert to using
python's logging.  These are just a first step towards that goal.  All
that is needed is to search out the #print's and replace them with teh
proper logging statement.

> I'll be happy once we're using subprocess :p.
> 

I have a number of improvements coming to the cmd().  but yeah, it'll
need more.

> A few typos, but I think this makes mount handling much more sane :).
> 
> Cheers,
> Trevor
> 

-- 
Brian Dolbec <dolsen@gentoo.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 3687 bytes --]

On Tue, Dec 17, 2013 at 09:18:04PM -0800, Brian Dolbec wrote:
> On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote:
> > On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> > > -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> > > -				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> > > +				"portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]),
> > > + 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> > 
> > Can we do this for the mountmap defaults too, and just override the
> > mountmap special cases in the SNAPCACHE branch?  I think this would be
> > a good commit for that (or we can do it in a commit after this).
> > 
> > I don't think the 'portage' → self.settings["repo_name"] replacement
> > should go in this commit.  
> 
> repo_name variable is already defined. This one got missed in the
> change.  You want it in the next commit by itself?  Can do.

That's what I want ;).  Just to avoid making this one more complicated
than it needs to be.

> > > -			self.mounts.append("/var/log/portage")
> > > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > > +			self.mounts.append("port_logdir")
> > > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > > +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> > 
> > I don't know where we stand on 'x=y' vs 'x = y', but I'd prefer the
> > latter here. 
> 
> I do too, but I thought it was agreed to fix all that separately
> later :/

Matt had me work some ' = ' spacing into the last series, but later
works for me.

> >  I also think that the PORT_LOGDIR environment variable
> > should be:
> > 
> >   self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]
> 
> meh, but I also prefer some uniformity.  I prefer all-caps for
> constants.  In this case, the end result later in a new defaults.py
> file...  since it is internal, why mix case in keys.  They are also
> pushed into the bash environment for the chroot scripts to use.  So,
> again, uniformity can be your friend when making changes.
> Especially with a large complex app like catalyst.

I don't care about caps, I was trying to fix settings → target_mounts,
since settings holds the mount source ;).

> > Nothing in the chroot should care where the source comes from ;).
> > I'd considering renaming mountmap → source_mounts for clarity,
> > using existing settings to override source_mounts at
> > initialization, and using source_mounts thereafter.
> 
> mounts and mountmap were existing variable names.  With some of the
> flack I've gotten over my choice of variable name already, now you
> want me to go through even more???  Do it in a separate commit.
> (after the rewrite patches are done for.)

Fair enough ;).

> > > +					#print "bind(); cmd =", cmd
> > 
> > If it's not useful enough to print, I don't think we should commit it
> > ;).  
> 
> There are various of these added to the code.  They are EXTREMELY
> helpful and even necessary to debug major code changes.  Of which the
> mounts, mountmap changes were one of the toughest to debug.
> 
> Both you and I have already stated that we want to convert to using
> python's logging.  These are just a first step towards that goal.  All
> that is needed is to search out the #print's and replace them with teh
> proper logging statement.

Ok.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Tue, 2013-12-17 at 21:35 -0800, W. Trevor King wrote:
> On Tue, Dec 17, 2013 at 09:18:04PM -0800, Brian Dolbec wrote:
> > On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote:
> > > On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:

> > > > -			self.mounts.append("/var/log/portage")
> > > > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > > > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > > > +			self.mounts.append("port_logdir")
> > > > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > > > +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> > > 

> > >  I also think that the PORT_LOGDIR environment variable
> > > should be:
> > > 
> > >   self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]

> I don't care about caps, I was trying to fix settings → target_mounts,
> since settings holds the mount source ;).
> 
> Cheers,
> Trevor
> 

No, this code block is adding it to the environment if defined in
settings.  This patch was just removing the hard coded paths.

This is the full if block:

		if "port_logdir" in self.settings:
			self.mounts.append("port_logdir")
			self.mountmap["port_logdir"]=self.settings["port_logdir"]
			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'

So far it is not used from the new TARGET_MOUNTS_DEFAULTS.  
It is so far only enabled if uncommented and defined in catalyst.conf or
from the cli.

Should I remove it from the TARGET_MOUNTS_DEFAULTS for the time being?
-- 
Brian Dolbec <dolsen@gentoo.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Brian Dolbec]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

On Wed, 2013-12-18 at 23:34 -0800, Brian Dolbec wrote:
> On Tue, 2013-12-17 at 21:35 -0800, W. Trevor King wrote:
> > On Tue, Dec 17, 2013 at 09:18:04PM -0800, Brian Dolbec wrote:
> > > On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote:
> > > > On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> 
> > > > > -			self.mounts.append("/var/log/portage")
> > > > > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > > > > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > > > > +			self.mounts.append("port_logdir")
> > > > > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > > > > +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> > > > 
> 
> > > >  I also think that the PORT_LOGDIR environment variable
> > > > should be:
> > > > 
> > > >   self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]
> 
> > I don't care about caps, I was trying to fix settings → target_mounts,
> > since settings holds the mount source ;).
> > 
> > Cheers,
> > Trevor
> > 
> 
> No, this code block is adding it to the environment if defined in
> settings.  This patch was just removing the hard coded paths.
> 
> This is the full if block:
> 
> 		if "port_logdir" in self.settings:
> 			self.mounts.append("port_logdir")
> 			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> 			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> 			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
> 
> So far it is not used from the new TARGET_MOUNTS_DEFAULTS.  
> It is so far only enabled if uncommented and defined in catalyst.conf or
> from the cli.
> 
> Should I remove it from the TARGET_MOUNTS_DEFAULTS for the time being?

What we could do is add a "PortageLog" to the options setting which
would turn on and use the defaults unless otherwise overridden by
setting the port_logdir config option.  But that would be something to
add later.

This patch is to fix/complete the mounts, mountmap changes from the
hard-coded paths and paths as keys.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 4531 bytes --]

On Wed, Dec 18, 2013 at 11:47:25PM -0800, Brian Dolbec wrote:
> On Wed, 2013-12-18 at 23:34 -0800, Brian Dolbec wrote:
> > On Tue, 2013-12-17 at 21:35 -0800, W. Trevor King wrote:
> > > On Tue, Dec 17, 2013 at 09:18:04PM -0800, Brian Dolbec wrote:
> > > > On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote:
> > > > > On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> > > > > > -			self.mounts.append("/var/log/portage")
> > > > > > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > > > > > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > > > > > +			self.mounts.append("port_logdir")
> > > > > > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > > > > > +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> > > > > 
> > > > >  I also think that the PORT_LOGDIR environment variable
> > > > > should be:
> > > > > 
> > > > >   self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]
> > >
> > > I don't care about caps, I was trying to fix settings → target_mounts,
> > > since settings holds the mount source ;).
> > 
> > No, this code block is adding it to the environment if defined in
> > settings.  This patch was just removing the hard coded paths.
> > 
> > This is the full if block:
> > 
> > 		if "port_logdir" in self.settings:
> > 			self.mounts.append("port_logdir")
> > 			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > 			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> > 			self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
> > 
> > So far it is not used from the new TARGET_MOUNTS_DEFAULTS.  
> > It is so far only enabled if uncommented and defined in catalyst.conf or
> > from the cli.
> > 
> > Should I remove it from the TARGET_MOUNTS_DEFAULTS for the time being?
> 
> What we could do is add a "PortageLog" to the options setting which
> would turn on and use the defaults unless otherwise overridden by
> setting the port_logdir config option.  But that would be something to
> add later.
> 
> This patch is to fix/complete the mounts, mountmap changes from the
> hard-coded paths and paths as keys.

Who is consuming the env dict?  I thought it was for scripts running
in the chroot, in which case it can't possibly care about the mount
source, but might conceivably care about the mount target.  In any
case, current usage in your pending branch is inconsistent:

  $ git grep 'self\.env\[.*\] *=' origin/pending --
  origin/pending:modules/generic_stage_target.py:    self.env["CCACHE_DIR"] = self.target_mounts["ccache"]
  origin/pending:modules/generic_stage_target.py:    self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"]
  origin/pending:modules/generic_stage_target.py:    self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
  origin/pending:modules/generic_stage_target.py:    self.env["PORT_LOGDIR_CLEAN"]='find "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete'
  origin/pending:modules/generic_stage_target.py:    self.env[varname]=self.settings[x]
  origin/pending:modules/generic_stage_target.py:    self.env[varname]=string.join(self.settings[x])
  origin/pending:modules/generic_stage_target.py:    self.env[varname]="true"
  origin/pending:modules/generic_stage_target.py:    self.env[varname]="false"
  origin/pending:modules/generic_stage_target.py:    self.env["MAKEOPTS"]=self.settings["makeopts"]
  origin/pending:modules/generic_stage_target.py:    self.env[kname+"_kernelopts"]=myopts
  origin/pending:modules/generic_stage_target.py:    self.env[kname+"_kernelopts"]=""
  origin/pending:modules/generic_stage_target.py:    self.env["clst_kextraversion"]=\
  origin/pending:modules/generic_target.py:    self.env["PATH"]="/bin:/sbin:/usr/bin:/usr/sbin"

CCACHE_DIR is set using target_mounts (which makes sense to me), but
PORT_LOGDIR is set using settings (which does not make sense to me).
For example, if I decide to configure port_logdir (via my Catalyst
settings) to /some/wonky/path/on/the/host/system, I want Catalyst to
bind-mount that to /var/tmp/portage
(TARGET_MOUNTS_DEFAULTS['port_logdir']) and then have emerges in the
chroot use /var/tmp/portage.  If emerges in the chroot try to use
/some/wonky/path/on/the/host/system thinks are not going to work.

Cheers,
TRevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]