feat: Add etcd database support #1023

hudeng-go · 2022-02-08T05:16:03Z

improve concurrent access and high availability of aptly with the help of the characteristics of etcd

Fixes #

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

Checklist

unit-test added (if change is algorithm)
functional test added/updated (if change is functional)
man page updated (if applicable)
bash completion updated (if applicable)
documentation updated
author name in AUTHORS

hudeng-go · 2022-02-08T05:17:19Z

@lbolla Resolve system test errors at #1022

lbolla · 2022-02-08T15:41:46Z

@hudeng-go Looking good to me. What do you think of this comment on the other ticket? #1022 (comment)

lbolla · 2022-02-09T15:31:51Z

system/run-etcd.py

@@ -0,0 +1,175 @@
+#!/usr/bin/env python


This script is almost identical to system/run.py:

/tmp/aptly etcd-dev ❯ diff system/run.py system/run-etcd.py 97a98,100 > t.configOverride = { > "databaseEtcd": "127.0.0.1:2379" > }

It would be much better to have a flag or an env variable to tell the test runner script to use etcd, rather than duplicate the whole script.

Moreover, I suspect that using configOverride may not work on all cases. Many unittests set their own overrides, so either they will be overwritten with the databaseEtcd key or viceversa.

Maybe databaseEtcd could come from the environment, e.g. APTLY_ETCD_DATABASE and set in lib.py:BaseTest.config dictionary?

lbolla · 2022-02-10T09:22:52Z

system/lib.py

@@ -142,6 +142,7 @@ class BaseTest(object):
        "gpgDisableSign": False,
        "ppaDistributorID": "ubuntu",
        "ppaCodename": "",
+        "databaseEtcd": "%s" % os.environ["APTLY_ETCD_DATABASE"],


This will throw a KeyError if the env var is not set, which it isn't in the normal system-tests. Maybe use os.environ.get("APTLY_ETCD_DATABASE") or "" to make sure we have a valid value in the config?

lbolla · 2022-02-10T12:02:36Z

context/context.go

