{"thread":{"id":"51618","subject":"Non-robust lock files in containers can lead to repo corruption","startedAt":"2019-08-10T16:05:48Z","lastAt":"2019-08-13T19:13:19Z","messageCount":5,"participants":["Gregory Szorc","Randall S. Becker","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380265","messageId":"qimq0i$49gn$1@blaine.gmane.org","threadId":"51618","inReplyTo":null,"subject":"Non-robust lock files in containers can lead to repo corruption","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2019-08-10T16:05:33Z","receivedAt":"2019-08-10T16:05:48Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"I tracked down a source of Git corrupting repositories to lock file\ndesign not being robust when containers / PID namespaces are present.\n\nIn my case, the corruption stemmed from premature release of the `git\ngc` lock in the gc.pid file. But since the lock file code for that file\nis in gc.c, there could be other lock files in Git affected by the same\ndesign limitation as well.\n\nThe lock design of gc.pid stores the current hostname and PID of the\nlocking process in the file. If another process comes along and its\nhostname matches the stored hostname, it checks to see if the listed PID\nexists. If the PID is missing, it assumes the lock is stale and releases\nthe lock.\n\nA limitation with this approach is it isn't robust in the presence of\ncontainers / PID namespaces. In containers, it is common for the\nhostname to match the container host's hostname. Or the hostname will be\nstatic string. In Kubernetes, all containers within a pod share the same\nhostname. Containers (almost always) run in separate PID namespaces, so\nPIDs from outside the container aren't visible to the container itself.\nThis means that if e.g. 2 `git gc` processes are running with the same\nhostname in separate containers / PID namespaces, Git could prematurely\nrelease the lock file because it thinks the \"other\" PID is dead and repo\ncorruption could ensue due to the 2 `git gc` processes racing with each\nother.\n\nThe on-disk format of lock files obviously needs to be backwards\ncompatible with older clients. One backwards compatible solution is to\nappend something to the hostname to disambiguate containers / PID\nnamespaces. Mercurial appends the current PID namespace identifier to\nthe hostname [1] and my experience is that this is sufficient to\nmitigate the issue. It is possible more robust solutions are achievable.\n\nGregory\n\n[1] https://www.mercurial-scm.org/repo/hg/rev/1f151a33af8e\n\n"},{"id":"380304","messageId":"003901d55114$9428df40$bc7a9dc0$@nexbridge.com","threadId":"51618","inReplyTo":"qimq0i$49gn$1@blaine.gmane.org","subject":"RE: Non-robust lock files in containers can lead to repo corruption","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-08-12T13:48:04Z","receivedAt":"2019-08-12T13:48:14Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On August 10, 2019 12:06 PM, Gregory Szorc wrote:\n> I tracked down a source of Git corrupting repositories to lock file design not\n> being robust when containers / PID namespaces are present.\n> \n> In my case, the corruption stemmed from premature release of the `git gc`\n> lock in the gc.pid file. But since the lock file code for that file is in gc.c, there\n> could be other lock files in Git affected by the same design limitation as well.\n> \n> The lock design of gc.pid stores the current hostname and PID of the locking\n> process in the file. If another process comes along and its hostname matches\n> the stored hostname, it checks to see if the listed PID exists. If the PID is\n> missing, it assumes the lock is stale and releases the lock.\n> \n> A limitation with this approach is it isn't robust in the presence of containers\n> / PID namespaces. In containers, it is common for the hostname to match\n> the container host's hostname. Or the hostname will be static string. In\n> Kubernetes, all containers within a pod share the same hostname. Containers\n> (almost always) run in separate PID namespaces, so PIDs from outside the\n> container aren't visible to the container itself.\n> This means that if e.g. 2 `git gc` processes are running with the same\n> hostname in separate containers / PID namespaces, Git could prematurely\n> release the lock file because it thinks the \"other\" PID is dead and repo\n> corruption could ensue due to the 2 `git gc` processes racing with each other.\n> \n> The on-disk format of lock files obviously needs to be backwards compatible\n> with older clients. One backwards compatible solution is to append\n> something to the hostname to disambiguate containers / PID namespaces.\n> Mercurial appends the current PID namespace identifier to the hostname [1]\n> and my experience is that this is sufficient to mitigate the issue. It is possible\n> more robust solutions are achievable.\n\nWhile I like the idea personally, many platforms, including NonStop (TNS/E),  do not support pid namespaces. In particular setns(2) may not be implemented. Please make sure that any changes detect this condition properly and omit the use of namespaces.\n\nRegards,\nRandall\n\n\n"},{"id":"380309","messageId":"xmqqk1bibcqs.fsf@gitster-ct.c.googlers.com","threadId":"51618","inReplyTo":"003901d55114$9428df40$bc7a9dc0$@nexbridge.com","subject":"Re: Non-robust lock files in containers can lead to repo corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-12T15:54:35Z","receivedAt":"2019-08-12T15:54:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n>> The lock design of gc.pid stores the current hostname and PID of the locking\n>> process in the file. If another process comes along and its hostname matches\n>> the stored hostname, it checks to see if the listed PID exists. If the PID is\n>> missing, it assumes the lock is stale and releases the lock.\n\nThe assumption that <hostname, pid> pair can be used to identify\nrunning process (until the pid wraps around and gets reused) does\nnot sound particularly limited Git.  Don't container folks solve it\nwithout touching individual applications?  For example, is it a\nviable option to isolate UTS namespaces (CLONE_NEWUTS) in addition\nto PID namespaces (CLONE_NEWPID)?\n"},{"id":"380316","messageId":"20190812163812.GB26231@sigill.intra.peff.net","threadId":"51618","inReplyTo":"qimq0i$49gn$1@blaine.gmane.org","subject":"Re: Non-robust lock files in containers can lead to repo corruption","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-12T16:38:13Z","receivedAt":"2019-08-12T16:38:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 10, 2019 at 09:05:33AM -0700, Gregory Szorc wrote:\n\n> I tracked down a source of Git corrupting repositories to lock file\n> design not being robust when containers / PID namespaces are present.\n> \n> In my case, the corruption stemmed from premature release of the `git\n> gc` lock in the gc.pid file. But since the lock file code for that file\n> is in gc.c, there could be other lock files in Git affected by the same\n> design limitation as well.\n\nI don't think there are. Most of Git's locks are predicated purely on\nthe existence of the lockfile (with the intent that they'd work over\nsystems like NFS). The gc lock is a weird one-off.\n\nAnd while it's not great for multiple gc's to run at the same time\n(because it wastes CPU), two of them running at the same time shouldn't\ncause a corruption. If you have a reproducible demonstration where that\nhappens, I'd be very interested to see it.\n\n-Peff\n"},{"id":"380412","messageId":"xmqq36i498vs.fsf@gitster-ct.c.googlers.com","threadId":"51618","inReplyTo":"20190812163812.GB26231@sigill.intra.peff.net","subject":"Re: Non-robust lock files in containers can lead to repo corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-13T19:13:11Z","receivedAt":"2019-08-13T19:13:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't think there are. Most of Git's locks are predicated purely on\n> the existence of the lockfile (with the intent that they'd work over\n> systems like NFS). The gc lock is a weird one-off.\n>\n> And while it's not great for multiple gc's to run at the same time\n> (because it wastes CPU), two of them running at the same time shouldn't\n> cause a corruption. If you have a reproducible demonstration where that\n> happens, I'd be very interested to see it.\n\nGood point.\n\nAnd come to think of it, gc \"lock\" does not have to be a lock to\nbegin with.  It is not \"I am forbidding all of you guys from doing\ngc, because that would break the result of gc _I_ am doing right\nnow\", which is what we traditionally call \"lock\".  It is merely a\n\"We need to do this every once in a while and I am doing it now.  I\nlet others know that I am already doing so, so they do not have to\nstart the same thing right now\" advisory.\n\nAnd the code (i.e. lock_repo_for_gc()) allows the current process to\nrun when\n\n - \"--force\" option is given, or\n - the lockfile cannot be open()ed, or\n - the lockfile cannot be fstat()ed, or\n - the lockfile is older than 12 hours, or\n - the lockfile has malformed contents, or\n - the lockfile was taken on a host with the same name from ours,\n   and a process with the same pid as recorded is not running.\n\nFollowing the \"\"\"12 hour limit is very generous as gc should never\ntake that long. On the other hand we don't really need a strict\nlimit here, running gc --auto one day late is not a big\nproblem. --force can be used in manual gc after the user verifies\nthat no gc is running.\"\"\" reasoning, I suspect that it shouldn't be\ntoo bad even if we dropped the last condition (i.e. \"is the process\nstill running?\")  from the set of these conditions.\n"}]}