From 58c7358113a2dfc456100f20dbd5db3b1ff996d4 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Thu, 14 Jun 2018 00:41:45 +0300 Subject: [PATCH 01/12] Unit tests for PGP signing/verification These unit-tests cover operations via both PGP providers: built-in `openpgp` and external `gpg`. Next step is to run these tests for gpg1 & gpg2 as separate entities. --- pgp/gnupg.go | 14 +--- pgp/gnupg_test.go | 32 +++++++ pgp/internal_test.go | 87 ++++--------------- pgp/keyrings/aptly.pub | Bin 0 -> 915 bytes pgp/keyrings/aptly.sec | Bin 0 -> 977 bytes pgp/keyrings/aptly_passphrase.pub | Bin 0 -> 915 bytes pgp/keyrings/aptly_passphrase.sec | Bin 0 -> 1052 bytes pgp/sign_test.go | 134 ++++++++++++++++++++++++++++++ pgp/verify_test.go | 93 +++++++++++++++++++++ 9 files changed, 274 insertions(+), 86 deletions(-) create mode 100644 pgp/keyrings/aptly.pub create mode 100644 pgp/keyrings/aptly.sec create mode 100644 pgp/keyrings/aptly_passphrase.pub create mode 100644 pgp/keyrings/aptly_passphrase.sec create mode 100644 pgp/sign_test.go create mode 100644 pgp/verify_test.go diff --git a/pgp/gnupg.go b/pgp/gnupg.go index 5d416281..ba5f24b3 100644 --- a/pgp/gnupg.go +++ b/pgp/gnupg.go @@ -174,7 +174,7 @@ type GpgVerifier struct { keyRings []string } -// NewGpgVerifier creates a new gpg signer +// NewGpgVerifier creates a new gpg verifier func NewGpgVerifier() *GpgVerifier { gpg, err := findGPG1() if err != nil { @@ -191,18 +191,6 @@ func NewGpgVerifier() *GpgVerifier { // InitKeyring verifies that gpg is installed and some keys are trusted func (g *GpgVerifier) InitKeyring() error { - cmd, err := findGPG1() - if err != nil { - return err - } - g.gpg = cmd - - cmd, err = findGPGV1() - if err != nil { - return err - } - g.gpgv = cmd - if len(g.keyRings) == 0 { // using default keyring output, err := exec.Command(g.gpg, "--no-default-keyring", "--no-auto-check-trustdb", "--keyring", "trustedkeys.gpg", "--list-keys").Output() diff --git a/pgp/gnupg_test.go b/pgp/gnupg_test.go index 670ab114..30e93ca3 100644 --- a/pgp/gnupg_test.go +++ b/pgp/gnupg_test.go @@ -77,3 +77,35 @@ func (s *GnupgSuite) TestGPGVNothing(c *C) { c.Assert(func() { NewGpgVerifier() }, PanicMatches, `Couldn't find a suitable gpgv executable.+`) } + +type Gnupg1VerifierSuite struct { + VerifierSuite +} + +var _ = Suite(&Gnupg1VerifierSuite{}) + +func (s *Gnupg1VerifierSuite) SetUpTest(c *C) { + s.verifier = NewGpgVerifier() + s.verifier.AddKeyring("./trusted.gpg") + + c.Assert(s.verifier.InitKeyring(), IsNil) +} + +type Gnupg1SignerSuite struct { + SignerSuite +} + +var _ = Suite(&Gnupg1SignerSuite{}) + +func (s *Gnupg1SignerSuite) SetUpTest(c *C) { + s.signer = NewGpgSigner() + s.signer.SetBatch(true) + + s.verifier = &GoVerifier{} + s.verifier.AddKeyring("./keyrings/aptly.pub") + s.verifier.AddKeyring("./keyrings/aptly_passphrase.pub") + + c.Assert(s.verifier.InitKeyring(), IsNil) + + s.SignerSuite.SetUpTest(c) +} diff --git a/pgp/internal_test.go b/pgp/internal_test.go index 772e52bd..79fd60d5 100644 --- a/pgp/internal_test.go +++ b/pgp/internal_test.go @@ -1,14 +1,11 @@ package pgp import ( - "io/ioutil" - "os" - . "gopkg.in/check.v1" ) type GoVerifierSuite struct { - verifier Verifier + VerifierSuite } var _ = Suite(&GoVerifierSuite{}) @@ -20,77 +17,21 @@ func (s *GoVerifierSuite) SetUpTest(c *C) { c.Assert(s.verifier.InitKeyring(), IsNil) } -func (s *GoVerifierSuite) TestVerifyDetached(c *C) { - for _, test := range []struct { - textName, signatureName string - }{ - {"1.text", "1.signature"}, - {"2.text", "2.signature"}, - {"3.text", "3.signature"}, - } { - cleartext, err := os.Open(test.textName) - c.Assert(err, IsNil) - - signature, err := os.Open(test.signatureName) - c.Assert(err, IsNil) - - err = s.verifier.VerifyDetachedSignature(signature, cleartext, false) - c.Assert(err, IsNil) - - signature.Close() - cleartext.Close() - } +type GoSignerSuite struct { + SignerSuite } -func (s *GoVerifierSuite) TestVerifyClearsigned(c *C) { - for _, test := range []struct { - clearSignedName string - }{ - {"1.clearsigned"}, - } { - clearsigned, err := os.Open(test.clearSignedName) - c.Assert(err, IsNil) +var _ = Suite(&GoSignerSuite{}) - keyInfo, err := s.verifier.VerifyClearsigned(clearsigned, false) - c.Assert(err, IsNil) - c.Check(keyInfo.GoodKeys, DeepEquals, []Key{"8B48AD6246925553", "7638D0442B90D010"}) - c.Check(keyInfo.MissingKeys, DeepEquals, []Key(nil)) +func (s *GoSignerSuite) SetUpTest(c *C) { + s.signer = &GoSigner{} + s.signer.SetBatch(true) - clearsigned.Close() - } -} - -func (s *GoVerifierSuite) TestExtractClearsigned(c *C) { - for _, test := range []struct { - clearSignedName, clearTextName string - }{ - {"1.clearsigned", "1.cleartext"}, - } { - clearsigned, err := os.Open(test.clearSignedName) - c.Assert(err, IsNil) - - cleartext, err := os.Open(test.clearTextName) - c.Assert(err, IsNil) - - is, err := s.verifier.IsClearSigned(clearsigned) - c.Assert(err, IsNil) - c.Check(is, Equals, true) - - clearsigned.Seek(0, 0) - - extractedF, err := s.verifier.ExtractClearsigned(clearsigned) - c.Assert(err, IsNil) - - expected, err := ioutil.ReadAll(cleartext) - c.Assert(err, IsNil) - - extracted, err := ioutil.ReadAll(extractedF) - c.Assert(err, IsNil) - - c.Check(expected, DeepEquals, extracted) - - extractedF.Close() - clearsigned.Close() - cleartext.Close() - } + s.verifier = &GoVerifier{} + s.verifier.AddKeyring("./keyrings/aptly.pub") + s.verifier.AddKeyring("./keyrings/aptly_passphrase.pub") + + c.Assert(s.verifier.InitKeyring(), IsNil) + + s.SignerSuite.SetUpTest(c) } diff --git a/pgp/keyrings/aptly.pub b/pgp/keyrings/aptly.pub new file mode 100644 index 0000000000000000000000000000000000000000..08758e4302de6ce4f49a2af5c44c3933784c7aaa GIT binary patch literal 915 zcmV;E18n@60ipy_`=yT&1OU0Xct7+Dh9|KF?G`P|&qxfns=ia+?_C}Z3Q77aY&oDm z=DPPGT}v`rhosG`k(mHKkP*T<;A>!VCtNV^wrbkr>Li1NuVz(`hR_)ne z3th6NpdiQ7<Fwy!G{GEu6DDvIH>J2*jMl!ZAe17%%=dLdNX9~nLZfdb`w!OtYzEZ zU$tG7+;oa-r;o+m6H6^)RKfTUEn;!>+{aCdpam;laCrjBCQ$?bPc%>)c$RkTb8gOo zrva@giIB^900*$mRq4tsG9J{_hH8zMD=;ruT--<4KXg9&0lu4`2*h(Q_s&%G^(Qn2 zVuFZYUd@|3-K+MvV;N!hz-S>vtdu82MEjJB_U#G=RRF%3O;1~3hdN8HZHx{C9 z1CNbQHrRY!)*y&Ov@Jn!bZmJbRAqB?WpW@WWN&UKbRczeWguyEDIh#_Wpi{uVQ_S8 zc`j*gW^X=-VgwTr0stZf0#f^>j{+Mb1`7!Y2Ll2I6$kzyoEjx*d~P+M;6PgR0^f+5npe&Ob>g0mP-)vE<&{=?mIr z23Ur<0SyFF`=yT%1OW6E{*kf_=~7suCOy4D*L(qZ6Ku9KyXE#t+4?gNe8$M_(Kmq- zP;Yssfh76u=2mM8H4M=@hjQBhqbgdyn+ihl14|aY*z!=QmG;cwgo~FYwKN))*q5??(Uw1q1-f`}d!7QG;m{ zd-x)&h$fKs*|h%_ERc**yvS^~>Cd~Ej{+ME0162ZA=|i|7TZ2;IfekBy@su2 pbXyVDebzEULM=)oxk=@Z0G>PR9ZewfiG<^n85QUm&on6Zx23xEox=bC literal 0 HcmV?d00001 diff --git a/pgp/keyrings/aptly.sec b/pgp/keyrings/aptly.sec new file mode 100644 index 0000000000000000000000000000000000000000..f90e1c913181ff15a8dc9434d9b43a64ccefee16 GIT binary patch literal 977 zcmV;?11|iP0lNfJ`=yT&1OU0Xct7+Dh9|KF?G`P|&qxfns=ia+?_C}Z3Q77aY&oDm z=DPPGT}v`rhosG`k(mHKkP*T<;A>!VCtNV^wrbkr>Li1NuVz(`hR_)ne z3th6NpdiQ7<Fwy!G{GEu6DDvIH>J2*jMl!ZAe17%%=dLdNX9~nLZfdb`w!OtYzEZ zU$tG7+;oa-r;o+m6H6^)RKfTUEn;!>+{aCdpam;laCrjBCQ$?bPc%>)c$RkTb8gOo zrva@giIB^900*$mRq4tsG9J{_hH8zMD=;ruT--<4KXg9&0lu4`2*h(Q_s&%G^(Qn2 zVuFZYUd@|3-K+MvV;N!hz-S>vtdu82MEjJB_U#G=RRF%3O;1~3hdN8HZHx{C9 z1CNbQHrRY!)*y&O005w)1dfm)xUZvne1p<;!PnDwZ9?-4)U+)@aCB^WAXH^@bY*fN zC}eMLCv+fnb7dfDbSWS_bY*jNKw)rnY0HE3a(ZB;`uDTtQSlXgu zM=odGfgQv0Qk5Cj1975qb5DQ zLDzf%coS^4GrQ&XN!j`{4}8YR?a?=Z5>Rh>r-3B-?dDc%3N;MTI)`%G9>92-n&|a% z)LlaXfply-t^?#vGJ^2g{?cK~Krj|0&Rn4^{p$0Xv3rm7)`x=C9dStXI_Eb}gm_=( zKriske%2Twp6^Eh0|f*C%KP`9b5Vn76MOg~tB59$_Sv-m7A%mAQM|}(x9QKjnB(_V zXtb9;au%wdN7F+V;=@Y=go{bR&2wgoV)FVDFfpOyZ35#g;O5Kp{lK80cnONvC==;` zMkJTzP8M|J&oob%LA4M)&u6GkeK2k=j4ojRt+6foRS$h2I9E${008;|bEM3c%`#AN zru560gfdK6&xeM|gQ~xG>`LCTiFnl$FNjG57!d*h2?YXD`=yTp8w>yn2@oOMxSST- zK5aRM0H0M8G=^ntPd5ey_o*~SD*~Mfd4vF;U8*?cN<@Qwtw{io%pDp^eS*?nqr;}I-q#?f(6RWI z-OXcvP^lCq*Y(mz1OSYhefdk_v|koHb|0LjoxH?qTlgA$bn7b870Rh(E1cglq~G{> zWL?y~4}83gH#RycF+D%8ydsilZBTai=zl~H98lZSjS(v;$SGmeyYy8*$-j|mLziOa z{obdM51&ogV40AocwqWy+AlT_X#AfTRq$3*Tw5Ulo75{ z5GH^K+7jr|XFr#NX?6%so!OiZh!Wm`05$wkSQradtZH3SskB4Zw%f(MlI2R-7a=l+ z{8{$?w!RKVk-wSPG5bS^_ZEKw8Ne}Uuq?ynS-DDNnLJuM7R5qGzPE3U5p_#<2X?R+ zuJ3`wrlz5JMxYP|v@Jn!bZmJbRAqB?WpW@WWN&UKbRczeWguyEDIh#_Wpi{uVQ_S8 zc`j*gW^X=-VgwTr0stZf0#pF_hXNZT1`7!Y2Ll2I6$kfGlbf zPQGN+gxEe1^{B*lQvX=Ds$?0p8=xACrv5hEUyUMX#wm|s_X7X}1_S`N5*%@3y6^74 z;EF*YS!LmMi4Gt({SiHgpC!7P5z)S3^=$Wb)i;Na;L}$HUX!z_VwFNdy=X0ssjG0#pF_hXNZ60162Z^A3!;&E6{bVvhizhW1^F pQh@M!p0s?4b08k6=4vgD0G>G%4Dudkh>m4~)&`HDtX0QWgp6=$od5s; literal 0 HcmV?d00001 diff --git a/pgp/keyrings/aptly_passphrase.sec b/pgp/keyrings/aptly_passphrase.sec new file mode 100644 index 0000000000000000000000000000000000000000..2ffe24ef89c4e7d10037073e8862f7503f8618cc GIT binary patch literal 1052 zcmV+%1mpXa0pSEx0QiRy1OSd4WNFxt1~o!-S1@skL_54;9Geppna?(rQ9M6;^61m0 zTSc%gL&7ChBb7d0Or!C1EV9=15fqC^yng{ATtzFu#5gJRR62G-ad;H|u8fW1KTwD| zCDAtEgtlGHCcZom-PUPIqgO@njh~(Gx=D-y-tNOz))K;%IhEXR8>;}I-q#?f(6RWI z-OXcvP^lCq*Y(mz1OSYhefdk_v|koHb|0LjoxH?qTlgA$bn7b870Rh(E1cglq~G{> zWL?y~4}83gH#RycF+D%8ydsilZBTai=zl~H98lZSjS(v;$SGmeyYy8*$-j|mLziOa z{obdM51&ogV40AocwqWy+AlT_X#AfTRq$3*Tw5Ulo75{ z5GH^K+7jr|XFr#NX?6%so!OiZh!Wm`05$wkSQradtZH3SskB4Zw%f(MlI2R-7a=l+ z{8{$?w!RKVk-wSPG5bS^_ZEKw8Ne}Uuq?ynS-DDNnLJuM7R5qGzPE3U5p_#<2X?R+ zuJ3`wrlz5JMxYP|{sRL7D=XB?HUmj|U^$$e&t2|=B_YXad=mkJ=kX&cY2vWjGN3nB zD-j=-J1wYJT@3T@DFIFtwk?epcH;rGEkST}Yyg0vjU+3ke7Z0|EvW2m%QT3j`Jd z0|5da0Rk6*0162Z^A3!;&E6{bMJNEA4<(}n&Y3^*){v!(-Bs8-C|2#O0HCM&2wQDL zL1o(Y!zdTRXwe3#C+wX8R|Hf5_=gY#0O}1zeu71#Pm+V%W03BEpY+~@YNsmzLu}9< z&~xP=E_?QwN@)RxScujUPBN7AA^+HG%i6s$B3mGKZ5tj*`G&8FiOB@0g)rSHZ@?Eg zK@T)4WW2ZO_6d|uzGT#d*gg>TsKje>Z zf0g#1l9hhR0AF)>qD$GcIyY`KhK&{VxJh?n50SC%vbZd5af68kggg`TG9C524rc0esPistbb8{ZDgdC~ W)GO}0JmQ4yFg1%k_1g1bkD$UqR@;aG literal 0 HcmV?d00001 diff --git a/pgp/sign_test.go b/pgp/sign_test.go new file mode 100644 index 00000000..583329e7 --- /dev/null +++ b/pgp/sign_test.go @@ -0,0 +1,134 @@ +package pgp + +import ( + "crypto/rand" + "io" + "io/ioutil" + "os" + "path" + + . "gopkg.in/check.v1" +) + +// Common set of tests shared by internal & external GnuPG implementations +type SignerSuite struct { + signer Signer + verifier Verifier + + clearF *os.File + signedF *os.File + cleartext []byte + + passwordFile string +} + +func (s *SignerSuite) SetUpTest(c *C) { + tempDir := c.MkDir() + + var err error + s.clearF, err = os.Create(path.Join(tempDir, "cleartext")) + c.Assert(err, IsNil) + + s.cleartext = make([]byte, 0, 1024) + _, err = rand.Read(s.cleartext) + c.Assert(err, IsNil) + + _, err = s.clearF.Write(s.cleartext) + c.Assert(err, IsNil) + + _, err = s.clearF.Seek(0, io.SeekStart) + c.Assert(err, IsNil) + + s.signedF, err = os.Create(path.Join(tempDir, "signed")) + c.Assert(err, IsNil) + + s.passwordFile = path.Join(tempDir, "password") + f, err := os.OpenFile(s.passwordFile, os.O_CREATE|os.O_WRONLY, 0600) + c.Assert(err, IsNil) + + _, err = f.Write([]byte("verysecret")) + c.Assert(err, IsNil) + + f.Close() + + s.signer.SetBatch(true) +} + +func (s *SignerSuite) TearDownTest(c *C) { + s.clearF.Close() + s.signedF.Close() +} + +func (s *SignerSuite) testSignDetached(c *C) { + c.Assert(s.signer.Init(), IsNil) + + err := s.signer.DetachedSign(s.clearF.Name(), s.signedF.Name()) + c.Assert(err, IsNil) + + err = s.verifier.VerifyDetachedSignature(s.signedF, s.clearF, false) + c.Assert(err, IsNil) +} + +func (s *SignerSuite) TestSignDetachedNoPassphrase(c *C) { + s.signer.SetKeyRing("keyrings/aptly.pub", "keyrings/aptly.sec") + + s.testSignDetached(c) +} + +func (s *SignerSuite) TestSignDetachedPassphrase(c *C) { + s.signer.SetKeyRing("keyrings/aptly_passphrase.pub", "keyrings/aptly_passphrase.sec") + s.signer.SetPassphrase("verysecret", "") + + s.testSignDetached(c) +} + +func (s *SignerSuite) TestSignDetachedPassphraseFile(c *C) { + s.signer.SetKeyRing("keyrings/aptly_passphrase.pub", "keyrings/aptly_passphrase.sec") + s.signer.SetPassphrase("", s.passwordFile) + + s.testSignDetached(c) +} + +func (s *SignerSuite) testClearSign(c *C, expectedKey Key) { + c.Assert(s.signer.Init(), IsNil) + + err := s.signer.ClearSign(s.clearF.Name(), s.signedF.Name()) + c.Assert(err, IsNil) + + keyInfo, err := s.verifier.VerifyClearsigned(s.signedF, false) + c.Assert(err, IsNil) + + c.Assert(keyInfo.GoodKeys, DeepEquals, []Key{expectedKey}) + c.Assert(keyInfo.MissingKeys, DeepEquals, []Key(nil)) + + _, err = s.signedF.Seek(0, io.SeekStart) + c.Assert(err, IsNil) + extractedF, err := s.verifier.ExtractClearsigned(s.signedF) + c.Assert(err, IsNil) + defer extractedF.Close() + + extracted, err := ioutil.ReadAll(extractedF) + c.Assert(err, IsNil) + + c.Assert(extracted, DeepEquals, s.cleartext) +} + +func (s *SignerSuite) TestClearSignNoPassphrase(c *C) { + s.signer.SetKeyRing("keyrings/aptly.pub", "keyrings/aptly.sec") + + s.testClearSign(c, "21DBB89C16DB3E6D") +} + +func (s *SignerSuite) TestClearSignPassphrase(c *C) { + s.signer.SetKeyRing("keyrings/aptly_passphrase.pub", "keyrings/aptly_passphrase.sec") + s.signer.SetPassphrase("verysecret", "") + + s.testClearSign(c, "F30E8CB9CDDE2AF8") +} + +func (s *SignerSuite) TestClearSignPassphraseFile(c *C) { + s.signer.SetKeyRing("keyrings/aptly_passphrase.pub", "keyrings/aptly_passphrase.sec") + s.signer.SetPassphrase("", s.passwordFile) + + s.testClearSign(c, "F30E8CB9CDDE2AF8") +} diff --git a/pgp/verify_test.go b/pgp/verify_test.go new file mode 100644 index 00000000..a5071534 --- /dev/null +++ b/pgp/verify_test.go @@ -0,0 +1,93 @@ +package pgp + +import ( + "bytes" + "io/ioutil" + "os" + + . "gopkg.in/check.v1" +) + +// Common set of tests shared by internal & external GnuPG implementations +type VerifierSuite struct { + verifier Verifier +} + +func (s *VerifierSuite) TestVerifyDetached(c *C) { + for _, test := range []struct { + textName, signatureName string + }{ + {"1.text", "1.signature"}, + {"2.text", "2.signature"}, + {"3.text", "3.signature"}, + } { + cleartext, err := os.Open(test.textName) + c.Assert(err, IsNil) + + signature, err := os.Open(test.signatureName) + c.Assert(err, IsNil) + + err = s.verifier.VerifyDetachedSignature(signature, cleartext, false) + c.Assert(err, IsNil) + + signature.Close() + cleartext.Close() + } +} + +func (s *VerifierSuite) TestVerifyClearsigned(c *C) { + for _, test := range []struct { + clearSignedName string + }{ + {"1.clearsigned"}, + } { + clearsigned, err := os.Open(test.clearSignedName) + c.Assert(err, IsNil) + + keyInfo, err := s.verifier.VerifyClearsigned(clearsigned, false) + c.Assert(err, IsNil) + c.Check(keyInfo.GoodKeys, DeepEquals, []Key{"8B48AD6246925553", "7638D0442B90D010"}) + c.Check(keyInfo.MissingKeys, DeepEquals, []Key(nil)) + + clearsigned.Close() + } +} + +func (s *VerifierSuite) TestExtractClearsigned(c *C) { + for _, test := range []struct { + clearSignedName, clearTextName string + }{ + {"1.clearsigned", "1.cleartext"}, + } { + clearsigned, err := os.Open(test.clearSignedName) + c.Assert(err, IsNil) + + cleartext, err := os.Open(test.clearTextName) + c.Assert(err, IsNil) + + is, err := s.verifier.IsClearSigned(clearsigned) + c.Assert(err, IsNil) + c.Check(is, Equals, true) + + clearsigned.Seek(0, 0) + + extractedF, err := s.verifier.ExtractClearsigned(clearsigned) + c.Assert(err, IsNil) + + expected, err := ioutil.ReadAll(cleartext) + c.Assert(err, IsNil) + + extracted, err := ioutil.ReadAll(extractedF) + c.Assert(err, IsNil) + + // normalize newlines + extracted = bytes.TrimRight(bytes.Replace(extracted, []byte("\r\n"), []byte("\n"), -1), "\n") + expected = bytes.Replace(expected, []byte("\r\n"), []byte("\n"), -1) + + c.Check(extracted, DeepEquals, expected) + + extractedF.Close() + clearsigned.Close() + cleartext.Close() + } +} From 747b9752cec9bfe072f85a3a6dfef4be678c269d Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Sat, 14 Jul 2018 00:17:36 +0300 Subject: [PATCH 02/12] Keep checksum of not compressed index file even if it's not uploaded Fixes: #756 --- deb/index_files.go | 5 ++++- system/t06_publish/repo.py | 8 ++++++++ system/t06_publish/snapshot.py | 12 ++++++++++++ system/t06_publish/switch.py | 12 ++++++++++++ system/t06_publish/update.py | 8 ++++++++ 5 files changed, 44 insertions(+), 1 deletion(-) diff --git a/deb/index_files.go b/deb/index_files.go index 713e6c95..78200b66 100644 --- a/deb/index_files.go +++ b/deb/index_files.go @@ -77,14 +77,17 @@ func (file *indexFile) Finalize(signer pgp.Signer) error { file.tempFile.Close() exts := []string{""} + cksumExts := exts if file.compressable { exts = append(exts, ".gz", ".bz2") + cksumExts = exts if file.onlyGzip { exts = []string{".gz"} + cksumExts = []string{"", ".gz"} } } - for _, ext := range exts { + for _, ext := range cksumExts { var checksumInfo utils.ChecksumInfo checksumInfo, err = utils.ChecksumsForFile(file.tempFilename + ext) diff --git a/system/t06_publish/repo.py b/system/t06_publish/repo.py index b0892ccd..4f1af5cb 100644 --- a/system/t06_publish/repo.py +++ b/system/t06_publish/repo.py @@ -69,6 +69,10 @@ class PublishRepo1Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) @@ -464,6 +468,10 @@ class PublishRepo17Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) diff --git a/system/t06_publish/snapshot.py b/system/t06_publish/snapshot.py index 9ed1f6f3..ec3382af 100644 --- a/system/t06_publish/snapshot.py +++ b/system/t06_publish/snapshot.py @@ -79,6 +79,10 @@ class PublishSnapshot1Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) @@ -724,6 +728,10 @@ class PublishSnapshot26Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) @@ -947,6 +955,10 @@ class PublishSnapshot35Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) diff --git a/system/t06_publish/switch.py b/system/t06_publish/switch.py index 5ee91d0a..e1413207 100644 --- a/system/t06_publish/switch.py +++ b/system/t06_publish/switch.py @@ -61,6 +61,10 @@ class PublishSwitch1Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) @@ -321,6 +325,10 @@ class PublishSwitch8Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) @@ -506,6 +514,10 @@ class PublishSwitch14Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) diff --git a/system/t06_publish/update.py b/system/t06_publish/update.py index c917c9cb..b27b852f 100644 --- a/system/t06_publish/update.py +++ b/system/t06_publish/update.py @@ -61,6 +61,10 @@ class PublishUpdate1Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) @@ -402,6 +406,10 @@ class PublishUpdate12Test(BaseTest): pathsSeen = set() for l in release: fileHash, fileSize, path = l.split() + if "Contents" in path and not path.endswith(".gz"): + # "Contents" are present in index, but not really written to disk + continue + pathsSeen.add(path) fileSize = int(fileSize) From 021b8c4cff213dbceb5348f3f6a480edda2a6f69 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Fri, 20 Jul 2018 01:04:51 +0300 Subject: [PATCH 03/12] Lower memory usage for `aptly db cleanup` This is not a complete fix, but the easiest first step. During `db cleanup`, aptly is loading every repo/mirror/... into memory, and even though each object is processed only once, collection holds a reference to all the loaded objects, so they won't be GC'd until process exits. CollectionFactory.Flush() releases pointers to collection objects, making objects egligble for GC. This is not a complete fix, as during iteration we could have tried to release a link to every object being GCed and that would have helped much more. --- cmd/db_cleanup.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cmd/db_cleanup.go b/cmd/db_cleanup.go index 4a1b0535..be246915 100644 --- a/cmd/db_cleanup.go +++ b/cmd/db_cleanup.go @@ -59,6 +59,8 @@ func aptlyDbCleanup(cmd *commander.Command, args []string) error { return err } + context.CollectionFactory().Flush() + if verbose { context.Progress().ColoredPrintf("@{y}Loading local repos:@|") } @@ -90,6 +92,8 @@ func aptlyDbCleanup(cmd *commander.Command, args []string) error { return err } + context.CollectionFactory().Flush() + if verbose { context.Progress().ColoredPrintf("@{y}Loading snapshots:@|") } @@ -118,6 +122,8 @@ func aptlyDbCleanup(cmd *commander.Command, args []string) error { return err } + context.CollectionFactory().Flush() + if verbose { context.Progress().ColoredPrintf("@{y}Loading published repositories:@|") } @@ -150,6 +156,8 @@ func aptlyDbCleanup(cmd *commander.Command, args []string) error { return err } + context.CollectionFactory().Flush() + // ... and compare it to the list of all packages context.Progress().ColoredPrintf("@{w!}Loading list of all packages...@|") allPackageRefs := context.CollectionFactory().PackageCollection().AllPackageRefs() @@ -192,6 +200,8 @@ func aptlyDbCleanup(cmd *commander.Command, args []string) error { } } + context.CollectionFactory().Flush() + // now, build a list of files that should be present in Repository (package pool) context.Progress().ColoredPrintf("@{w!}Building list of files referenced by packages...@|") referencedFiles := make([]string, 0, existingPackageRefs.Len()) From 0f4bbc4752e41c9f8f818afa8d46a40ff34927e5 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Fri, 3 Aug 2018 00:59:18 +0300 Subject: [PATCH 04/12] Implement lazy iteration (ForEach) over collections See #761 aptly had a concept of loading small amount of info per each object into memory once collection is accessed for the first time. This might have simplified some operations, but it doesn't scale well with huge aptly databases. This is just intermediate step towards better memory management - list of objects is not loaded unless some method is called. `ForEach` method (mainly used in cleanup) is reimplemented to iterate over database without ever loading all the objects into memory. Memory was even worse with previous approach, as for each item usually `LoadComplete()` is called, which pulls even more data into memory and item stays in memory till the end of the iteration as it is referenced from `collection.list`. For the subsequent PR: reimplement `ByUUID()` and probably other methods to avoid loading all the items into memory, at least for all the collecitons except for published repos. When published repository is being loaded, it might pull source local repo which in turn would trigger loading for all the local repos which is not acceptable. --- deb/local.go | 43 ++++++++++++++++++++++++++++------------- deb/publish.go | 50 +++++++++++++++++++++++++++++++++++------------- deb/remote.go | 41 ++++++++++++++++++++++++++------------- deb/snapshot.go | 51 +++++++++++++++++++++++++++++++++++-------------- 4 files changed, 132 insertions(+), 53 deletions(-) diff --git a/deb/local.go b/deb/local.go index e9fa17cb..9b2207c6 100644 --- a/deb/local.go +++ b/deb/local.go @@ -99,28 +99,34 @@ type LocalRepoCollection struct { // NewLocalRepoCollection loads LocalRepos from DB and makes up collection func NewLocalRepoCollection(db database.Storage) *LocalRepoCollection { - result := &LocalRepoCollection{ + return &LocalRepoCollection{ RWMutex: &sync.RWMutex{}, db: db, } +} - blobs := db.FetchByPrefix([]byte("L")) - result.list = make([]*LocalRepo, 0, len(blobs)) +func (collection *LocalRepoCollection) loadList() { + if collection.list != nil { + return + } + + blobs := collection.db.FetchByPrefix([]byte("L")) + collection.list = make([]*LocalRepo, 0, len(blobs)) for _, blob := range blobs { r := &LocalRepo{} if err := r.Decode(blob); err != nil { log.Printf("Error decoding repo: %s\n", err) } else { - result.list = append(result.list, r) + collection.list = append(collection.list, r) } } - - return result } // Add appends new repo to collection and saves it func (collection *LocalRepoCollection) Add(repo *LocalRepo) error { + collection.loadList() + for _, r := range collection.list { if r.Name == repo.Name { return fmt.Errorf("local repo with name %s already exists", repo.Name) @@ -153,6 +159,8 @@ func (collection *LocalRepoCollection) Update(repo *LocalRepo) error { // LoadComplete loads additional information for local repo func (collection *LocalRepoCollection) LoadComplete(repo *LocalRepo) error { + collection.loadList() + encoded, err := collection.db.Get(repo.RefKey()) if err == database.ErrNotFound { return nil @@ -167,6 +175,8 @@ func (collection *LocalRepoCollection) LoadComplete(repo *LocalRepo) error { // ByName looks up repository by name func (collection *LocalRepoCollection) ByName(name string) (*LocalRepo, error) { + collection.loadList() + for _, r := range collection.list { if r.Name == name { return r, nil @@ -177,6 +187,8 @@ func (collection *LocalRepoCollection) ByName(name string) (*LocalRepo, error) { // ByUUID looks up repository by uuid func (collection *LocalRepoCollection) ByUUID(uuid string) (*LocalRepo, error) { + collection.loadList() + for _, r := range collection.list { if r.UUID == uuid { return r, nil @@ -187,23 +199,28 @@ func (collection *LocalRepoCollection) ByUUID(uuid string) (*LocalRepo, error) { // ForEach runs method for each repository func (collection *LocalRepoCollection) ForEach(handler func(*LocalRepo) error) error { - var err error - for _, r := range collection.list { - err = handler(r) - if err != nil { - return err + return collection.db.ProcessByPrefix([]byte("L"), func(key, blob []byte) error { + r := &LocalRepo{} + if err := r.Decode(blob); err != nil { + log.Printf("Error decoding repo: %s\n", err) + return nil } - } - return err + + return handler(r) + }) } // Len returns number of remote repos func (collection *LocalRepoCollection) Len() int { + collection.loadList() + return len(collection.list) } // Drop removes remote repo from collection func (collection *LocalRepoCollection) Drop(repo *LocalRepo) error { + collection.loadList() + repoPosition := -1 for i, r := range collection.list { diff --git a/deb/publish.go b/deb/publish.go index 13152ef5..d8d8ba3e 100644 --- a/deb/publish.go +++ b/deb/publish.go @@ -852,28 +852,34 @@ type PublishedRepoCollection struct { // NewPublishedRepoCollection loads PublishedRepos from DB and makes up collection func NewPublishedRepoCollection(db database.Storage) *PublishedRepoCollection { - result := &PublishedRepoCollection{ + return &PublishedRepoCollection{ RWMutex: &sync.RWMutex{}, db: db, } +} - blobs := db.FetchByPrefix([]byte("U")) - result.list = make([]*PublishedRepo, 0, len(blobs)) +func (collection *PublishedRepoCollection) loadList() { + if collection.list != nil { + return + } + + blobs := collection.db.FetchByPrefix([]byte("U")) + collection.list = make([]*PublishedRepo, 0, len(blobs)) for _, blob := range blobs { r := &PublishedRepo{} if err := r.Decode(blob); err != nil { log.Printf("Error decoding published repo: %s\n", err) } else { - result.list = append(result.list, r) + collection.list = append(collection.list, r) } } - - return result } // Add appends new repo to collection and saves it func (collection *PublishedRepoCollection) Add(repo *PublishedRepo) error { + collection.loadList() + if collection.CheckDuplicate(repo) != nil { return fmt.Errorf("published repo with storage/prefix/distribution %s/%s/%s already exists", repo.Storage, repo.Prefix, repo.Distribution) } @@ -889,6 +895,8 @@ func (collection *PublishedRepoCollection) Add(repo *PublishedRepo) error { // CheckDuplicate verifies that there's no published repo with the same name func (collection *PublishedRepoCollection) CheckDuplicate(repo *PublishedRepo) *PublishedRepo { + collection.loadList() + for _, r := range collection.list { if r.Prefix == repo.Prefix && r.Distribution == repo.Distribution && r.Storage == repo.Storage { return r @@ -978,6 +986,8 @@ func (collection *PublishedRepoCollection) LoadComplete(repo *PublishedRepo, col // ByStoragePrefixDistribution looks up repository by storage, prefix & distribution func (collection *PublishedRepoCollection) ByStoragePrefixDistribution(storage, prefix, distribution string) (*PublishedRepo, error) { + collection.loadList() + for _, r := range collection.list { if r.Prefix == prefix && r.Distribution == distribution && r.Storage == storage { return r, nil @@ -991,6 +1001,8 @@ func (collection *PublishedRepoCollection) ByStoragePrefixDistribution(storage, // ByUUID looks up repository by uuid func (collection *PublishedRepoCollection) ByUUID(uuid string) (*PublishedRepo, error) { + collection.loadList() + for _, r := range collection.list { if r.UUID == uuid { return r, nil @@ -1001,6 +1013,8 @@ func (collection *PublishedRepoCollection) ByUUID(uuid string) (*PublishedRepo, // BySnapshot looks up repository by snapshot source func (collection *PublishedRepoCollection) BySnapshot(snapshot *Snapshot) []*PublishedRepo { + collection.loadList() + var result []*PublishedRepo for _, r := range collection.list { if r.SourceKind == SourceSnapshot { @@ -1021,6 +1035,8 @@ func (collection *PublishedRepoCollection) BySnapshot(snapshot *Snapshot) []*Pub // ByLocalRepo looks up repository by local repo source func (collection *PublishedRepoCollection) ByLocalRepo(repo *LocalRepo) []*PublishedRepo { + collection.loadList() + var result []*PublishedRepo for _, r := range collection.list { if r.SourceKind == SourceLocalRepo { @@ -1041,18 +1057,21 @@ func (collection *PublishedRepoCollection) ByLocalRepo(repo *LocalRepo) []*Publi // ForEach runs method for each repository func (collection *PublishedRepoCollection) ForEach(handler func(*PublishedRepo) error) error { - var err error - for _, r := range collection.list { - err = handler(r) - if err != nil { - return err + return collection.db.ProcessByPrefix([]byte("U"), func(key, blob []byte) error { + r := &PublishedRepo{} + if err := r.Decode(blob); err != nil { + log.Printf("Error decoding published repo: %s\n", err) + return nil } - } - return err + + return handler(r) + }) } // Len returns number of remote repos func (collection *PublishedRepoCollection) Len() int { + collection.loadList() + return len(collection.list) } @@ -1060,6 +1079,8 @@ func (collection *PublishedRepoCollection) Len() int { func (collection *PublishedRepoCollection) CleanupPrefixComponentFiles(prefix string, components []string, publishedStorage aptly.PublishedStorage, collectionFactory *CollectionFactory, progress aptly.Progress) error { + collection.loadList() + var err error referencedFiles := map[string][]string{} @@ -1141,6 +1162,9 @@ func (collection *PublishedRepoCollection) CleanupPrefixComponentFiles(prefix st func (collection *PublishedRepoCollection) Remove(publishedStorageProvider aptly.PublishedStorageProvider, storage, prefix, distribution string, collectionFactory *CollectionFactory, progress aptly.Progress, force, skipCleanup bool) error { + + collection.loadList() + repo, err := collection.ByStoragePrefixDistribution(storage, prefix, distribution) if err != nil { return err diff --git a/deb/remote.go b/deb/remote.go index 167d45f1..2646dd0d 100644 --- a/deb/remote.go +++ b/deb/remote.go @@ -660,28 +660,34 @@ type RemoteRepoCollection struct { // NewRemoteRepoCollection loads RemoteRepos from DB and makes up collection func NewRemoteRepoCollection(db database.Storage) *RemoteRepoCollection { - result := &RemoteRepoCollection{ + return &RemoteRepoCollection{ RWMutex: &sync.RWMutex{}, db: db, } +} - blobs := db.FetchByPrefix([]byte("R")) - result.list = make([]*RemoteRepo, 0, len(blobs)) +func (collection *RemoteRepoCollection) loadList() { + if collection.list != nil { + return + } + + blobs := collection.db.FetchByPrefix([]byte("R")) + collection.list = make([]*RemoteRepo, 0, len(blobs)) for _, blob := range blobs { r := &RemoteRepo{} if err := r.Decode(blob); err != nil { log.Printf("Error decoding mirror: %s\n", err) } else { - result.list = append(result.list, r) + collection.list = append(collection.list, r) } } - - return result } // Add appends new repo to collection and saves it func (collection *RemoteRepoCollection) Add(repo *RemoteRepo) error { + collection.loadList() + for _, r := range collection.list { if r.Name == repo.Name { return fmt.Errorf("mirror with name %s already exists", repo.Name) @@ -728,6 +734,8 @@ func (collection *RemoteRepoCollection) LoadComplete(repo *RemoteRepo) error { // ByName looks up repository by name func (collection *RemoteRepoCollection) ByName(name string) (*RemoteRepo, error) { + collection.loadList() + for _, r := range collection.list { if r.Name == name { return r, nil @@ -738,6 +746,8 @@ func (collection *RemoteRepoCollection) ByName(name string) (*RemoteRepo, error) // ByUUID looks up repository by uuid func (collection *RemoteRepoCollection) ByUUID(uuid string) (*RemoteRepo, error) { + collection.loadList() + for _, r := range collection.list { if r.UUID == uuid { return r, nil @@ -748,23 +758,28 @@ func (collection *RemoteRepoCollection) ByUUID(uuid string) (*RemoteRepo, error) // ForEach runs method for each repository func (collection *RemoteRepoCollection) ForEach(handler func(*RemoteRepo) error) error { - var err error - for _, r := range collection.list { - err = handler(r) - if err != nil { - return err + return collection.db.ProcessByPrefix([]byte("R"), func(key, blob []byte) error { + r := &RemoteRepo{} + if err := r.Decode(blob); err != nil { + log.Printf("Error decoding mirror: %s\n", err) + return nil } - } - return err + + return handler(r) + }) } // Len returns number of remote repos func (collection *RemoteRepoCollection) Len() int { + collection.loadList() + return len(collection.list) } // Drop removes remote repo from collection func (collection *RemoteRepoCollection) Drop(repo *RemoteRepo) error { + collection.loadList() + repoPosition := -1 for i, r := range collection.list { diff --git a/deb/snapshot.go b/deb/snapshot.go index fc7689c8..bdf32480 100644 --- a/deb/snapshot.go +++ b/deb/snapshot.go @@ -179,28 +179,34 @@ type SnapshotCollection struct { // NewSnapshotCollection loads Snapshots from DB and makes up collection func NewSnapshotCollection(db database.Storage) *SnapshotCollection { - result := &SnapshotCollection{ + return &SnapshotCollection{ RWMutex: &sync.RWMutex{}, db: db, } +} - blobs := db.FetchByPrefix([]byte("S")) - result.list = make([]*Snapshot, 0, len(blobs)) +func (collection *SnapshotCollection) loadList() { + if collection.list != nil { + return + } + + blobs := collection.db.FetchByPrefix([]byte("S")) + collection.list = make([]*Snapshot, 0, len(blobs)) for _, blob := range blobs { s := &Snapshot{} if err := s.Decode(blob); err != nil { log.Printf("Error decoding snapshot: %s\n", err) } else { - result.list = append(result.list, s) + collection.list = append(collection.list, s) } } - - return result } // Add appends new repo to collection and saves it func (collection *SnapshotCollection) Add(snapshot *Snapshot) error { + collection.loadList() + for _, s := range collection.list { if s.Name == snapshot.Name { return fmt.Errorf("snapshot with name %s already exists", snapshot.Name) @@ -216,7 +222,7 @@ func (collection *SnapshotCollection) Add(snapshot *Snapshot) error { return nil } -// Update stores updated information about repo in DB +// Update stores updated information about snapshot in DB func (collection *SnapshotCollection) Update(snapshot *Snapshot) error { err := collection.db.Put(snapshot.Key(), snapshot.Encode()) if err != nil { @@ -241,6 +247,8 @@ func (collection *SnapshotCollection) LoadComplete(snapshot *Snapshot) error { // ByName looks up snapshot by name func (collection *SnapshotCollection) ByName(name string) (*Snapshot, error) { + collection.loadList() + for _, s := range collection.list { if s.Name == name { return s, nil @@ -251,6 +259,8 @@ func (collection *SnapshotCollection) ByName(name string) (*Snapshot, error) { // ByUUID looks up snapshot by UUID func (collection *SnapshotCollection) ByUUID(uuid string) (*Snapshot, error) { + collection.loadList() + for _, s := range collection.list { if s.UUID == uuid { return s, nil @@ -261,6 +271,8 @@ func (collection *SnapshotCollection) ByUUID(uuid string) (*Snapshot, error) { // ByRemoteRepoSource looks up snapshots that have specified RemoteRepo as a source func (collection *SnapshotCollection) ByRemoteRepoSource(repo *RemoteRepo) []*Snapshot { + collection.loadList() + var result []*Snapshot for _, s := range collection.list { @@ -273,6 +285,8 @@ func (collection *SnapshotCollection) ByRemoteRepoSource(repo *RemoteRepo) []*Sn // ByLocalRepoSource looks up snapshots that have specified LocalRepo as a source func (collection *SnapshotCollection) ByLocalRepoSource(repo *LocalRepo) []*Snapshot { + collection.loadList() + var result []*Snapshot for _, s := range collection.list { @@ -285,6 +299,8 @@ func (collection *SnapshotCollection) ByLocalRepoSource(repo *LocalRepo) []*Snap // BySnapshotSource looks up snapshots that have specified snapshot as a source func (collection *SnapshotCollection) BySnapshotSource(snapshot *Snapshot) []*Snapshot { + collection.loadList() + var result []*Snapshot for _, s := range collection.list { @@ -297,18 +313,21 @@ func (collection *SnapshotCollection) BySnapshotSource(snapshot *Snapshot) []*Sn // ForEach runs method for each snapshot func (collection *SnapshotCollection) ForEach(handler func(*Snapshot) error) error { - var err error - for _, s := range collection.list { - err = handler(s) - if err != nil { - return err + return collection.db.ProcessByPrefix([]byte("S"), func(key, blob []byte) error { + s := &Snapshot{} + if err := s.Decode(blob); err != nil { + log.Printf("Error decoding snapshot: %s\n", err) + return nil } - } - return err + + return handler(s) + }) } // ForEachSorted runs method for each snapshot following some sort order func (collection *SnapshotCollection) ForEachSorted(sortMethod string, handler func(*Snapshot) error) error { + collection.loadList() + sorter, err := newSnapshotSorter(sortMethod, collection) if err != nil { return err @@ -327,11 +346,15 @@ func (collection *SnapshotCollection) ForEachSorted(sortMethod string, handler f // Len returns number of snapshots in collection // ForEach runs method for each snapshot func (collection *SnapshotCollection) Len() int { + collection.loadList() + return len(collection.list) } // Drop removes snapshot from collection func (collection *SnapshotCollection) Drop(snapshot *Snapshot) error { + collection.loadList() + snapshotPosition := -1 for i, s := range collection.list { From de38011dd25f37c651e9d9e8285998d52ce961db Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Tue, 14 Aug 2018 00:56:15 +0300 Subject: [PATCH 05/12] Add simple benchmark for SnapshotCollection.ForEach() --- deb/snapshot_bench_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 deb/snapshot_bench_test.go diff --git a/deb/snapshot_bench_test.go b/deb/snapshot_bench_test.go new file mode 100644 index 00000000..19997c2c --- /dev/null +++ b/deb/snapshot_bench_test.go @@ -0,0 +1,38 @@ +package deb + +import ( + "fmt" + "os" + "testing" + + "github.com/aptly-dev/aptly/database" +) + +func BenchmarkSnapshotCollectionForEach(b *testing.B) { + const count = 1024 + + tmpDir := os.TempDir() + defer os.RemoveAll(tmpDir) + + db, _ := database.NewOpenDB(tmpDir) + defer db.Close() + + collection := NewSnapshotCollection(db) + + for i := 0; i < count; i++ { + snapshot := NewSnapshotFromRefList(fmt.Sprintf("snapshot%d", i), nil, NewPackageRefList(), fmt.Sprintf("Snapshot number %d", i)) + if collection.Add(snapshot) != nil { + b.FailNow() + } + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + collection = NewSnapshotCollection(db) + + collection.ForEach(func(s *Snapshot) error { + return nil + }) + } +} From 5a9f4bee1293af4dabc6ca1bce40ae00c308b91a Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Fri, 17 Aug 2018 18:11:49 +0300 Subject: [PATCH 06/12] Fix system tests on `master` branch --- system/t04_mirror/UpdateMirror19Test_gold | 2 +- system/t04_mirror/UpdateMirror21Test_gold | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/system/t04_mirror/UpdateMirror19Test_gold b/system/t04_mirror/UpdateMirror19Test_gold index 668bac58..ac3ee71b 100644 --- a/system/t04_mirror/UpdateMirror19Test_gold +++ b/system/t04_mirror/UpdateMirror19Test_gold @@ -6,6 +6,6 @@ gpgv: Good signature from "Package Maintainer (PagerDuty, Inc.) Date: Tue, 7 Aug 2018 01:09:32 +0300 Subject: [PATCH 07/12] Reimplement DB collections for mirrors, repos and snapshots See #765, #761 Collections were relying on keeping in-memory list of all the objects for any kind of operation which doesn't scale well the number of objects in the database. With this rewrite, objects are loaded only on demand which might be pessimization in some edge cases but should improve performance and memory footprint signifcantly. --- Makefile | 5 +- deb/local.go | 126 +++++++++++++---------- deb/local_test.go | 5 + deb/remote.go | 123 ++++++++++++---------- deb/remote_test.go | 5 + deb/snapshot.go | 204 ++++++++++++++++++------------------- deb/snapshot_bench_test.go | 60 +++++++++++ deb/snapshot_test.go | 28 ++++- 8 files changed, 340 insertions(+), 216 deletions(-) diff --git a/Makefile b/Makefile index ff4cee4f..7d3702d8 100644 --- a/Makefile +++ b/Makefile @@ -13,7 +13,7 @@ RUN_LONG_TESTS?=yes GO_1_10_AND_HIGHER=$(shell (printf '%s\n' go1.10 $(GOVERSION) | sort -cV >/dev/null 2>&1) && echo "yes") -all: test check system-test +all: test bench check system-test prepare: go get -u github.com/alecthomas/gometalinter @@ -57,6 +57,9 @@ else go test -v `go list ./... | grep -v vendor/` -gocheck.v=true endif +bench: + go test -v ./deb -run=nothing -bench=. -benchmem + mem.png: mem.dat mem.gp gnuplot mem.gp open mem.png diff --git a/deb/local.go b/deb/local.go index 9b2207c6..229f6e1d 100644 --- a/deb/local.go +++ b/deb/local.go @@ -2,6 +2,7 @@ package deb import ( "bytes" + "errors" "fmt" "log" "sync" @@ -93,8 +94,8 @@ func (repo *LocalRepo) RefKey() []byte { // LocalRepoCollection does listing, updating/adding/deleting of LocalRepos type LocalRepoCollection struct { *sync.RWMutex - db database.Storage - list []*LocalRepo + db database.Storage + cache map[string]*LocalRepo } // NewLocalRepoCollection loads LocalRepos from DB and makes up collection @@ -102,43 +103,59 @@ func NewLocalRepoCollection(db database.Storage) *LocalRepoCollection { return &LocalRepoCollection{ RWMutex: &sync.RWMutex{}, db: db, + cache: make(map[string]*LocalRepo), } } -func (collection *LocalRepoCollection) loadList() { - if collection.list != nil { - return - } - - blobs := collection.db.FetchByPrefix([]byte("L")) - collection.list = make([]*LocalRepo, 0, len(blobs)) - - for _, blob := range blobs { - r := &LocalRepo{} - if err := r.Decode(blob); err != nil { - log.Printf("Error decoding repo: %s\n", err) - } else { - collection.list = append(collection.list, r) +func (collection *LocalRepoCollection) search(filter func(*LocalRepo) bool, unique bool) []*LocalRepo { + result := []*LocalRepo(nil) + for _, r := range collection.cache { + if filter(r) { + result = append(result, r) } } + + if unique && len(result) > 0 { + return result + } + + collection.db.ProcessByPrefix([]byte("L"), func(key, blob []byte) error { + r := &LocalRepo{} + if err := r.Decode(blob); err != nil { + log.Printf("Error decoding local repo: %s\n", err) + return nil + } + + if filter(r) { + if _, exists := collection.cache[r.UUID]; !exists { + collection.cache[r.UUID] = r + result = append(result, r) + if unique { + return errors.New("abort") + } + } + } + + return nil + }) + + return result } // Add appends new repo to collection and saves it func (collection *LocalRepoCollection) Add(repo *LocalRepo) error { - collection.loadList() + _, err := collection.ByName(repo.Name) - for _, r := range collection.list { - if r.Name == repo.Name { - return fmt.Errorf("local repo with name %s already exists", repo.Name) - } + if err == nil { + return fmt.Errorf("local repo with name %s already exists", repo.Name) } - err := collection.Update(repo) + err = collection.Update(repo) if err != nil { return err } - collection.list = append(collection.list, repo) + collection.cache[repo.UUID] = repo return nil } @@ -159,8 +176,6 @@ func (collection *LocalRepoCollection) Update(repo *LocalRepo) error { // LoadComplete loads additional information for local repo func (collection *LocalRepoCollection) LoadComplete(repo *LocalRepo) error { - collection.loadList() - encoded, err := collection.db.Get(repo.RefKey()) if err == database.ErrNotFound { return nil @@ -175,26 +190,39 @@ func (collection *LocalRepoCollection) LoadComplete(repo *LocalRepo) error { // ByName looks up repository by name func (collection *LocalRepoCollection) ByName(name string) (*LocalRepo, error) { - collection.loadList() - - for _, r := range collection.list { - if r.Name == name { - return r, nil - } + result := collection.search(func(r *LocalRepo) bool { return r.Name == name }, true) + if len(result) == 0 { + return nil, fmt.Errorf("local repo with name %s not found", name) } - return nil, fmt.Errorf("local repo with name %s not found", name) + + return result[0], nil } // ByUUID looks up repository by uuid func (collection *LocalRepoCollection) ByUUID(uuid string) (*LocalRepo, error) { - collection.loadList() - - for _, r := range collection.list { - if r.UUID == uuid { - return r, nil - } + if r, ok := collection.cache[uuid]; ok { + return r, nil } - return nil, fmt.Errorf("local repo with uuid %s not found", uuid) + + key := (&LocalRepo{UUID: uuid}).Key() + + value, err := collection.db.Get(key) + if err == database.ErrNotFound { + return nil, fmt.Errorf("local repo with uuid %s not found", uuid) + } + + if err != nil { + return nil, err + } + + r := &LocalRepo{} + err = r.Decode(value) + + if err == nil { + collection.cache[r.UUID] = r + } + + return r, err } // ForEach runs method for each repository @@ -212,30 +240,16 @@ func (collection *LocalRepoCollection) ForEach(handler func(*LocalRepo) error) e // Len returns number of remote repos func (collection *LocalRepoCollection) Len() int { - collection.loadList() - - return len(collection.list) + return len(collection.db.KeysByPrefix([]byte("L"))) } // Drop removes remote repo from collection func (collection *LocalRepoCollection) Drop(repo *LocalRepo) error { - collection.loadList() - - repoPosition := -1 - - for i, r := range collection.list { - if r == repo { - repoPosition = i - break - } - } - - if repoPosition == -1 { + if _, err := collection.db.Get(repo.Key()); err == database.ErrNotFound { panic("local repo not found!") } - collection.list[len(collection.list)-1], collection.list[repoPosition], collection.list = - nil, collection.list[len(collection.list)-1], collection.list[:len(collection.list)-1] + delete(collection.cache, repo.UUID) err := collection.db.Delete(repo.Key()) if err != nil { diff --git a/deb/local_test.go b/deb/local_test.go index e472463d..82df0497 100644 --- a/deb/local_test.go +++ b/deb/local_test.go @@ -124,6 +124,11 @@ func (s *LocalRepoCollectionSuite) TestByUUID(c *C) { r, err := s.collection.ByUUID(repo.UUID) c.Assert(err, IsNil) + c.Assert(r, Equals, repo) + + collection := NewLocalRepoCollection(s.db) + r, err = collection.ByUUID(repo.UUID) + c.Assert(err, IsNil) c.Assert(r.String(), Equals, repo.String()) } diff --git a/deb/remote.go b/deb/remote.go index 2646dd0d..cc26eef1 100644 --- a/deb/remote.go +++ b/deb/remote.go @@ -3,6 +3,7 @@ package deb import ( "bytes" gocontext "context" + "errors" "fmt" "log" "net/url" @@ -654,8 +655,8 @@ func (repo *RemoteRepo) RefKey() []byte { // RemoteRepoCollection does listing, updating/adding/deleting of RemoteRepos type RemoteRepoCollection struct { *sync.RWMutex - db database.Storage - list []*RemoteRepo + db database.Storage + cache map[string]*RemoteRepo } // NewRemoteRepoCollection loads RemoteRepos from DB and makes up collection @@ -663,43 +664,59 @@ func NewRemoteRepoCollection(db database.Storage) *RemoteRepoCollection { return &RemoteRepoCollection{ RWMutex: &sync.RWMutex{}, db: db, + cache: make(map[string]*RemoteRepo), } } -func (collection *RemoteRepoCollection) loadList() { - if collection.list != nil { - return - } - - blobs := collection.db.FetchByPrefix([]byte("R")) - collection.list = make([]*RemoteRepo, 0, len(blobs)) - - for _, blob := range blobs { - r := &RemoteRepo{} - if err := r.Decode(blob); err != nil { - log.Printf("Error decoding mirror: %s\n", err) - } else { - collection.list = append(collection.list, r) +func (collection *RemoteRepoCollection) search(filter func(*RemoteRepo) bool, unique bool) []*RemoteRepo { + result := []*RemoteRepo(nil) + for _, r := range collection.cache { + if filter(r) { + result = append(result, r) } } + + if unique && len(result) > 0 { + return result + } + + collection.db.ProcessByPrefix([]byte("R"), func(key, blob []byte) error { + r := &RemoteRepo{} + if err := r.Decode(blob); err != nil { + log.Printf("Error decoding remote repo: %s\n", err) + return nil + } + + if filter(r) { + if _, exists := collection.cache[r.UUID]; !exists { + collection.cache[r.UUID] = r + result = append(result, r) + if unique { + return errors.New("abort") + } + } + } + + return nil + }) + + return result } // Add appends new repo to collection and saves it func (collection *RemoteRepoCollection) Add(repo *RemoteRepo) error { - collection.loadList() + _, err := collection.ByName(repo.Name) - for _, r := range collection.list { - if r.Name == repo.Name { - return fmt.Errorf("mirror with name %s already exists", repo.Name) - } + if err == nil { + return fmt.Errorf("mirror with name %s already exists", repo.Name) } - err := collection.Update(repo) + err = collection.Update(repo) if err != nil { return err } - collection.list = append(collection.list, repo) + collection.cache[repo.UUID] = repo return nil } @@ -734,26 +751,38 @@ func (collection *RemoteRepoCollection) LoadComplete(repo *RemoteRepo) error { // ByName looks up repository by name func (collection *RemoteRepoCollection) ByName(name string) (*RemoteRepo, error) { - collection.loadList() - - for _, r := range collection.list { - if r.Name == name { - return r, nil - } + result := collection.search(func(r *RemoteRepo) bool { return r.Name == name }, true) + if len(result) == 0 { + return nil, fmt.Errorf("mirror with name %s not found", name) } - return nil, fmt.Errorf("mirror with name %s not found", name) + + return result[0], nil } // ByUUID looks up repository by uuid func (collection *RemoteRepoCollection) ByUUID(uuid string) (*RemoteRepo, error) { - collection.loadList() - - for _, r := range collection.list { - if r.UUID == uuid { - return r, nil - } + if r, ok := collection.cache[uuid]; ok { + return r, nil } - return nil, fmt.Errorf("mirror with uuid %s not found", uuid) + + key := (&RemoteRepo{UUID: uuid}).Key() + + value, err := collection.db.Get(key) + if err == database.ErrNotFound { + return nil, fmt.Errorf("mirror with uuid %s not found", uuid) + } + if err != nil { + return nil, err + } + + r := &RemoteRepo{} + err = r.Decode(value) + + if err == nil { + collection.cache[r.UUID] = r + } + + return r, err } // ForEach runs method for each repository @@ -771,30 +800,16 @@ func (collection *RemoteRepoCollection) ForEach(handler func(*RemoteRepo) error) // Len returns number of remote repos func (collection *RemoteRepoCollection) Len() int { - collection.loadList() - - return len(collection.list) + return len(collection.db.KeysByPrefix([]byte("R"))) } // Drop removes remote repo from collection func (collection *RemoteRepoCollection) Drop(repo *RemoteRepo) error { - collection.loadList() - - repoPosition := -1 - - for i, r := range collection.list { - if r == repo { - repoPosition = i - break - } - } - - if repoPosition == -1 { + if _, err := collection.db.Get(repo.Key()); err == database.ErrNotFound { panic("repo not found!") } - collection.list[len(collection.list)-1], collection.list[repoPosition], collection.list = - nil, collection.list[len(collection.list)-1], collection.list[:len(collection.list)-1] + delete(collection.cache, repo.UUID) err := collection.db.Delete(repo.Key()) if err != nil { diff --git a/deb/remote_test.go b/deb/remote_test.go index 568ec5af..08fb7f64 100644 --- a/deb/remote_test.go +++ b/deb/remote_test.go @@ -651,6 +651,11 @@ func (s *RemoteRepoCollectionSuite) TestByUUID(c *C) { r, err := s.collection.ByUUID(repo.UUID) c.Assert(err, IsNil) + c.Assert(r, Equals, repo) + + collection := NewRemoteRepoCollection(s.db) + r, err = collection.ByUUID(repo.UUID) + c.Assert(err, IsNil) c.Assert(r.String(), Equals, repo.String()) } diff --git a/deb/snapshot.go b/deb/snapshot.go index bdf32480..307433af 100644 --- a/deb/snapshot.go +++ b/deb/snapshot.go @@ -173,8 +173,8 @@ func (s *Snapshot) Decode(input []byte) error { // SnapshotCollection does listing, updating/adding/deleting of Snapshots type SnapshotCollection struct { *sync.RWMutex - db database.Storage - list []*Snapshot + db database.Storage + cache map[string]*Snapshot } // NewSnapshotCollection loads Snapshots from DB and makes up collection @@ -182,43 +182,23 @@ func NewSnapshotCollection(db database.Storage) *SnapshotCollection { return &SnapshotCollection{ RWMutex: &sync.RWMutex{}, db: db, - } -} - -func (collection *SnapshotCollection) loadList() { - if collection.list != nil { - return - } - - blobs := collection.db.FetchByPrefix([]byte("S")) - collection.list = make([]*Snapshot, 0, len(blobs)) - - for _, blob := range blobs { - s := &Snapshot{} - if err := s.Decode(blob); err != nil { - log.Printf("Error decoding snapshot: %s\n", err) - } else { - collection.list = append(collection.list, s) - } + cache: map[string]*Snapshot{}, } } // Add appends new repo to collection and saves it func (collection *SnapshotCollection) Add(snapshot *Snapshot) error { - collection.loadList() - - for _, s := range collection.list { - if s.Name == snapshot.Name { - return fmt.Errorf("snapshot with name %s already exists", snapshot.Name) - } + _, err := collection.ByName(snapshot.Name) + if err == nil { + return fmt.Errorf("snapshot with name %s already exists", snapshot.Name) } - err := collection.Update(snapshot) + err = collection.Update(snapshot) if err != nil { return err } - collection.list = append(collection.list, snapshot) + collection.cache[snapshot.UUID] = snapshot return nil } @@ -245,70 +225,96 @@ func (collection *SnapshotCollection) LoadComplete(snapshot *Snapshot) error { return snapshot.packageRefs.Decode(encoded) } -// ByName looks up snapshot by name -func (collection *SnapshotCollection) ByName(name string) (*Snapshot, error) { - collection.loadList() - - for _, s := range collection.list { - if s.Name == name { - return s, nil +func (collection *SnapshotCollection) search(filter func(*Snapshot) bool, unique bool) []*Snapshot { + result := []*Snapshot(nil) + for _, s := range collection.cache { + if filter(s) { + result = append(result, s) } } + + if unique && len(result) > 0 { + return result + } + + collection.db.ProcessByPrefix([]byte("S"), func(key, blob []byte) error { + s := &Snapshot{} + if err := s.Decode(blob); err != nil { + log.Printf("Error decoding snapshot: %s\n", err) + return nil + } + + if filter(s) { + if _, exists := collection.cache[s.UUID]; !exists { + collection.cache[s.UUID] = s + result = append(result, s) + if unique { + return errors.New("abort") + } + } + } + + return nil + }) + + return result +} + +// ByName looks up snapshot by name +func (collection *SnapshotCollection) ByName(name string) (*Snapshot, error) { + result := collection.search(func(s *Snapshot) bool { return s.Name == name }, true) + if len(result) > 0 { + return result[0], nil + } + return nil, fmt.Errorf("snapshot with name %s not found", name) } // ByUUID looks up snapshot by UUID func (collection *SnapshotCollection) ByUUID(uuid string) (*Snapshot, error) { - collection.loadList() - - for _, s := range collection.list { - if s.UUID == uuid { - return s, nil - } + if s, ok := collection.cache[uuid]; ok { + return s, nil } - return nil, fmt.Errorf("snapshot with uuid %s not found", uuid) + + key := (&Snapshot{UUID: uuid}).Key() + + value, err := collection.db.Get(key) + if err == database.ErrNotFound { + return nil, fmt.Errorf("snapshot with uuid %s not found", uuid) + } + if err != nil { + return nil, err + } + + s := &Snapshot{} + err = s.Decode(value) + + if err == nil { + collection.cache[s.UUID] = s + } + + return s, err } // ByRemoteRepoSource looks up snapshots that have specified RemoteRepo as a source func (collection *SnapshotCollection) ByRemoteRepoSource(repo *RemoteRepo) []*Snapshot { - collection.loadList() - - var result []*Snapshot - - for _, s := range collection.list { - if s.SourceKind == SourceRemoteRepo && utils.StrSliceHasItem(s.SourceIDs, repo.UUID) { - result = append(result, s) - } - } - return result + return collection.search(func(s *Snapshot) bool { + return s.SourceKind == SourceRemoteRepo && utils.StrSliceHasItem(s.SourceIDs, repo.UUID) + }, false) } // ByLocalRepoSource looks up snapshots that have specified LocalRepo as a source func (collection *SnapshotCollection) ByLocalRepoSource(repo *LocalRepo) []*Snapshot { - collection.loadList() - - var result []*Snapshot - - for _, s := range collection.list { - if s.SourceKind == SourceLocalRepo && utils.StrSliceHasItem(s.SourceIDs, repo.UUID) { - result = append(result, s) - } - } - return result + return collection.search(func(s *Snapshot) bool { + return s.SourceKind == SourceLocalRepo && utils.StrSliceHasItem(s.SourceIDs, repo.UUID) + }, false) } // BySnapshotSource looks up snapshots that have specified snapshot as a source func (collection *SnapshotCollection) BySnapshotSource(snapshot *Snapshot) []*Snapshot { - collection.loadList() - - var result []*Snapshot - - for _, s := range collection.list { - if s.SourceKind == "snapshot" && utils.StrSliceHasItem(s.SourceIDs, snapshot.UUID) { - result = append(result, s) - } - } - return result + return collection.search(func(s *Snapshot) bool { + return s.SourceKind == "snapshot" && utils.StrSliceHasItem(s.SourceIDs, snapshot.UUID) + }, false) } // ForEach runs method for each snapshot @@ -326,15 +332,25 @@ func (collection *SnapshotCollection) ForEach(handler func(*Snapshot) error) err // ForEachSorted runs method for each snapshot following some sort order func (collection *SnapshotCollection) ForEachSorted(sortMethod string, handler func(*Snapshot) error) error { - collection.loadList() + blobs := collection.db.FetchByPrefix([]byte("S")) + list := make([]*Snapshot, 0, len(blobs)) - sorter, err := newSnapshotSorter(sortMethod, collection) + for _, blob := range blobs { + s := &Snapshot{} + if err := s.Decode(blob); err != nil { + log.Printf("Error decoding snapshot: %s\n", err) + } else { + list = append(list, s) + } + } + + sorter, err := newSnapshotSorter(sortMethod, list) if err != nil { return err } - for _, i := range sorter.list { - err = handler(collection.list[i]) + for _, s := range sorter.list { + err = handler(s) if err != nil { return err } @@ -346,30 +362,16 @@ func (collection *SnapshotCollection) ForEachSorted(sortMethod string, handler f // Len returns number of snapshots in collection // ForEach runs method for each snapshot func (collection *SnapshotCollection) Len() int { - collection.loadList() - - return len(collection.list) + return len(collection.db.KeysByPrefix([]byte("S"))) } // Drop removes snapshot from collection func (collection *SnapshotCollection) Drop(snapshot *Snapshot) error { - collection.loadList() - - snapshotPosition := -1 - - for i, s := range collection.list { - if s == snapshot { - snapshotPosition = i - break - } - } - - if snapshotPosition == -1 { + if _, err := collection.db.Get(snapshot.Key()); err == database.ErrNotFound { panic("snapshot not found!") } - collection.list[len(collection.list)-1], collection.list[snapshotPosition], collection.list = - nil, collection.list[len(collection.list)-1], collection.list[:len(collection.list)-1] + delete(collection.cache, snapshot.UUID) err := collection.db.Delete(snapshot.Key()) if err != nil { @@ -386,13 +388,12 @@ const ( ) type snapshotSorter struct { - list []int - collection *SnapshotCollection + list []*Snapshot sortMethod int } -func newSnapshotSorter(sortMethod string, collection *SnapshotCollection) (*snapshotSorter, error) { - s := &snapshotSorter{collection: collection} +func newSnapshotSorter(sortMethod string, list []*Snapshot) (*snapshotSorter, error) { + s := &snapshotSorter{list: list} switch sortMethod { case "time", "Time": @@ -403,11 +404,6 @@ func newSnapshotSorter(sortMethod string, collection *SnapshotCollection) (*snap return nil, fmt.Errorf("sorting method \"%s\" unknown", sortMethod) } - s.list = make([]int, len(collection.list)) - for i := range s.list { - s.list[i] = i - } - sort.Sort(s) return s, nil @@ -420,9 +416,9 @@ func (s *snapshotSorter) Swap(i, j int) { func (s *snapshotSorter) Less(i, j int) bool { switch s.sortMethod { case SortName: - return s.collection.list[s.list[i]].Name < s.collection.list[s.list[j]].Name + return s.list[i].Name < s.list[j].Name case SortTime: - return s.collection.list[s.list[i]].CreatedAt.Before(s.collection.list[s.list[j]].CreatedAt) + return s.list[i].CreatedAt.Before(s.list[j].CreatedAt) } panic("unknown sort method") } diff --git a/deb/snapshot_bench_test.go b/deb/snapshot_bench_test.go index 19997c2c..0240b9f8 100644 --- a/deb/snapshot_bench_test.go +++ b/deb/snapshot_bench_test.go @@ -36,3 +36,63 @@ func BenchmarkSnapshotCollectionForEach(b *testing.B) { }) } } + +func BenchmarkSnapshotCollectionByUUID(b *testing.B) { + const count = 1024 + + tmpDir := os.TempDir() + defer os.RemoveAll(tmpDir) + + db, _ := database.NewOpenDB(tmpDir) + defer db.Close() + + collection := NewSnapshotCollection(db) + + uuids := []string{} + for i := 0; i < count; i++ { + snapshot := NewSnapshotFromRefList(fmt.Sprintf("snapshot%d", i), nil, NewPackageRefList(), fmt.Sprintf("Snapshot number %d", i)) + if collection.Add(snapshot) != nil { + b.FailNow() + } + uuids = append(uuids, snapshot.UUID) + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + collection = NewSnapshotCollection(db) + + if _, err := collection.ByUUID(uuids[i%len(uuids)]); err != nil { + b.FailNow() + } + } +} + +func BenchmarkSnapshotCollectionByName(b *testing.B) { + const count = 1024 + + tmpDir := os.TempDir() + defer os.RemoveAll(tmpDir) + + db, _ := database.NewOpenDB(tmpDir) + defer db.Close() + + collection := NewSnapshotCollection(db) + + for i := 0; i < count; i++ { + snapshot := NewSnapshotFromRefList(fmt.Sprintf("snapshot%d", i), nil, NewPackageRefList(), fmt.Sprintf("Snapshot number %d", i)) + if collection.Add(snapshot) != nil { + b.FailNow() + } + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + collection = NewSnapshotCollection(db) + + if _, err := collection.ByName(fmt.Sprintf("snapshot%d", i%count)); err != nil { + b.FailNow() + } + } +} diff --git a/deb/snapshot_test.go b/deb/snapshot_test.go index 85bf14ca..0d1bd25b 100644 --- a/deb/snapshot_test.go +++ b/deb/snapshot_test.go @@ -2,6 +2,7 @@ package deb import ( "errors" + "sort" "github.com/aptly-dev/aptly/database" @@ -158,6 +159,10 @@ func (s *SnapshotCollectionSuite) TestAddByNameByUUID(c *C) { snapshot, err = collection.ByUUID(s.snapshot1.UUID) c.Assert(err, IsNil) c.Assert(snapshot.String(), Equals, s.snapshot1.String()) + + snapshot, err = collection.ByUUID(s.snapshot2.UUID) + c.Assert(err, IsNil) + c.Assert(snapshot.String(), Equals, s.snapshot2.String()) } func (s *SnapshotCollectionSuite) TestUpdateLoadComplete(c *C) { @@ -193,6 +198,23 @@ func (s *SnapshotCollectionSuite) TestForEachAndLen(c *C) { c.Assert(err, Equals, e) } +func (s *SnapshotCollectionSuite) TestForEachSorted(c *C) { + s.collection.Add(s.snapshot2) + s.collection.Add(s.snapshot1) + s.collection.Add(s.snapshot4) + s.collection.Add(s.snapshot3) + + names := []string{} + + err := s.collection.ForEachSorted("name", func(snapshot *Snapshot) error { + names = append(names, snapshot.Name) + return nil + }) + c.Assert(err, IsNil) + + c.Check(sort.StringsAreSorted(names), Equals, true) +} + func (s *SnapshotCollectionSuite) TestFindByRemoteRepoSource(c *C) { c.Assert(s.collection.Add(s.snapshot1), IsNil) c.Assert(s.collection.Add(s.snapshot2), IsNil) @@ -230,7 +252,11 @@ func (s *SnapshotCollectionSuite) TestFindSnapshotSource(c *C) { c.Assert(s.collection.Add(snapshot4), IsNil) c.Assert(s.collection.Add(snapshot5), IsNil) - c.Check(s.collection.BySnapshotSource(s.snapshot1), DeepEquals, []*Snapshot{snapshot3, snapshot4}) + list := s.collection.BySnapshotSource(s.snapshot1) + sorter, _ := newSnapshotSorter("name", list) + sort.Sort(sorter) + + c.Check(sorter.list, DeepEquals, []*Snapshot{snapshot3, snapshot4}) c.Check(s.collection.BySnapshotSource(s.snapshot2), DeepEquals, []*Snapshot{snapshot3}) c.Check(s.collection.BySnapshotSource(snapshot5), DeepEquals, []*Snapshot(nil)) } From c741cca5f27a2bd5b869febd4b17f2164600a32b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steven!=20Ragnar=C3=B6k?= Date: Thu, 13 Sep 2018 00:21:14 -0400 Subject: [PATCH 08/12] Use github.com/aptly-dev/aptly as the go package path. Without this change the first time setup instructions fail at the `make install` stage. --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 31dbcffd..72bc7878 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -86,8 +86,8 @@ to prepend it or to skip this test if you're security conscious. As Go is using repository path in import paths, it's better to clone aptly repo (not your fork) at default location: - mkdir -p ~/go/src/github.com/smira - cd ~/go/src/github.com/smira + mkdir -p ~/go/src/github.com/aptly-dev + cd ~/go/src/github.com/aptly-dev git clone git@github.com:aptly-dev/aptly.git cd aptly From e45f85cc1ed5238b5123cc0478f86ae44acdf270 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Fri, 14 Sep 2018 01:22:32 +0300 Subject: [PATCH 09/12] Replace to new docker container w aptly & nginx Old project not supported long time --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index c30f332d..5c45218d 100644 --- a/README.rst +++ b/README.rst @@ -90,7 +90,7 @@ Vagrant: Docker: - `Docker container `_ with aptly inside by Mike Purvis -- `Docker container `_ with aptly and nginx by Bryan Hong +- `Docker container `_ with aptly and nginx by Artem Smirnov With configuration management systems: From 814a0498df5771325b9826339f5a5bd8b57a0b8a Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Fri, 14 Sep 2018 01:24:21 +0300 Subject: [PATCH 10/12] Little syntax fix --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 5c45218d..724894f1 100644 --- a/README.rst +++ b/README.rst @@ -71,7 +71,7 @@ If you have Go environment set up, you can build aptly from source by running (g cd $GOPATH/src/github.com/aptly-dev/aptly make install -Binary would be installed to ```$GOPATH/bin/aptly``. +Binary would be installed to ``$GOPATH/bin/aptly``. Contributing ------------ From ea32d8627e3030bef70cf0397a3ada9a30cc491d Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Fri, 14 Sep 2018 01:29:11 +0300 Subject: [PATCH 11/12] Update AUTHORS --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 105599c8..00d6a403 100644 --- a/AUTHORS +++ b/AUTHORS @@ -33,3 +33,4 @@ List of contributors, in chronological order: * Petr Jediny (https://github.com/pjediny) * Maximilian Stein (https://github.com/steinymity) * Strajan Sebastian (https://github.com/strajansebastian) +* Artem Smirnov (https://github.com/urpylka) From fbafde6e27fc7b5adbedd08c692b9364570aefca Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Wed, 19 Sep 2018 01:23:17 +0300 Subject: [PATCH 12/12] Bump Go versions with Go 1.11 release --- .travis.yml | 6 +++--- README.rst | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index e11af1b4..9ce86b75 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,11 +24,11 @@ matrix: env: RUN_LONG_TESTS=no fast_finish: true include: - - go: 1.8.x - env: RUN_LONG_TESTS=no - go: 1.9.x - env: RUN_LONG_TESTS=yes + env: RUN_LONG_TESTS=no - go: 1.10.x + env: RUN_LONG_TESTS=yes + - go: 1.11.x env: - RUN_LONG_TESTS=yes - DEPLOY_BINARIES=yes diff --git a/README.rst b/README.rst index 724894f1..f7bfdf49 100644 --- a/README.rst +++ b/README.rst @@ -64,7 +64,7 @@ If you would like to use nightly builds (unstable), please use following reposit Binary executables (depends almost only on libc) are available for download from `Bintray `_. -If you have Go environment set up, you can build aptly from source by running (go 1.8+ required):: +If you have Go environment set up, you can build aptly from source by running (go 1.9+ required):: mkdir -p $GOPATH/src/github.com/aptly-dev/aptly git clone https://github.com/aptly-dev/aptly $GOPATH/src/github.com/aptly-dev/aptly