Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch setting role exception and output a warning instead of stop migration process #33

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Catch setting role exception and output a warning instead of stop migration process #33

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da827b4
catch setting role exception and output a warning instead of stop mig…
runwuf Apr 26, 2022
2759321
add -fr , --filtered-roles flag to filter roles not to be migrated
runwuf Apr 26, 2022
7a98a1c
filter roles feature improvements
runwuf Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 49 additions & 16 deletions aiven_db_migrate/migrate/pgmigrate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,13 @@ class PGCluster:
_pg_roles: Dict[str, PGRole]
_mangle: bool

def __init__(self, conn_info: Union[str, Dict[str, Any]], filtered_db: Optional[str] = None, mangle: bool = False):
def __init__(
self,
conn_info: Union[str, Dict[str, Any]],
filtered_db: Optional[str] = None,
filtered_roles: Tuple[str] = ((), ),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type for filtered_roles is wrong (tuple has fixed size, you need sequence or list or set) and the default argument type doesn't match.

mangle: bool = False
):
self.log = logging.getLogger(self.__class__.__name__)
self.conn_info = get_connection_info(conn_info)
self.conn_lock = threading.RLock()
Expand All @@ -125,6 +131,8 @@ def __init__(self, conn_info: Union[str, Dict[str, Any]], filtered_db: Optional[
self.filtered_db = filtered_db.split(",")
else:
self.filtered_db = []
self.filtered_roles = filtered_roles

if "application_name" not in self.conn_info:
self.conn_info["application_name"] = f"aiven-db-migrate/{__version__}"
self._mangle = mangle
Expand Down Expand Up @@ -318,11 +326,19 @@ def pg_lang(self) -> List[Dict[str, Any]]:

@property
def pg_roles(self) -> Dict[str, PGRole]:

if not self._pg_roles:
# exclude system roles
roles = self.c("SELECT quote_ident(rolname) as safe_rolname, * FROM pg_catalog.pg_roles WHERE oid > 16384")
roles = self.c(f"SELECT quote_ident(rolname) as safe_rolname, * FROM pg_catalog.pg_roles WHERE oid > 16384")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove f-string as it's not used anymore, please


for r in roles:
rolname = r["rolname"]

if rolname in self.filtered_roles:
self.log.debug(f'Skipping filtered role: [{r["rolname"]}]')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: loggers have their own formatting system, no need to use f-strings.

continue

self.log.debug(f'Migrating role: [{r["rolname"]}]')
# create semi-random placeholder password for role with login
rolpassword = (
"placeholder_{}".format("".join(random.choices(string.ascii_lowercase, k=16)))
Expand Down Expand Up @@ -738,15 +754,20 @@ def __init__(
verbose: bool = False,
mangle: bool = False,
filtered_db: Optional[str] = None,
filtered_roles: Tuple[str] = ((), ),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, typing issues

skip_tables: Optional[List[str]] = None,
with_tables: Optional[List[str]] = None,
replicate_extensions: bool = True,
):
if skip_tables and with_tables:
raise Exception("Can only specify a skip table list or a with table list")
self.log = logging.getLogger(self.__class__.__name__)
self.source = PGSource(conn_info=source_conn_info, filtered_db=filtered_db, mangle=mangle)
self.target = PGTarget(conn_info=target_conn_info, filtered_db=filtered_db, mangle=mangle)
self.source = PGSource(
conn_info=source_conn_info, filtered_db=filtered_db, filtered_roles=filtered_roles, mangle=mangle
)
self.target = PGTarget(
conn_info=target_conn_info, filtered_db=filtered_db, filtered_roles=filtered_roles, mangle=mangle
)
self.skip_tables = self._convert_table_names(skip_tables)
self.with_tables = self._convert_table_names(with_tables)
self.pgbin = Path()
Expand Down Expand Up @@ -992,18 +1013,23 @@ def _migrate_roles(self) -> Dict[str, PGRoleTask]:
message=err.diag.message_primary,
)
else:
if role.rolconfig:
for conf in role.rolconfig:
key, value = conf.split("=", 1)
self.log.info("Setting config for role %r: %s = %s", role.rolname, key, value)
self.target.c(f'ALTER ROLE {role.safe_rolname} SET "{key}" = %s', args=(value, ), return_rows=0)
roles[role.rolname] = PGRoleTask(
rolname=rolname,
rolpassword=role.rolpassword,
status=PGRoleStatus.created,
message="role created",
)

try:
if role.rolconfig:
for conf in role.rolconfig:
key, value = conf.split("=", 1)
self.log.info("Setting config for role %r: %s = %s", role.rolname, key, value)
self.target.c(f'ALTER ROLE {role.safe_rolname} SET "{key}" = %s', args=(value, ), return_rows=0)
runwuf marked this conversation as resolved.
Show resolved Hide resolved
roles[role.rolname] = PGRoleTask(
rolname=rolname,
rolpassword=role.rolpassword,
status=PGRoleStatus.created,
message="role created",
)
# display warning when ProgrammingErrorERROR 42501: InsufficientPrivilege: permission denied to set parameter for a role
except psycopg2.errors.InsufficientPrivilege:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you choose to go for the "filtered roles" route, why do you still need this change? Isn't this overlapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope to keep this change, with the filtered roles flag, user needs to know exactly what roles to filter before the migration and supply that to the cli argument, should there be any special setting with this role that is unable to be set with avnadmin's permission it would stop the migration as the exception is not caught. I think failing to set a role should not stop the migration process completely.

Copy link

@ghost ghost Apr 28, 2022 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is... it's quite a bold statement. We'd need to be sure that such role is not needed in the destination database. If we go on and then we discover after months that the migrated db doesn't work anymore, that's an issue. It's better to fail fast.

Again, this will impact everything: aiven_db_migrate is a library that could be used anywhere (and it's used in aiven-core as well, as a library), we're adding a very generic workaround for a very narrow use case.

This is an AWS-specific thing. Maybe an idea could be add a specific flag to the command line, something like --ignore-source-aws-roles, and you put AWS-specific roles there, and pass them to filtered roles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds like a good idea, I could just add rdstopmgr to the filtered_roles if --ignore-source-aws-roles is specified, do you think we should include all the rds* roles when the flag specified? and a similar flag such as --ignore-source-cloudsql-roles? one concern is if aws and google changes the names of these roles, probably not something we want to maintain in the migration tool code...

self.log.warning(
f'Setting [{role.rolname}]: [{key}] = [{value}] failed. psycopg2.errors.InsufficientPrivilege'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: logger formatting

)
return roles

@staticmethod
Expand Down Expand Up @@ -1263,6 +1289,9 @@ def main(args=None, *, prog="pg_migrate"):
parser.add_argument(
"-t", "--target", help="Target PostgreSQL server, either postgres:// uri or libpq connection string.", required=True
)
parser.add_argument(
"-fr", "--filtered-roles", help="Comma separated list of roles to filter out during migrations", required=False
)
parser.add_argument(
"-f", "--filtered-db", help="Comma separated list of databases to filter out during migrations", required=False
)
Expand Down Expand Up @@ -1343,6 +1372,9 @@ def main(args=None, *, prog="pg_migrate"):
else:
logging.basicConfig(level=logging.INFO, format=log_format)

if not args.filtered_roles:
args.filtered_roles = ()
Copy link

@ghost ghost Apr 28, 2022 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parser has already the support for default values. but this can't work. should be an empty string as default value, I think.

then, down there, you could do something like

filtered_roles=[ role for role in args.filtered_roles.split(",") if role != "" ] (untested code)

so that the filtered_roles sequence is actually empty when passing it to the main func.


pg_mig = PGMigrate(
source_conn_info=args.source,
target_conn_info=args.target,
Expand All @@ -1351,6 +1383,7 @@ def main(args=None, *, prog="pg_migrate"):
stop_replication=args.stop_replication,
verbose=args.verbose,
filtered_db=args.filtered_db,
filtered_roles=args.filtered_roles,
mangle=args.mangle,
skip_tables=args.skip_table,
with_tables=args.with_table,
Expand Down