@@ -287,7 +288,12 @@ func (context *AptlyContext) _database() (database.Storage, error) {
 	if context.database == nil {
 		var err error

-		context.database, err = goleveldb.NewDB(context.dbPath())
+		if context.config().DatabaseEtcd != "" {


I wonder if we should have a more explicit way to pick the database, rather than look at DatabaseEtcd.

Maybe having a new config like databaseBackend to be one of: "leveldb", "etcd", etc.? What do you think?

lbolla · 2022-02-10T12:51:07Z

@hudeng-go Tests failed... https://github.com/aptly-dev/aptly/runs/5140408342?check_suite_focus=true
They also log a lot and take quite a long time, which will eat our free allowance of GH actions... Can you think of a solution?

hudeng-go · 2022-02-11T01:01:45Z

@hudeng-go Tests failed... https://github.com/aptly-dev/aptly/runs/5140408342?check_suite_focus=true They also log a lot and take quite a long time, which will eat our free allowance of GH actions... Can you think of a solution?

Sorry, it took a long time to link etcd when migrating data. We are looking for the reason.

lbolla · 2022-02-11T08:40:49Z

@hudeng-go I had to stop the CI once again, as it was stuck... Can we make the script so that it would crash at the first error, so we don't continue a clearly failing test?

hudeng-go · 2022-02-11T08:47:42Z

@hudeng-go I had to stop the CI once again, as it was stuck... Can we make the script so that it would crash at the first error, so we don't continue a clearly failing test?

I have modified the migration script to exit with an error. Sorry again.

hudeng-go · 2022-02-11T10:18:08Z

@hudeng-go I had to stop the CI once again, as it was stuck... Can we make the script so that it would crash at the first error, so we don't continue a clearly failing test?

In my environment, data can be migrated to normally. I don't know why I can't connect to etcd database in GH action environment.

Makefile

lbolla · 2022-02-11T11:36:34Z

context/context.go

+		if context.config().DatabaseBackend.Type == "etcd" {
+			context.database, err = etcddb.NewDB(context.config().DatabaseBackend.Url)
+		} else {
+			context.database, err = goleveldb.NewDB(context.dbPath())


I see the reason for using DatabaseBackend.Type as recommended. I wonder though if we can adapt dbPath (i.e. rootPath/db) into your new DbConfig structure? We must be backward compatible though. Maybe it's worth raising a separate ticket once this is merged.

I don't quite understand. At present, this dbpath should be available only for leveldb, and the current processing method should be compatible with it. My idea is that if the dbconfig configuration in the configuration file is empty, the default leveldb is used as the back-end, and leveldb compatibility should not be required in dbconfig. This is also explained in the man document.

Yeah, if we want to be backward compatible that's fine. I was considering the opportunity to fit the current dbPath behavior in your struct, too, to be "future-proof" (so in the future we can deprecate dbPath and just stay with your struct implementation). Food for though.

lbolla · 2022-02-12T19:11:08Z

@hudeng-go I had to stop the CI once again, as it was stuck... Can we make the script so that it would crash at the first error, so we don't continue a clearly failing test?

Once #1033 is merged, you can rebase to ensure we have stricter timeouts for CI jobs, and avoid tests wasting our GHA allowance.

lbolla · 2022-02-13T20:09:24Z

@hudeng-go I had to stop the CI once again, as it was stuck... Can we make the script so that it would crash at the first error, so we don't continue a clearly failing test?

Once #1033 is merged, you can rebase to ensure we have stricter timeouts for CI jobs, and avoid tests wasting our GHA allowance.

#1033 is merged. Please rebase this branch before carrying out further tests. Thanks!

lbolla · 2022-02-14T13:10:36Z

@hudeng-go On my humble LXD image, which I use for development, etcd is using quite a lot of resources. Maybe it crashes on CI because it's too memory/cpu hungry?

improve concurrent access and high availability of aptly with the help of the characteristics of etcd

databaseBackend config contains type and url sub config, It can facilitate the expansion of other types of databases in the future.

tomsiewert · 2022-10-20T09:28:18Z

Is there any update to bring this patch into upstream?

tonobo · 2023-04-19T12:26:35Z

Howdy, any chance to get this back on track?

tomsiewert · 2023-12-01T13:21:56Z

@randombenj As you seem to be the most active reviewer/contributor for Aptly, could you please review this?

neolynx · 2023-12-01T13:53:31Z

Hi all,

maybe we should rebase this on master first, which seems to be a bit of work.. any chance someone could help with this ?

neolynx · 2024-06-01T21:32:18Z

Hi all,

could aomeone have a look at why the test is running into a timeout in #1284 ?

@tomsiewert @tonobo @hudeng-go ?

hudeng-go force-pushed the etcd-dev branch from d9fdec6 to 5d82b85 Compare February 9, 2022 10:22

lbolla suggested changes Feb 9, 2022

View reviewed changes

hudeng-go force-pushed the etcd-dev branch 2 times, most recently from b7b80fd to 30c5016 Compare February 10, 2022 06:57

lbolla reviewed Feb 10, 2022

View reviewed changes

hudeng-go force-pushed the etcd-dev branch 2 times, most recently from bab0475 to c3a4869 Compare February 10, 2022 09:40

hudeng-go requested a review from lbolla February 10, 2022 10:15

lbolla reviewed Feb 10, 2022

View reviewed changes

hudeng-go force-pushed the etcd-dev branch from c3a4869 to 30fc1ab Compare February 11, 2022 06:49

lbolla mentioned this pull request Feb 11, 2022

Consider adding timeouts to CI jobs to avoid runaway tests #1032

Closed

hudeng-go force-pushed the etcd-dev branch from 30fc1ab to 31fcfe1 Compare February 11, 2022 08:45

hudeng-go force-pushed the etcd-dev branch from 31fcfe1 to 9b875f5 Compare February 11, 2022 09:42

lbolla reviewed Feb 11, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

lbolla reviewed Feb 11, 2022

View reviewed changes

hudeng-go force-pushed the etcd-dev branch 2 times, most recently from ae5456d to 1122a66 Compare February 18, 2022 02:42

hudeng-go added 3 commits February 21, 2022 10:08

feat: Add etcd database support

1da74de

improve concurrent access and high availability of aptly with the help of the characteristics of etcd

feat: Add system test for etcd

d03d232

feat: Use databaseBackend config repace databaseEtcd

23a7313

databaseBackend config contains type and url sub config, It can facilitate the expansion of other types of databases in the future.

hudeng-go force-pushed the etcd-dev branch from 1122a66 to 23a7313 Compare February 21, 2022 02:08

r4co0n mentioned this pull request Mar 12, 2022

Aptly in build pipeline #996

Open

neolynx self-assigned this Jan 14, 2024

neolynx mentioned this pull request Apr 21, 2024

feat: Add etcd database support #1284

Open

6 tasks

neolynx added the PR superseded label Apr 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add etcd database support #1023

feat: Add etcd database support #1023

hudeng-go commented Feb 8, 2022 •

edited

hudeng-go commented Feb 8, 2022

lbolla commented Feb 8, 2022

lbolla Feb 9, 2022

lbolla Feb 10, 2022

lbolla Feb 10, 2022

lbolla commented Feb 10, 2022

hudeng-go commented Feb 11, 2022

lbolla commented Feb 11, 2022

hudeng-go commented Feb 11, 2022

hudeng-go commented Feb 11, 2022

lbolla Feb 11, 2022

hudeng-go Feb 17, 2022

lbolla Feb 17, 2022

lbolla commented Feb 12, 2022

lbolla commented Feb 13, 2022

lbolla commented Feb 14, 2022

tomsiewert commented Oct 20, 2022

tonobo commented Apr 19, 2023

tomsiewert commented Dec 1, 2023

neolynx commented Dec 1, 2023

neolynx commented Jun 1, 2024 •

edited

feat: Add etcd database support #1023

Are you sure you want to change the base?

feat: Add etcd database support #1023

Conversation

hudeng-go commented Feb 8, 2022 • edited

Requirements

Description of the Change

Checklist

hudeng-go commented Feb 8, 2022

lbolla commented Feb 8, 2022

lbolla Feb 9, 2022

Choose a reason for hiding this comment

lbolla Feb 10, 2022

Choose a reason for hiding this comment

lbolla Feb 10, 2022

Choose a reason for hiding this comment

lbolla commented Feb 10, 2022

hudeng-go commented Feb 11, 2022

lbolla commented Feb 11, 2022

hudeng-go commented Feb 11, 2022

hudeng-go commented Feb 11, 2022

lbolla Feb 11, 2022

Choose a reason for hiding this comment

hudeng-go Feb 17, 2022

Choose a reason for hiding this comment

lbolla Feb 17, 2022

Choose a reason for hiding this comment

lbolla commented Feb 12, 2022

lbolla commented Feb 13, 2022

lbolla commented Feb 14, 2022

tomsiewert commented Oct 20, 2022

tonobo commented Apr 19, 2023

tomsiewert commented Dec 1, 2023

neolynx commented Dec 1, 2023

neolynx commented Jun 1, 2024 • edited

hudeng-go commented Feb 8, 2022 •

edited

neolynx commented Jun 1, 2024 •

edited