[pve-devel] [PATCH access-control/wt/manager v2] add realm sync jobs

[pve-devel] [PATCH access-control/wt/manager v2] add realm sync jobs

[Dominik Csapak]

with this, users now can schedule realm sync jobs, instead of manually
pressing 'sync' or configuring a cronjob for 'pveum realm sync'

the access-control patch needs special care, since i try to sync
independent pve-scheduler calls across the cluster. in my tests here
it worked, but that does not mean i didn't overlook some things.

pve-manager depends on the new access-control and widget-toolkit

i tried to find a way to not add two levels of directories there,
while keeping the suggested 'PVE::API2::AccessControl::Job::RealmSync'
path, but the only other method i saw was to put all the code into
'Job.pm' which felt weird. So we introduce two levels of dirs
to properly split the perl packages.

the first patch of access-control and manager could be applied without
the rest, it clears up the confusion for using the checkboxes
and the realm default

the result is that now the user always gets what he sees when using
the sync window or realm sync job edit and never implicitly falling
back to the realm defaults. If we want that, i can do that and
send a v3 (basically i'd have to add a 'use default' checkbox
for the remove-vanished option)

since PVE::Jobs::Registry lives in pve-common now, there is no
cyclic dependency between manager/access control anymore

changes from v1:
* include thomas suggestions
* add patches to allow 'none' for remove-vanished
* add patch for wt that allows filtering for the realm combobox
* load the default values (if any) from the realm on sync job create,
  but not on edit

pve-access-control:

Dominik Csapak (2):
  realm: sync: allow 'none' for 'remove-vanished' option
  add realmsync plugin for jobs and CRUD api for domainsync-jobs

 src/PVE/API2/AccessControl.pm               |   6 +
 src/PVE/API2/AccessControl/Job.pm           |  47 +++
 src/PVE/API2/AccessControl/Job/Makefile     |   6 +
 src/PVE/API2/AccessControl/Job/RealmSync.pm | 324 ++++++++++++++++++++
 src/PVE/API2/AccessControl/Makefile         |   9 +
 src/PVE/API2/Makefile                       |   4 +
 src/PVE/Auth/Plugin.pm                      |   8 +-
 src/PVE/Jobs/Makefile                       |   6 +
 src/PVE/Jobs/RealmSync.pm                   | 193 ++++++++++++
 src/PVE/Makefile                            |   1 +
 10 files changed, 601 insertions(+), 3 deletions(-)
 create mode 100644 src/PVE/API2/AccessControl/Job.pm
 create mode 100644 src/PVE/API2/AccessControl/Job/Makefile
 create mode 100644 src/PVE/API2/AccessControl/Job/RealmSync.pm
 create mode 100644 src/PVE/API2/AccessControl/Makefile
 create mode 100644 src/PVE/Jobs/Makefile
 create mode 100644 src/PVE/Jobs/RealmSync.pm

proxmox-widget-toolkit:

Dominik Csapak (1):
  RealmComboBox: add custom store filters for callers

 src/form/RealmComboBox.js | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

pve-manager:

Dominik Csapak (3):
  ui: realm: sync: don't use realm defaults for remove-vanished
  Jobs: add RealmSync Plugin and register it
  ui: add Realm Sync panel

 PVE/Jobs.pm                     |   2 +
 www/manager6/Makefile           |   1 +
 www/manager6/dc/Config.js       |   7 +
 www/manager6/dc/RealmSyncJob.js | 380 ++++++++++++++++++++++++++++++++
 www/manager6/dc/SyncWindow.js   |   2 +
 5 files changed, 392 insertions(+)
 create mode 100644 www/manager6/dc/RealmSyncJob.js

-- 
2.30.2

[pve-devel] [PATCH access-control v2 1/2] realm: sync: allow 'none' for 'remove-vanished' option

[Dominik Csapak]

with that, the api call can now override the default option
that is set on the realm (if any) by providing 'none'

it was not possible previously to override the realm default
when one wanted no properties to delete

no other code changes are necessary since we only extract the
known values 'acl' etc. and 'none' has no meaning there

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Auth/Plugin.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 03d3342..bae9fb9 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -63,10 +63,12 @@ my $realm_sync_options_desc = {
 	    ." vanishes during a sync. The following values are possible: 'entry' removes the"
 	    ." user/group when not returned from the sync. 'properties' removes the set"
 	    ." properties on existing user/group that do not appear in the source (even custom ones)."
-	    ." 'acl' removes acls when the user/group is not returned from the sync.",
+	    ." 'acl' removes acls when the user/group is not returned from the sync."
+	    ." Instead of a list it also can be 'none' (the default).",
 	type => 'string',
-	typetext => "[acl];[properties];[entry]",
-	pattern => "(?:$remove_options\;)*$remove_options",
+	default => 'none',
+	typetext => "([acl];[properties];[entry])|none",
+	pattern => "(?:(?:$remove_options\;)*$remove_options)|none",
 	optional => '1',
     },
     # TODO check/rewrite in pve7to8, and remove with 8.0
-- 
2.30.2

[pve-devel] applied: [PATCH access-control v2 1/2] realm: sync: allow 'none' for 'remove-vanished' option

[Thomas Lamprecht]

Am 06/12/2022 um 12:06 schrieb Dominik Csapak:
> with that, the api call can now override the default option
> that is set on the realm (if any) by providing 'none'
> 
> it was not possible previously to override the realm default
> when one wanted no properties to delete
> 
> no other code changes are necessary since we only extract the
> known values 'acl' etc. and 'none' has no meaning there
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Auth/Plugin.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH access-control v2 2/2] add realmsync plugin for jobs and CRUD api for domainsync-jobs

[Dominik Csapak]

to be able to define automated jobs that sync ldap/ad

The jobs plugin contains special handling when no node is given, since
we only want it to run on a single node when that triggers. For that,
we save a statefile in /etc/pve/priv/jobs/ which contains the
node/time/upid of the node that runs the job. The first node that
is able to lock the realm (via cfs_lock_domain) "wins" and may
sync from the ldap.

in case a specific node was selected, this is omitted and the Jobs
handling will not let it run on other nodes anyway

the API part is our usual sectionconfig CRUD api, but specialized
for the specific type of job.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/API2/AccessControl.pm               |   6 +
 src/PVE/API2/AccessControl/Job.pm           |  47 +++
 src/PVE/API2/AccessControl/Job/Makefile     |   6 +
 src/PVE/API2/AccessControl/Job/RealmSync.pm | 324 ++++++++++++++++++++
 src/PVE/API2/AccessControl/Makefile         |   9 +
 src/PVE/API2/Makefile                       |   4 +
 src/PVE/Jobs/Makefile                       |   6 +
 src/PVE/Jobs/RealmSync.pm                   | 193 ++++++++++++
 src/PVE/Makefile                            |   1 +
 9 files changed, 596 insertions(+)
 create mode 100644 src/PVE/API2/AccessControl/Job.pm
 create mode 100644 src/PVE/API2/AccessControl/Job/Makefile
 create mode 100644 src/PVE/API2/AccessControl/Job/RealmSync.pm
 create mode 100644 src/PVE/API2/AccessControl/Makefile
 create mode 100644 src/PVE/Jobs/Makefile
 create mode 100644 src/PVE/Jobs/RealmSync.pm

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 104e7e3..221ecb2 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -14,6 +14,7 @@ use PVE::DataCenterConfig;
 use PVE::RESTHandler;
 use PVE::AccessControl;
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::API2::AccessControl::Job;
 use PVE::API2::Domains;
 use PVE::API2::User;
 use PVE::API2::Group;
@@ -57,6 +58,11 @@ __PACKAGE__->register_method ({
     path => 'domains',
 });
 
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::AccessControl::Job",
+    path => 'jobs',
+});
+
 __PACKAGE__->register_method ({
     subclass => "PVE::API2::OpenId",
     path => 'openid',
diff --git a/src/PVE/API2/AccessControl/Job.pm b/src/PVE/API2/AccessControl/Job.pm
new file mode 100644
index 0000000..cf2d0a9
--- /dev/null
+++ b/src/PVE/API2/AccessControl/Job.pm
@@ -0,0 +1,47 @@
+package PVE::API2::AccessControl::Job;
+
+use strict;
+use warnings;
+
+use PVE::RESTHandler;
+
+use PVE::API2::AccessControl::Job::RealmSync;
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::AccessControl::Job::RealmSync",
+    path => 'realm-sync',
+});
+
+__PACKAGE__->register_method ({
+    name => 'index',
+    path => '',
+    method => 'GET',
+    description => "Directory index.",
+    permissions => {
+	user => 'all',
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => "object",
+	    properties => {
+		subdir => { type => 'string' },
+	    },
+	},
+	links => [ { rel => 'child', href => "{subdir}" } ],
+    },
+    code => sub {
+	my ($param) = @_;
+
+	return {
+	    subdir => 'realm-sync',
+	};
+    }});
+
+1;
diff --git a/src/PVE/API2/AccessControl/Job/Makefile b/src/PVE/API2/AccessControl/Job/Makefile
new file mode 100644
index 0000000..5feae4b
--- /dev/null
+++ b/src/PVE/API2/AccessControl/Job/Makefile
@@ -0,0 +1,6 @@
+API2_SOURCES= 		\
+	RealmSync.pm	\
+
+.PHONY: install
+install:
+	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/AccessControl/Job/$$i; done
diff --git a/src/PVE/API2/AccessControl/Job/RealmSync.pm b/src/PVE/API2/AccessControl/Job/RealmSync.pm
new file mode 100644
index 0000000..f14bb2b
--- /dev/null
+++ b/src/PVE/API2/AccessControl/Job/RealmSync.pm
@@ -0,0 +1,324 @@
+package PVE::API2::AccessControl::Job::RealmSync;
+
+use strict;
+use warnings;
+
+use JSON;
+
+use PVE::Cluster qw (cfs_read_file cfs_write_file cfs_lock_file);
+use PVE::Exception qw(raise_param_exc);
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::Job::Registry;
+use PVE::RESTHandler;
+use PVE::SafeSyslog;
+use PVE::Tools qw(extract_param);
+
+use PVE::AccessControl;
+use PVE::Auth::Plugin;
+use PVE::Jobs::RealmSync;
+
+
+use base qw(PVE::RESTHandler);
+
+my $get_cluster_last_run = sub {
+    my ($jobid) = @_;
+
+    my $fn = "/etc/pve/priv/jobs/realmsync-$jobid.json";
+    my $raw = PVE::Tools::file_get_contents($fn);
+    my $state = eval { decode_json($raw) };
+    die "json decode error on $fn: $@\n" if $@;
+
+    if (my $upid = $state->{upid}) {
+	if (my $decoded = PVE::Tools::upid_decode($upid)) {
+	    return $decoded->{starttime};
+	}
+    } else {
+	return $state->{time};
+    }
+
+    return undef;
+};
+
+__PACKAGE__->register_method ({
+    name => 'syncjob_index',
+    path => '',
+    method => 'GET',
+    description => "List configured realm-sync-jobs.",
+    permissions => {
+	check => ['perm', '/', ['Sys.Audit']],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => "object",
+	    properties => {
+		id => {
+		    description => "The ID of the entry.",
+		    type => 'string'
+		},
+		enabled => {
+		    description => "If the job is enalbed or not.",
+		    type => 'boolean',
+		},
+		comment => {
+		    description => "A comment for the job.",
+		    type => 'string',
+		    optional => 1,
+		},
+		schedule => {
+		    description => "The configured sync schedule.",
+		    type => 'string',
+		    optional => 1,
+		},
+		realm => {
+		    description => "The realm to sync.",
+		    type => 'string',
+		    optional => 1,
+		},
+		node => {
+		    description => "The node to run the job on, if any.",
+		    type => 'string',
+		    optional => 1,
+		},
+		scope => {
+		    description => "The Selection of what to sync.",
+		    type => 'string',
+		    optional => 1,
+		},
+		'remove-vanished' => {
+		    description => "A list of things to remove when the entity vanished during a sync.",
+		    type => 'string',
+		    optional => 1,
+		},
+		'last-run' => {
+		    description => "UNIX epoch when the job last ran.",
+		    type => 'integer',
+		    optional => 1,
+		},
+		'next-run' => {
+		    description => "UNIX epoch when the job is planned to run next.",
+		    type => 'integer',
+		    optional => 1,
+		},
+	    },
+	},
+	links => [ { rel => 'child', href => "{id}" } ],
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $jobs_data = cfs_read_file('jobs.cfg');
+	my $order = $jobs_data->{order};
+	my $jobs = $jobs_data->{ids};
+
+	my $res = [];
+	for my $jobid (sort { $order->{$a} <=> $order->{$b} } keys %$jobs) {
+	    my $job = $jobs->{$jobid};
+	    next if $job->{type} ne 'realmsync';
+
+	    $job->{id} = $jobid;
+	    if (my $schedule = $job->{schedule}) {
+		$job->{'last-run'} = eval { $get_cluster_last_run->($jobid) };
+		my $last_run = $job->{'last-run'} // time(); # current time as fallback
+		my $calendar_event = Proxmox::RS::CalendarEvent->new($schedule);
+		my $next_run = $calendar_event->compute_next_event($last_run);
+		$job->{'next-run'} = $next_run if defined($next_run);
+	    }
+
+	    push @$res, $job;
+	}
+
+	return $res;
+    }});
+
+__PACKAGE__->register_method({
+    name => 'read_job',
+    path => '{id}',
+    method => 'GET',
+    description => "Read realm-sync job definition.",
+    permissions => {
+	check => ['perm', '/', ['Sys.Audit']],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    id => {
+		type => 'string',
+		format => 'pve-configid',
+	    },
+	},
+    },
+    returns => {
+	type => 'object',
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $jobs = cfs_read_file('jobs.cfg');
+	my $id = $param->{id};
+	my $job = $jobs->{ids}->{$id};
+	return $job if $job && $job->{type} eq 'realmsync';
+
+	raise_param_exc({ id => "No such job '$id'" });
+
+    }});
+
+__PACKAGE__->register_method({
+    name => 'create_job',
+    path => '{id}',
+    method => 'POST',
+    protected => 1,
+    description => "Create new realm-sync job.",
+    permissions => {
+	description => "'Realm.AllocateUser' on '/access/realm/<realm>' and "
+	    ."'User.Modify' permissions to '/access/groups/'.",
+	check => [ 'and',
+	    ['perm', '/access/realm/{realm}', ['Realm.AllocateUser']],
+	    ['perm', '/access/groups', ['User.Modify']],
+	],
+    },
+    parameters => PVE::Jobs::RealmSync->createSchema(),
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $id = extract_param($param, 'id');
+
+	cfs_lock_file('jobs.cfg', undef, sub {
+	    my $data = cfs_read_file('jobs.cfg');
+
+	    die "Job '$id' already exists\n"
+		if $data->{ids}->{$id};
+
+	    my $plugin = PVE::Job::Registry->lookup('realmsync');
+	    my $opts = $plugin->check_config($id, $param, 1, 1);
+
+	    my $realm = $opts->{realm};
+	    my $cfg = cfs_read_file('domains.cfg');
+
+	    raise_param_exc({ realm => "No such realm '$realm'" })
+		if !defined($cfg->{ids}->{$realm});
+
+	    my $realm_type = $cfg->{ids}->{$realm}->{type};
+	    raise_param_exc({ realm => "Only LDAP/AD realms can be synced." })
+		if $realm_type ne 'ldap' && $realm_type ne 'ad';
+
+	    $data->{ids}->{$id} = $opts;
+
+	    PVE::Jobs::create_job($id, 'realmsync');
+
+	    cfs_write_file('jobs.cfg', $data);
+	});
+	die "$@" if ($@);
+
+	return undef;
+    }});
+
+__PACKAGE__->register_method({
+    name => 'update_job',
+    path => '{id}',
+    method => 'PUT',
+    protected => 1,
+    description => "Update realm-sync job definition.",
+    permissions => {
+	description => "'Realm.AllocateUser' on '/access/realm/<realm>' and "
+	    ." 'User.Modify' permissions to '/access/groups/'.",
+	check => [ 'and',
+	    ['perm', '/access/realm/{realm}', ['Realm.AllocateUser']],
+	    ['perm', '/access/groups', ['User.Modify']],
+	],
+    },
+    parameters => PVE::Jobs::RealmSync->updateSchema(),
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $id = extract_param($param, 'id');
+	my $delete = extract_param($param, 'delete');
+	if ($delete) {
+	    $delete = [PVE::Tools::split_list($delete)];
+	}
+
+	cfs_lock_file('jobs.cfg', undef, sub {
+	    my $jobs = cfs_read_file('jobs.cfg');
+
+	    die "no options specified\n" if !scalar(keys %$param);
+
+	    my $plugin = PVE::Job::Registry->lookup('realmsync');
+	    my $opts = $plugin->check_config($id, $param, 0, 1);
+
+	    my $job = $jobs->{ids}->{$id};
+	    die "no such realmsync job\n" if !$job || $job->{type} ne 'realmsync';
+
+	    my $options = $plugin->options();
+	    for my $k (@$delete) {
+		my $d = $options->{$k} || die "no such option '$k'\n";
+		die "unable to delete required option '$k'\n" if !$d->{optional};
+		die "unable to delete fixed option '$k'\n" if $d->{fixed};
+		die "cannot set and delete property '$k' at the same time!\n"
+			if defined($opts->{$k});
+		delete $job->{$k};
+	    }
+
+	    $job->{$_} = $param->{$_} for keys $param->%*;
+
+	    cfs_write_file('jobs.cfg', $jobs);
+
+	    PVE::Jobs::detect_changed_runtime_props($id, 'realmsync', $job);
+
+	    return;
+	});
+	die "$@" if ($@);
+    }});
+
+
+__PACKAGE__->register_method({
+    name => 'delete_job',
+    path => '{id}',
+    method => 'DELETE',
+    description => "Delete realm-sync job definition.",
+    permissions => {
+	check => ['perm', '/', ['Sys.Modify']],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    id => {
+		type => 'string',
+		format => 'pve-configid',
+	    },
+	},
+    },
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $id = $param->{id};
+
+	cfs_lock_file('jobs.cfg', undef, sub {
+	    my $jobs = cfs_read_file('jobs.cfg');
+
+	    if (!defined($jobs->{ids}->{$id}) || $jobs->{ids}->{$id}->{type} ne 'realmsync') {
+		raise_param_exc({ id => "No such job '$id'" });
+	    }
+	    delete $jobs->{ids}->{$id};
+
+	    PVE::Jobs::remove_job($id, 'realmsync');
+
+	    cfs_write_file('jobs.cfg', $jobs);
+	    unlink "/etc/pve/priv/jobs/realmsync-$id.json";
+	});
+	die "$@" if $@;
+
+	return undef;
+    }});
+
+1;
diff --git a/src/PVE/API2/AccessControl/Makefile b/src/PVE/API2/AccessControl/Makefile
new file mode 100644
index 0000000..8d3fb58
--- /dev/null
+++ b/src/PVE/API2/AccessControl/Makefile
@@ -0,0 +1,9 @@
+API2_SOURCES= 	\
+	Job.pm	\
+
+SUBDIRS=Job
+
+.PHONY: install
+install:
+	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/AccessControl/$$i; done
+	set -e && for i in ${SUBDIRS}; do ${MAKE} -C $$i $@; done
diff --git a/src/PVE/API2/Makefile b/src/PVE/API2/Makefile
index 2817f48..7d2be93 100644
--- a/src/PVE/API2/Makefile
+++ b/src/PVE/API2/Makefile
@@ -2,6 +2,7 @@
 API2_SOURCES= 		 	\
 	AccessControl.pm 	\
 	Domains.pm	 	\
+	RealmSync.pm	 	\
 	ACL.pm		 	\
 	Role.pm		 	\
 	Group.pm	 	\
@@ -9,6 +10,9 @@ API2_SOURCES= 		 	\
 	TFA.pm			\
 	OpenId.pm
 
+SUBDIRS=AccessControl
+
 .PHONY: install
 install:
 	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/$$i; done
+	set -e && for i in ${SUBDIRS}; do ${MAKE} -C $$i $@; done
diff --git a/src/PVE/Jobs/Makefile b/src/PVE/Jobs/Makefile
new file mode 100644
index 0000000..9eed1b2
--- /dev/null
+++ b/src/PVE/Jobs/Makefile
@@ -0,0 +1,6 @@
+SOURCES=RealmSync.pm
+
+.PHONY: install
+install: ${SOURCES}
+	install -d -m 0755 ${DESTDIR}${PERLDIR}/PVE/Jobs
+	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Jobs/$$i; done
diff --git a/src/PVE/Jobs/RealmSync.pm b/src/PVE/Jobs/RealmSync.pm
new file mode 100644
index 0000000..e8a86e3
--- /dev/null
+++ b/src/PVE/Jobs/RealmSync.pm
@@ -0,0 +1,193 @@
+package PVE::Jobs::RealmSync;
+
+use strict;
+use warnings;
+
+use JSON;
+
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::Cluster;
+use PVE::CalendarEvent;
+use PVE::Tools;
+
+use PVE::API2::Domains;
+
+use base qw(PVE::Job::Registry);
+
+sub type {
+    return 'realmsync';
+}
+
+my $props = get_standard_option('realm-sync-options', {
+    realm => get_standard_option('realm'),
+});
+
+sub properties {
+    return $props;
+}
+
+sub options {
+    my $options = {
+	enabled => { optional => 1 },
+	schedule => {},
+	comment => { optional => 1 },
+	node => { optional => 1 },
+	scope => {},
+    };
+    for my $opt (keys %$props) {
+	next if defined($options->{$opt});
+	if ($props->{$opt}->{optional}) {
+	    $options->{$opt} = { optional => 1 };
+	} else {
+	    $options->{$opt} = {};
+	}
+    }
+    $options->{realm}->{fixed} = 1;
+
+    return $options;
+}
+
+sub decode_value {
+    my ($class, $type, $key, $value) = @_;
+    return $value;
+}
+
+sub encode_value {
+    my ($class, $type, $key, $value) = @_;
+    return $value;
+}
+
+sub createSchema {
+    my ($class, $skip_type) = @_;
+    my $schema = $class->SUPER::createSchema($skip_type);
+
+    my $opts = $class->options();
+    for my $opt (keys $schema->{properties}->%*) {
+	next if defined($opts->{$opt}) || $opt eq 'id';
+	delete $schema->{properties}->{$opt};
+    }
+
+    # remove legacy props
+    delete $schema->{properties}->{full};
+    delete $schema->{properties}->{purge};
+
+    return $schema;
+}
+
+sub updateSchema {
+    my ($class, $skip_type) = @_;
+    my $schema = $class->SUPER::updateSchema($skip_type);
+
+    my $opts = $class->options();
+    for my $opt (keys $schema->{properties}->%*) {
+	next if defined($opts->{$opt});
+	next if $opt eq 'id' || $opt eq 'delete';
+	delete $schema->{properties}->{$opt};
+    }
+
+    # remove legacy props
+    delete $schema->{properties}->{full};
+    delete $schema->{properties}->{purge};
+
+    return $schema;
+}
+
+sub run {
+    my ($class, $conf, $id, $schedule) = @_;
+
+    my $node = $conf->{node};
+    for my $opt (keys %$conf) {
+	delete $conf->{$opt} if !defined($props->{$opt});
+    }
+
+    my $realm = $conf->{realm};
+    my $statedir = "/etc/pve/priv/jobs";
+    my $statefile = "$statedir/realmsync-$id.json";
+
+    if (!defined($node)) {
+	# cluster synced
+	my $now = time();
+	my $nodename = PVE::INotify::nodename();
+
+	# check statefile in pmxcfs if we should start
+	my $shouldrun = PVE::Cluster::cfs_lock_domain($realm, undef, sub {
+	    mkdir $statedir;
+	    my $raw = eval { PVE::Tools::file_get_contents($statefile) } // '';
+	    my $members = PVE::Cluster::get_members();
+
+	    my $state = ($raw =~ m/^(\{.*\})$/) ? decode_json($1) : {};
+	    my $lastnode = $state->{node} // $nodename;
+	    my $lastupid = $state->{upid};
+	    my $lasttime = $state->{time};
+
+	    my $node_online = 0;
+	    if ($lastnode ne $nodename) {
+		if (defined(my $member = $members->{$lastnode})) {
+		    $node_online = $member->{online};
+		}
+	    } else {
+		$node_online = 1;
+	    }
+
+	    if (defined($lastupid)) {
+		# first check if the next run is scheduled
+		if (my $parsed = PVE::Tools::upid_decode($lastupid, 1)) {
+		    my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);
+		    my $next_sync = PVE::CalendarEvent::compute_next_event($calspec, $parsed->{starttime});
+		    return 0 if !defined($next_sync) || $now < $next_sync; # not yet its (next) turn
+		}
+		# check if still running and node is online
+		my $tasks = PVE::Cluster::get_tasklist();
+		for my $task (@$tasks) {
+		    next if $task->{upid} ne $lastupid;
+		    last if defined($task->{endtime}); # it's already finished
+		    last if !$node_online; # it's not finished but the node is offline
+		    return 0; # not finished and online
+		}
+	    } elsif (defined($lasttime) && ($lasttime+60) > $now && $node_online) {
+		# another node started in the last 60 seconds and is online
+		return 0;
+	    }
+
+	    # any of the following conditions should be true here:
+	    # * it was started on another node but that node is offline now
+	    # * it was started but either too long ago, or with an error
+	    # * the started task finished
+
+	    PVE::Tools::file_set_contents($statefile, encode_json({
+		node => $nodename,
+		time => $now,
+	    }));
+	    return 1;
+	});
+	die $@ if $@;
+
+	if ($shouldrun) {
+	    my $upid = eval { PVE::API2::Domains->sync($conf) };
+	    my $err = $@;
+	    PVE::Cluster::cfs_lock_domain($realm, undef, sub {
+		if ($err && !$upid) {
+		    PVE::Tools::file_set_contents($statefile, encode_json({
+			node => $nodename,
+			time => $now,
+			error => $err,
+		    }));
+		    die "$err\n";
+		}
+
+		PVE::Tools::file_set_contents($statefile, encode_json({
+		    node => $nodename,
+		    upid => $upid,
+		}));
+	    });
+	    die $@ if $@;
+	    return $upid;
+	}
+
+	return "OK"; # all other cases should not run the sync on this node
+    }
+
+    return PVE::API2::Domains->sync($conf);
+}
+
+1;
diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index c839d8f..dfc5314 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -8,3 +8,4 @@ install:
 	install -D -m 0644 TokenConfig.pm ${DESTDIR}${PERLDIR}/PVE/TokenConfig.pm
 	make -C API2 install
 	make -C CLI install
+	make -C Jobs install
-- 
2.30.2

Re: [pve-devel] [PATCH access-control v2 2/2] add realmsync plugin for jobs and CRUD api for domainsync-jobs

[Thomas Lamprecht]

Some mostly API and slightly higher level or cosmetical comments, but also some issue with
how you use the cfs domain locks at the end.

Also, a patch-wide s/realmsync/realm-sync/ would be great.

Am 06/12/2022 um 12:06 schrieb Dominik Csapak:
> to be able to define automated jobs that sync ldap/ad
> 
> The jobs plugin contains special handling when no node is given, since
> we only want it to run on a single node when that triggers. For that,
> we save a statefile in /etc/pve/priv/jobs/ which contains the
> node/time/upid of the node that runs the job. The first node that
> is able to lock the realm (via cfs_lock_domain) "wins" and may
> sync from the ldap.
> 
> in case a specific node was selected, this is omitted and the Jobs
> handling will not let it run on other nodes anyway
> 
> the API part is our usual sectionconfig CRUD api, but specialized
> for the specific type of job.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/API2/AccessControl.pm               |   6 +
>  src/PVE/API2/AccessControl/Job.pm           |  47 +++
>  src/PVE/API2/AccessControl/Job/Makefile     |   6 +
>  src/PVE/API2/AccessControl/Job/RealmSync.pm | 324 ++++++++++++++++++++
>  src/PVE/API2/AccessControl/Makefile         |   9 +
>  src/PVE/API2/Makefile                       |   4 +
>  src/PVE/Jobs/Makefile                       |   6 +
>  src/PVE/Jobs/RealmSync.pm                   | 193 ++++++++++++
>  src/PVE/Makefile                            |   1 +
>  9 files changed, 596 insertions(+)
>  create mode 100644 src/PVE/API2/AccessControl/Job.pm
>  create mode 100644 src/PVE/API2/AccessControl/Job/Makefile
>  create mode 100644 src/PVE/API2/AccessControl/Job/RealmSync.pm
>  create mode 100644 src/PVE/API2/AccessControl/Makefile
>  create mode 100644 src/PVE/Jobs/Makefile
>  create mode 100644 src/PVE/Jobs/RealmSync.pm
> 
> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
> index 104e7e3..221ecb2 100644
> --- a/src/PVE/API2/AccessControl.pm
> +++ b/src/PVE/API2/AccessControl.pm
> @@ -14,6 +14,7 @@ use PVE::DataCenterConfig;
>  use PVE::RESTHandler;
>  use PVE::AccessControl;
>  use PVE::JSONSchema qw(get_standard_option);
> +use PVE::API2::AccessControl::Job;
>  use PVE::API2::Domains;
>  use PVE::API2::User;
>  use PVE::API2::Group;
> @@ -57,6 +58,11 @@ __PACKAGE__->register_method ({
>      path => 'domains',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::AccessControl::Job",
> +    path => 'jobs',
> +});
> +
>  __PACKAGE__->register_method ({
>      subclass => "PVE::API2::OpenId",
>      path => 'openid',
> diff --git a/src/PVE/API2/AccessControl/Job.pm b/src/PVE/API2/AccessControl/Job.pm
> new file mode 100644
> index 0000000..cf2d0a9
> --- /dev/null
> +++ b/src/PVE/API2/AccessControl/Job.pm
> @@ -0,0 +1,47 @@
> +package PVE::API2::AccessControl::Job;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::RESTHandler;
> +
> +use PVE::API2::AccessControl::Job::RealmSync;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::AccessControl::Job::RealmSync",
> +    path => 'realm-sync',
> +});
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "Directory index.",
> +    permissions => {
> +	user => 'all',
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {
> +		subdir => { type => 'string' },
> +	    },
> +	},
> +	links => [ { rel => 'child', href => "{subdir}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	return {
> +	    subdir => 'realm-sync',
> +	};

as from the return type schema this should be an array:

return [
    { subdir => 'realm-sync' },
];

> +    }});
> +
> +1;
> diff --git a/src/PVE/API2/AccessControl/Job/Makefile b/src/PVE/API2/AccessControl/Job/Makefile
> new file mode 100644
> index 0000000..5feae4b
> --- /dev/null
> +++ b/src/PVE/API2/AccessControl/Job/Makefile
> @@ -0,0 +1,6 @@
> +API2_SOURCES= 		\
> +	RealmSync.pm	\
> +
> +.PHONY: install
> +install:
> +	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/AccessControl/Job/$$i; done
> diff --git a/src/PVE/API2/AccessControl/Job/RealmSync.pm b/src/PVE/API2/AccessControl/Job/RealmSync.pm
> new file mode 100644
> index 0000000..f14bb2b
> --- /dev/null
> +++ b/src/PVE/API2/AccessControl/Job/RealmSync.pm
> @@ -0,0 +1,324 @@
> +package PVE::API2::AccessControl::Job::RealmSync;
> +
> +use strict;
> +use warnings;
> +
> +use JSON;
> +
> +use PVE::Cluster qw (cfs_read_file cfs_write_file cfs_lock_file);
> +use PVE::Exception qw(raise_param_exc);
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Job::Registry;
> +use PVE::RESTHandler;
> +use PVE::SafeSyslog;
> +use PVE::Tools qw(extract_param);
> +
> +use PVE::AccessControl;
> +use PVE::Auth::Plugin;
> +use PVE::Jobs::RealmSync;
> +
> +
> +use base qw(PVE::RESTHandler);
> +
> +my $get_cluster_last_run = sub {
> +    my ($jobid) = @_;
> +
> +    my $fn = "/etc/pve/priv/jobs/realmsync-$jobid.json";
> +    my $raw = PVE::Tools::file_get_contents($fn);
> +    my $state = eval { decode_json($raw) };
> +    die "json decode error on $fn: $@\n" if $@;
> +
> +    if (my $upid = $state->{upid}) {
> +	if (my $decoded = PVE::Tools::upid_decode($upid)) {
> +	    return $decoded->{starttime};
> +	}
> +    } else {
> +	return $state->{time};
> +    }
> +
> +    return undef;
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'syncjob_index',
> +    path => '',
> +    method => 'GET',
> +    description => "List configured realm-sync-jobs.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Audit']],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {
> +		id => {
> +		    description => "The ID of the entry.",
> +		    type => 'string'
> +		},
> +		enabled => {
> +		    description => "If the job is enalbed or not.",

typo: enabled

> +		    type => 'boolean',
> +		},
> +		comment => {
> +		    description => "A comment for the job.",
> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		schedule => {
> +		    description => "The configured sync schedule.",
> +		    type => 'string',
> +		    optional => 1,

do we want to allow a empty schedule if there's a enable boolean switch anyway?

Or is this for making the "only used to run manually" use case slightly less confusing
to configure (as enabled=false + bogus schedule might be weird for some)?

> +		},
> +		realm => {
> +		    description => "The realm to sync.",
> +		    type => 'string',
> +		    optional => 1,

is this really optional?

> +		},
> +		node => {
> +		    description => "The node to run the job on, if any.",

what if this is empty? might want to communicate that over the description, either with
extra sentence or something like "The node the job is limited too run on"

Albeit it's IMO a bit of a odd thing in general, as especially realm syncs will always
have effects on the whole cluster, so why expose that and not just auto select one
(e.h., jitter sleep + node lock + check if recently run); we can always add such things
if there's an actual need for it.

> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		scope => {
> +		    description => "The Selection of what to sync.",

might want to define the enum here? or better said, should this be a (registered) format
we can reuse here?

> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		'remove-vanished' => {
> +		    description => "A list of things to remove when the entity vanished during a sync.",

same here

> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		'last-run' => {
> +		    description => "UNIX epoch when the job last ran.",

"Last execution time of the job in seconds since the beginning of the UNIX epoch."

> +		    type => 'integer',
> +		    optional => 1,
> +		},
> +		'next-run' => {
> +		    description => "UNIX epoch when the job is planned to run next.",

"Next planned execution time of the job in seconds since the beginning of the UNIX epoch."

> +		    type => 'integer',
> +		    optional => 1,
> +		},
> +	    },
> +	},
> +	links => [ { rel => 'child', href => "{id}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	my $order = $jobs_data->{order};
> +	my $jobs = $jobs_data->{ids};
> +
> +	my $res = [];
> +	for my $jobid (sort { $order->{$a} <=> $order->{$b} } keys %$jobs) {
> +	    my $job = $jobs->{$jobid};
> +	    next if $job->{type} ne 'realmsync';

Should the "type" be "realm-sync" for consistency?

> +
> +	    $job->{id} = $jobid;
> +	    if (my $schedule = $job->{schedule}) {
> +		$job->{'last-run'} = eval { $get_cluster_last_run->($jobid) };
> +		my $last_run = $job->{'last-run'} // time(); # current time as fallback
> +		my $calendar_event = Proxmox::RS::CalendarEvent->new($schedule);
> +		my $next_run = $calendar_event->compute_next_event($last_run);
> +		$job->{'next-run'} = $next_run if defined($next_run);
> +	    }
> +
> +	    push @$res, $job;
> +	}
> +
> +	return $res;
> +    }});
> +
> +__PACKAGE__->register_method({
> +    name => 'read_job',
> +    path => '{id}',
> +    method => 'GET',
> +    description => "Read realm-sync job definition.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Audit']],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    id => {
> +		type => 'string',
> +		format => 'pve-configid',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'object',
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $jobs = cfs_read_file('jobs.cfg');
> +	my $id = $param->{id};
> +	my $job = $jobs->{ids}->{$id};
> +	return $job if $job && $job->{type} eq 'realmsync';
> +
> +	raise_param_exc({ id => "No such job '$id'" });
> +
> +    }});
> +
> +__PACKAGE__->register_method({
> +    name => 'create_job',
> +    path => '{id}',
> +    method => 'POST',
> +    protected => 1,
> +    description => "Create new realm-sync job.",
> +    permissions => {
> +	description => "'Realm.AllocateUser' on '/access/realm/<realm>' and "
> +	    ."'User.Modify' permissions to '/access/groups/'.",
> +	check => [ 'and',
> +	    ['perm', '/access/realm/{realm}', ['Realm.AllocateUser']],
> +	    ['perm', '/access/groups', ['User.Modify']],
> +	],
> +    },
> +    parameters => PVE::Jobs::RealmSync->createSchema(),
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $id = extract_param($param, 'id');
> +
> +	cfs_lock_file('jobs.cfg', undef, sub {
> +	    my $data = cfs_read_file('jobs.cfg');
> +
> +	    die "Job '$id' already exists\n"
> +		if $data->{ids}->{$id};
> +
> +	    my $plugin = PVE::Job::Registry->lookup('realmsync');
> +	    my $opts = $plugin->check_config($id, $param, 1, 1);
> +
> +	    my $realm = $opts->{realm};
> +	    my $cfg = cfs_read_file('domains.cfg');
> +
> +	    raise_param_exc({ realm => "No such realm '$realm'" })
> +		if !defined($cfg->{ids}->{$realm});
> +
> +	    my $realm_type = $cfg->{ids}->{$realm}->{type};
> +	    raise_param_exc({ realm => "Only LDAP/AD realms can be synced." })
> +		if $realm_type ne 'ldap' && $realm_type ne 'ad';
> +
> +	    $data->{ids}->{$id} = $opts;
> +
> +	    PVE::Jobs::create_job($id, 'realmsync');
> +
> +	    cfs_write_file('jobs.cfg', $data);
> +	});
> +	die "$@" if ($@);
> +
> +	return undef;
> +    }});
> +
> +__PACKAGE__->register_method({
> +    name => 'update_job',
> +    path => '{id}',
> +    method => 'PUT',
> +    protected => 1,
> +    description => "Update realm-sync job definition.",
> +    permissions => {
> +	description => "'Realm.AllocateUser' on '/access/realm/<realm>' and "
> +	    ." 'User.Modify' permissions to '/access/groups/'.",
> +	check => [ 'and',
> +	    ['perm', '/access/realm/{realm}', ['Realm.AllocateUser']],
> +	    ['perm', '/access/groups', ['User.Modify']],
> +	],
> +    },
> +    parameters => PVE::Jobs::RealmSync->updateSchema(),
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $id = extract_param($param, 'id');
> +	my $delete = extract_param($param, 'delete');
> +	if ($delete) {
> +	    $delete = [PVE::Tools::split_list($delete)];
> +	}
> +
> +	cfs_lock_file('jobs.cfg', undef, sub {
> +	    my $jobs = cfs_read_file('jobs.cfg');
> +
> +	    die "no options specified\n" if !scalar(keys %$param);
> +
> +	    my $plugin = PVE::Job::Registry->lookup('realmsync');
> +	    my $opts = $plugin->check_config($id, $param, 0, 1);
> +
> +	    my $job = $jobs->{ids}->{$id};
> +	    die "no such realmsync job\n" if !$job || $job->{type} ne 'realmsync';
> +
> +	    my $options = $plugin->options();
> +	    for my $k (@$delete) {
> +		my $d = $options->{$k} || die "no such option '$k'\n";
> +		die "unable to delete required option '$k'\n" if !$d->{optional};
> +		die "unable to delete fixed option '$k'\n" if $d->{fixed};
> +		die "cannot set and delete property '$k' at the same time!\n"
> +			if defined($opts->{$k});
> +		delete $job->{$k};
> +	    }

this seems something that we have a few times already in some form, might want to add this
as helper somehere, possibly to section config.

> +
> +	    $job->{$_} = $param->{$_} for keys $param->%*;
> +
> +	    cfs_write_file('jobs.cfg', $jobs);
> +
> +	    PVE::Jobs::detect_changed_runtime_props($id, 'realmsync', $job);
> +
> +	    return;
> +	});
> +	die "$@" if ($@);
> +    }});
> +
> +
> +__PACKAGE__->register_method({
> +    name => 'delete_job',
> +    path => '{id}',
> +    method => 'DELETE',
> +    description => "Delete realm-sync job definition.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Modify']],
> +    },
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    id => {
> +		type => 'string',
> +		format => 'pve-configid',
> +	    },
> +	},
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $id = $param->{id};
> +
> +	cfs_lock_file('jobs.cfg', undef, sub {
> +	    my $jobs = cfs_read_file('jobs.cfg');
> +
> +	    if (!defined($jobs->{ids}->{$id}) || $jobs->{ids}->{$id}->{type} ne 'realmsync') {
> +		raise_param_exc({ id => "No such job '$id'" });
> +	    }
> +	    delete $jobs->{ids}->{$id};
> +
> +	    PVE::Jobs::remove_job($id, 'realmsync');> +
> +	    cfs_write_file('jobs.cfg', $jobs);
> +	    unlink "/etc/pve/priv/jobs/realmsync-$id.json";

should be contained in a sub, either save_state($id, undef) or explicit delete_state one (names
are just examples)

> +	});
> +	die "$@" if $@;
> +
> +	return undef;
> +    }});
> +
> +1;
> diff --git a/src/PVE/API2/AccessControl/Makefile b/src/PVE/API2/AccessControl/Makefile
> new file mode 100644
> index 0000000..8d3fb58
> --- /dev/null
> +++ b/src/PVE/API2/AccessControl/Makefile
> @@ -0,0 +1,9 @@
> +API2_SOURCES= 	\
> +	Job.pm	\
> +
> +SUBDIRS=Job
> +
> +.PHONY: install
> +install:
> +	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/AccessControl/$$i; done
> +	set -e && for i in ${SUBDIRS}; do ${MAKE} -C $$i $@; done
> diff --git a/src/PVE/API2/Makefile b/src/PVE/API2/Makefile
> index 2817f48..7d2be93 100644
> --- a/src/PVE/API2/Makefile
> +++ b/src/PVE/API2/Makefile
> @@ -2,6 +2,7 @@
>  API2_SOURCES= 		 	\
>  	AccessControl.pm 	\
>  	Domains.pm	 	\
> +	RealmSync.pm	 	\
>  	ACL.pm		 	\
>  	Role.pm		 	\
>  	Group.pm	 	\
> @@ -9,6 +10,9 @@ API2_SOURCES= 		 	\
>  	TFA.pm			\
>  	OpenId.pm
>  
> +SUBDIRS=AccessControl
> +
>  .PHONY: install
>  install:
>  	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/$$i; done
> +	set -e && for i in ${SUBDIRS}; do ${MAKE} -C $$i $@; done
> diff --git a/src/PVE/Jobs/Makefile b/src/PVE/Jobs/Makefile
> new file mode 100644
> index 0000000..9eed1b2
> --- /dev/null
> +++ b/src/PVE/Jobs/Makefile
> @@ -0,0 +1,6 @@
> +SOURCES=RealmSync.pm
> +
> +.PHONY: install
> +install: ${SOURCES}
> +	install -d -m 0755 ${DESTDIR}${PERLDIR}/PVE/Jobs
> +	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Jobs/$$i; done
> diff --git a/src/PVE/Jobs/RealmSync.pm b/src/PVE/Jobs/RealmSync.pm
> new file mode 100644
> index 0000000..e8a86e3
> --- /dev/null
> +++ b/src/PVE/Jobs/RealmSync.pm
> @@ -0,0 +1,193 @@
> +package PVE::Jobs::RealmSync;
> +
> +use strict;
> +use warnings;
> +
> +use JSON;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Cluster;
> +use PVE::CalendarEvent;
> +use PVE::Tools;
> +
> +use PVE::API2::Domains;
> +
> +use base qw(PVE::Job::Registry);
> +
> +sub type {
> +    return 'realmsync';
> +}
> +
> +my $props = get_standard_option('realm-sync-options', {
> +    realm => get_standard_option('realm'),
> +});
> +
> +sub properties {
> +    return $props;
> +}
> +
> +sub options {
> +    my $options = {
> +	enabled => { optional => 1 },
> +	schedule => {},
> +	comment => { optional => 1 },
> +	node => { optional => 1 },

might want to drop that for now (as commented above in the api return schema) 

> +	scope => {},
> +    };
> +    for my $opt (keys %$props) {
> +	next if defined($options->{$opt});
> +	if ($props->{$opt}->{optional}) {
> +	    $options->{$opt} = { optional => 1 };
> +	} else {
> +	    $options->{$opt} = {};
> +	}
> +    }
> +    $options->{realm}->{fixed} = 1;
> +
> +    return $options;
> +}
> +
> +sub decode_value {
> +    my ($class, $type, $key, $value) = @_;
> +    return $value;
> +}
> +
> +sub encode_value {
> +    my ($class, $type, $key, $value) = @_;
> +    return $value;
> +}
> +
> +sub createSchema {
> +    my ($class, $skip_type) = @_;

add extra newline here

> +    my $schema = $class->SUPER::createSchema($skip_type);
> +
> +    my $opts = $class->options();
> +    for my $opt (keys $schema->{properties}->%*) {
> +	next if defined($opts->{$opt}) || $opt eq 'id';
> +	delete $schema->{properties}->{$opt};
> +    }
> +
> +    # remove legacy props

bit confused, we never had sync jobs, so why should it matter?

> +    delete $schema->{properties}->{full};
> +    delete $schema->{properties}->{purge};
> +
> +    return $schema;
> +}
> +
> +sub updateSchema {
> +    my ($class, $skip_type) = @_;
> +    my $schema = $class->SUPER::updateSchema($skip_type);
> +
> +    my $opts = $class->options();
> +    for my $opt (keys $schema->{properties}->%*) {
> +	next if defined($opts->{$opt});
> +	next if $opt eq 'id' || $opt eq 'delete';
> +	delete $schema->{properties}->{$opt};
> +    }
> +
> +    # remove legacy props

same here

> +    delete $schema->{properties}->{full};
> +    delete $schema->{properties}->{purge};
> +
> +    return $schema;
> +}
> +
> +sub run {
> +    my ($class, $conf, $id, $schedule) = @_;
> +
> +    my $node = $conf->{node};
> +    for my $opt (keys %$conf) {
> +	delete $conf->{$opt} if !defined($props->{$opt});
> +    }
> +
> +    my $realm = $conf->{realm};
> +    my $statedir = "/etc/pve/priv/jobs";
> +    my $statefile = "$statedir/realmsync-$id.json";

s/realmsync/realm-sync/

> +
> +    if (!defined($node)) {
> +	# cluster synced
> +	my $now = time();
> +	my $nodename = PVE::INotify::nodename();
> +
> +	# check statefile in pmxcfs if we should start
> +	my $shouldrun = PVE::Cluster::cfs_lock_domain($realm, undef, sub {

erm, using just the $realm as lock ID can go quite bad, e.g., if a realm is named
"ha" it would interfere with the HA domain locks. Can we prefix this, or just use
the general, non-realm specific "realm-sync"? Not much downside in practice and
adding things like groups might be better, even if just for auditability, if not done
in parallel anyway?

In anyway, a domain lock must never be (just) an $id.


> +	    mkdir $statedir;
> +	    my $raw = eval { PVE::Tools::file_get_contents($statefile) } // '';


a non-node specific kv-store might be really nice, would avoid some DB/disk write
churn (note that this specific one is adding that much extra churn, but in sum
we start to have now quite a few places where it would help, but can me  moved to
such a thing later on too)

> +	    my $members = PVE::Cluster::get_members();
> +
> +	    my $state = ($raw =~ m/^(\{.*\})$/) ? decode_json($1) : {};
> +	    my $lastnode = $state->{node} // $nodename;
> +	    my $lastupid = $state->{upid};
> +	    my $lasttime = $state->{time};

nit: please stay consistent with variable name style (snake_case vs noneatall - prefer the
former, at least if not for things where we used nostyle since pretty much always, e.g.,
nodename, albeit just $node is often fitting for that anyway)

> +
> +	    my $node_online = 0;

should this be $last_node_online?

> +	    if ($lastnode ne $nodename) {
> +		if (defined(my $member = $members->{$lastnode})) {
> +		    $node_online = $member->{online};
> +		}
> +	    } else {
> +		$node_online = 1;
> +	    }

could be way shorter (untested though):
 
my $node_online = $last_node eq $nodename || ($members->{$last_node} // {})->{online};

> +
> +	    if (defined($lastupid)) {
> +		# first check if the next run is scheduled
> +		if (my $parsed = PVE::Tools::upid_decode($lastupid, 1)) {
> +		    my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);

same here w.r.t. variable naming style

> +		    my $next_sync = PVE::CalendarEvent::compute_next_event($calspec, $parsed->{starttime});
> +		    return 0 if !defined($next_sync) || $now < $next_sync; # not yet its (next) turn
> +		}
> +		# check if still running and node is online
> +		my $tasks = PVE::Cluster::get_tasklist();
> +		for my $task (@$tasks) {
> +		    next if $task->{upid} ne $lastupid;
> +		    last if defined($task->{endtime}); # it's already finished
> +		    last if !$node_online; # it's not finished but the node is offline

s/but/and/ ?

> +		    return 0; # not finished and online
> +		}
> +	    } elsif (defined($lasttime) && ($lasttime+60) > $now && $node_online) {
> +		# another node started in the last 60 seconds and is online

"another node started this job in the last 60 seconds and is still online"

> +		return 0;
> +	    }
> +
> +	    # any of the following conditions should be true here:
> +	    # * it was started on another node but that node is offline now
> +	    # * it was started but either too long ago, or with an error
> +	    # * the started task finished
> +
> +	    PVE::Tools::file_set_contents($statefile, encode_json({
> +		node => $nodename,
> +		time => $now,
> +	    }));

I would slightly prefer to move this into a (just an example name) save_state method,
even if really small it's IMO better to contain it there, same for getting the state above.

> +	    return 1;
> +	});
> +	die $@ if $@;
> +
> +	if ($shouldrun) {
> +	    my $upid = eval { PVE::API2::Domains->sync($conf) };
> +	    my $err = $@;
> +	    PVE::Cluster::cfs_lock_domain($realm, undef, sub {

same here w.r.t. wrongly used domain locking

> +		if ($err && !$upid) {
> +		    PVE::Tools::file_set_contents($statefile, encode_json({
> +			node => $nodename,
> +			time => $now,
> +			error => $err,
> +		    }));
> +		    die "$err\n";
> +		}
> +
> +		PVE::Tools::file_set_contents($statefile, encode_json({
> +		    node => $nodename,
> +		    upid => $upid,
> +		}));

same here with state saving

> +	    });
> +	    die $@ if $@;
> +	    return $upid;
> +	}
> +
> +	return "OK"; # all other cases should not run the sync on this node

possibly stupid question, but why return a string here?

> +    }
> +
> +    return PVE::API2::Domains->sync($conf);
> +}
> +
> +1;
> diff --git a/src/PVE/Makefile b/src/PVE/Makefile
> index c839d8f..dfc5314 100644
> --- a/src/PVE/Makefile
> +++ b/src/PVE/Makefile
> @@ -8,3 +8,4 @@ install:
>  	install -D -m 0644 TokenConfig.pm ${DESTDIR}${PERLDIR}/PVE/TokenConfig.pm
>  	make -C API2 install
>  	make -C CLI install
> +	make -C Jobs install

Re: [pve-devel] [PATCH access-control v2 2/2] add realmsync plugin for jobs and CRUD api for domainsync-jobs

[Dominik Csapak]

On 12/13/22 14:43, Thomas Lamprecht wrote:
> Some mostly API and slightly higher level or cosmetical comments, but also some issue with
> how you use the cfs domain locks at the end.
> 
> Also, a patch-wide s/realmsync/realm-sync/ would be great.

sure, no problem


> 
> Am 06/12/2022 um 12:06 schrieb Dominik Csapak:
>> to be able to define automated jobs that sync ldap/ad
>>
>> The jobs plugin contains special handling when no node is given, since
>> we only want it to run on a single node when that triggers. For that,
>> we save a statefile in /etc/pve/priv/jobs/ which contains the
>> node/time/upid of the node that runs the job. The first node that
>> is able to lock the realm (via cfs_lock_domain) "wins" and may
>> sync from the ldap.
>>
>> in case a specific node was selected, this is omitted and the Jobs
>> handling will not let it run on other nodes anyway
>>
>> the API part is our usual sectionconfig CRUD api, but specialized
>> for the specific type of job.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/API2/AccessControl.pm               |   6 +
>>   src/PVE/API2/AccessControl/Job.pm           |  47 +++
>>   src/PVE/API2/AccessControl/Job/Makefile     |   6 +
>>   src/PVE/API2/AccessControl/Job/RealmSync.pm | 324 ++++++++++++++++++++
>>   src/PVE/API2/AccessControl/Makefile         |   9 +
>>   src/PVE/API2/Makefile                       |   4 +
>>   src/PVE/Jobs/Makefile                       |   6 +
>>   src/PVE/Jobs/RealmSync.pm                   | 193 ++++++++++++
>>   src/PVE/Makefile                            |   1 +
>>   9 files changed, 596 insertions(+)
>>   create mode 100644 src/PVE/API2/AccessControl/Job.pm
>>   create mode 100644 src/PVE/API2/AccessControl/Job/Makefile
>>   create mode 100644 src/PVE/API2/AccessControl/Job/RealmSync.pm
>>   create mode 100644 src/PVE/API2/AccessControl/Makefile
>>   create mode 100644 src/PVE/Jobs/Makefile
>>   create mode 100644 src/PVE/Jobs/RealmSync.pm
>>
>> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
>> index 104e7e3..221ecb2 100644
>> --- a/src/PVE/API2/AccessControl.pm
>> +++ b/src/PVE/API2/AccessControl.pm
>> @@ -14,6 +14,7 @@ use PVE::DataCenterConfig;
>>   use PVE::RESTHandler;
>>   use PVE::AccessControl;
>>   use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::API2::AccessControl::Job;
>>   use PVE::API2::Domains;
>>   use PVE::API2::User;
>>   use PVE::API2::Group;
>> @@ -57,6 +58,11 @@ __PACKAGE__->register_method ({
>>       path => 'domains',
>>   });
>>   
>> +__PACKAGE__->register_method ({
>> +    subclass => "PVE::API2::AccessControl::Job",
>> +    path => 'jobs',
>> +});
>> +
>>   __PACKAGE__->register_method ({
>>       subclass => "PVE::API2::OpenId",
>>       path => 'openid',
>> diff --git a/src/PVE/API2/AccessControl/Job.pm b/src/PVE/API2/AccessControl/Job.pm
>> new file mode 100644
>> index 0000000..cf2d0a9
>> --- /dev/null
>> +++ b/src/PVE/API2/AccessControl/Job.pm
>> @@ -0,0 +1,47 @@
>> +package PVE::API2::AccessControl::Job;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::RESTHandler;
>> +
>> +use PVE::API2::AccessControl::Job::RealmSync;
>> +
>> +use base qw(PVE::RESTHandler);
>> +
>> +__PACKAGE__->register_method ({
>> +    subclass => "PVE::API2::AccessControl::Job::RealmSync",
>> +    path => 'realm-sync',
>> +});
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'index',
>> +    path => '',
>> +    method => 'GET',
>> +    description => "Directory index.",
>> +    permissions => {
>> +	user => 'all',
>> +    },
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {},
>> +    },
>> +    returns => {
>> +	type => 'array',
>> +	items => {
>> +	    type => "object",
>> +	    properties => {
>> +		subdir => { type => 'string' },
>> +	    },
>> +	},
>> +	links => [ { rel => 'child', href => "{subdir}" } ],
>> +    },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	return {
>> +	    subdir => 'realm-sync',
>> +	};
> 
> as from the return type schema this should be an array:
> 
> return [
>      { subdir => 'realm-sync' },
> ];

oops

> 
>> +    }});
>> +
>> +1;
>> diff --git a/src/PVE/API2/AccessControl/Job/Makefile b/src/PVE/API2/AccessControl/Job/Makefile
>> new file mode 100644
>> index 0000000..5feae4b
>> --- /dev/null
>> +++ b/src/PVE/API2/AccessControl/Job/Makefile
>> @@ -0,0 +1,6 @@
>> +API2_SOURCES= 		\
>> +	RealmSync.pm	\
>> +
>> +.PHONY: install
>> +install:
>> +	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/AccessControl/Job/$$i; done
>> diff --git a/src/PVE/API2/AccessControl/Job/RealmSync.pm b/src/PVE/API2/AccessControl/Job/RealmSync.pm
>> new file mode 100644
>> index 0000000..f14bb2b
>> --- /dev/null
>> +++ b/src/PVE/API2/AccessControl/Job/RealmSync.pm
>> @@ -0,0 +1,324 @@
>> +package PVE::API2::AccessControl::Job::RealmSync;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use JSON;
>> +
>> +use PVE::Cluster qw (cfs_read_file cfs_write_file cfs_lock_file);
>> +use PVE::Exception qw(raise_param_exc);
>> +use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::Job::Registry;
>> +use PVE::RESTHandler;
>> +use PVE::SafeSyslog;
>> +use PVE::Tools qw(extract_param);
>> +
>> +use PVE::AccessControl;
>> +use PVE::Auth::Plugin;
>> +use PVE::Jobs::RealmSync;
>> +
>> +
>> +use base qw(PVE::RESTHandler);
>> +
>> +my $get_cluster_last_run = sub {
>> +    my ($jobid) = @_;
>> +
>> +    my $fn = "/etc/pve/priv/jobs/realmsync-$jobid.json";
>> +    my $raw = PVE::Tools::file_get_contents($fn);
>> +    my $state = eval { decode_json($raw) };
>> +    die "json decode error on $fn: $@\n" if $@;
>> +
>> +    if (my $upid = $state->{upid}) {
>> +	if (my $decoded = PVE::Tools::upid_decode($upid)) {
>> +	    return $decoded->{starttime};
>> +	}
>> +    } else {
>> +	return $state->{time};
>> +    }
>> +
>> +    return undef;
>> +};
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'syncjob_index',
>> +    path => '',
>> +    method => 'GET',
>> +    description => "List configured realm-sync-jobs.",
>> +    permissions => {
>> +	check => ['perm', '/', ['Sys.Audit']],
>> +    },
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {},
>> +    },
>> +    returns => {
>> +	type => 'array',
>> +	items => {
>> +	    type => "object",
>> +	    properties => {
>> +		id => {
>> +		    description => "The ID of the entry.",
>> +		    type => 'string'
>> +		},
>> +		enabled => {
>> +		    description => "If the job is enalbed or not.",
> 
> typo: enabled
> 
>> +		    type => 'boolean',
>> +		},
>> +		comment => {
>> +		    description => "A comment for the job.",
>> +		    type => 'string',
>> +		    optional => 1,
>> +		},
>> +		schedule => {
>> +		    description => "The configured sync schedule.",
>> +		    type => 'string',
>> +		    optional => 1,
> 
> do we want to allow a empty schedule if there's a enable boolean switch anyway?
> 
> Or is this for making the "only used to run manually" use case slightly less confusing
> to configure (as enabled=false + bogus schedule might be weird for some)?
> 
>> +		},
>> +		realm => {
>> +		    description => "The realm to sync.",
>> +		    type => 'string',
>> +		    optional => 1,
> 
> is this really optional?

no you're right, was too fast with c&p

> 
>> +		},
>> +		node => {
>> +		    description => "The node to run the job on, if any.",
> 
> what if this is empty? might want to communicate that over the description, either with
> extra sentence or something like "The node the job is limited too run on"
> 
> Albeit it's IMO a bit of a odd thing in general, as especially realm syncs will always
> have effects on the whole cluster, so why expose that and not just auto select one
> (e.h., jitter sleep + node lock + check if recently run); we can always add such things
> if there's an actual need for it.

sure makes sense


> 
>> +		    type => 'string',
>> +		    optional => 1,
>> +		},
>> +		scope => {
>> +		    description => "The Selection of what to sync.",
> 
> might want to define the enum here? or better said, should this be a (registered) format
> we can reuse here?
> 
>> +		    type => 'string',
>> +		    optional => 1,
>> +		},
>> +		'remove-vanished' => {
>> +		    description => "A list of things to remove when the entity vanished during a sync.",
> 
> same here
> 
>> +		    type => 'string',
>> +		    optional => 1,
>> +		},
>> +		'last-run' => {
>> +		    description => "UNIX epoch when the job last ran.",
> 
> "Last execution time of the job in seconds since the beginning of the UNIX epoch."
> 
>> +		    type => 'integer',
>> +		    optional => 1,
>> +		},
>> +		'next-run' => {
>> +		    description => "UNIX epoch when the job is planned to run next.",
> 
> "Next planned execution time of the job in seconds since the beginning of the UNIX epoch."
> 
>> +		    type => 'integer',
>> +		    optional => 1,
>> +		},
>> +	    },
>> +	},
>> +	links => [ { rel => 'child', href => "{id}" } ],
>> +    },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $rpcenv = PVE::RPCEnvironment::get();
>> +	my $user = $rpcenv->get_user();
>> +
>> +	my $jobs_data = cfs_read_file('jobs.cfg');
>> +	my $order = $jobs_data->{order};
>> +	my $jobs = $jobs_data->{ids};
>> +
>> +	my $res = [];
>> +	for my $jobid (sort { $order->{$a} <=> $order->{$b} } keys %$jobs) {
>> +	    my $job = $jobs->{$jobid};
>> +	    next if $job->{type} ne 'realmsync';
> 
> Should the "type" be "realm-sync" for consistency?
> 
>> +
>> +	    $job->{id} = $jobid;
>> +	    if (my $schedule = $job->{schedule}) {
>> +		$job->{'last-run'} = eval { $get_cluster_last_run->($jobid) };
>> +		my $last_run = $job->{'last-run'} // time(); # current time as fallback
>> +		my $calendar_event = Proxmox::RS::CalendarEvent->new($schedule);
>> +		my $next_run = $calendar_event->compute_next_event($last_run);
>> +		$job->{'next-run'} = $next_run if defined($next_run);
>> +	    }
>> +
>> +	    push @$res, $job;
>> +	}
>> +
>> +	return $res;
>> +    }});
>> +
>> +__PACKAGE__->register_method({
>> +    name => 'read_job',
>> +    path => '{id}',
>> +    method => 'GET',
>> +    description => "Read realm-sync job definition.",
>> +    permissions => {
>> +	check => ['perm', '/', ['Sys.Audit']],
>> +    },
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    id => {
>> +		type => 'string',
>> +		format => 'pve-configid',
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	type => 'object',
>> +    },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $jobs = cfs_read_file('jobs.cfg');
>> +	my $id = $param->{id};
>> +	my $job = $jobs->{ids}->{$id};
>> +	return $job if $job && $job->{type} eq 'realmsync';
>> +
>> +	raise_param_exc({ id => "No such job '$id'" });
>> +
>> +    }});
>> +
>> +__PACKAGE__->register_method({
>> +    name => 'create_job',
>> +    path => '{id}',
>> +    method => 'POST',
>> +    protected => 1,
>> +    description => "Create new realm-sync job.",
>> +    permissions => {
>> +	description => "'Realm.AllocateUser' on '/access/realm/<realm>' and "
>> +	    ."'User.Modify' permissions to '/access/groups/'.",
>> +	check => [ 'and',
>> +	    ['perm', '/access/realm/{realm}', ['Realm.AllocateUser']],
>> +	    ['perm', '/access/groups', ['User.Modify']],
>> +	],
>> +    },
>> +    parameters => PVE::Jobs::RealmSync->createSchema(),
>> +    returns => { type => 'null' },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $id = extract_param($param, 'id');
>> +
>> +	cfs_lock_file('jobs.cfg', undef, sub {
>> +	    my $data = cfs_read_file('jobs.cfg');
>> +
>> +	    die "Job '$id' already exists\n"
>> +		if $data->{ids}->{$id};
>> +
>> +	    my $plugin = PVE::Job::Registry->lookup('realmsync');
>> +	    my $opts = $plugin->check_config($id, $param, 1, 1);
>> +
>> +	    my $realm = $opts->{realm};
>> +	    my $cfg = cfs_read_file('domains.cfg');
>> +
>> +	    raise_param_exc({ realm => "No such realm '$realm'" })
>> +		if !defined($cfg->{ids}->{$realm});
>> +
>> +	    my $realm_type = $cfg->{ids}->{$realm}->{type};
>> +	    raise_param_exc({ realm => "Only LDAP/AD realms can be synced." })
>> +		if $realm_type ne 'ldap' && $realm_type ne 'ad';
>> +
>> +	    $data->{ids}->{$id} = $opts;
>> +
>> +	    PVE::Jobs::create_job($id, 'realmsync');
>> +
>> +	    cfs_write_file('jobs.cfg', $data);
>> +	});
>> +	die "$@" if ($@);
>> +
>> +	return undef;
>> +    }});
>> +
>> +__PACKAGE__->register_method({
>> +    name => 'update_job',
>> +    path => '{id}',
>> +    method => 'PUT',
>> +    protected => 1,
>> +    description => "Update realm-sync job definition.",
>> +    permissions => {
>> +	description => "'Realm.AllocateUser' on '/access/realm/<realm>' and "
>> +	    ." 'User.Modify' permissions to '/access/groups/'.",
>> +	check => [ 'and',
>> +	    ['perm', '/access/realm/{realm}', ['Realm.AllocateUser']],
>> +	    ['perm', '/access/groups', ['User.Modify']],
>> +	],
>> +    },
>> +    parameters => PVE::Jobs::RealmSync->updateSchema(),
>> +    returns => { type => 'null' },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $id = extract_param($param, 'id');
>> +	my $delete = extract_param($param, 'delete');
>> +	if ($delete) {
>> +	    $delete = [PVE::Tools::split_list($delete)];
>> +	}
>> +
>> +	cfs_lock_file('jobs.cfg', undef, sub {
>> +	    my $jobs = cfs_read_file('jobs.cfg');
>> +
>> +	    die "no options specified\n" if !scalar(keys %$param);
>> +
>> +	    my $plugin = PVE::Job::Registry->lookup('realmsync');
>> +	    my $opts = $plugin->check_config($id, $param, 0, 1);
>> +
>> +	    my $job = $jobs->{ids}->{$id};
>> +	    die "no such realmsync job\n" if !$job || $job->{type} ne 'realmsync';
>> +
>> +	    my $options = $plugin->options();
>> +	    for my $k (@$delete) {
>> +		my $d = $options->{$k} || die "no such option '$k'\n";
>> +		die "unable to delete required option '$k'\n" if !$d->{optional};
>> +		die "unable to delete fixed option '$k'\n" if $d->{fixed};
>> +		die "cannot set and delete property '$k' at the same time!\n"
>> +			if defined($opts->{$k});
>> +		delete $job->{$k};
>> +	    }
> 
> this seems something that we have a few times already in some form, might want to add this
> as helper somehere, possibly to section config.

yes, i'll see that i add a helper there for that

> 
>> +
>> +	    $job->{$_} = $param->{$_} for keys $param->%*;
>> +
>> +	    cfs_write_file('jobs.cfg', $jobs);
>> +
>> +	    PVE::Jobs::detect_changed_runtime_props($id, 'realmsync', $job);
>> +
>> +	    return;
>> +	});
>> +	die "$@" if ($@);
>> +    }});
>> +
>> +
>> +__PACKAGE__->register_method({
>> +    name => 'delete_job',
>> +    path => '{id}',
>> +    method => 'DELETE',
>> +    description => "Delete realm-sync job definition.",
>> +    permissions => {
>> +	check => ['perm', '/', ['Sys.Modify']],
>> +    },
>> +    protected => 1,
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    id => {
>> +		type => 'string',
>> +		format => 'pve-configid',
>> +	    },
>> +	},
>> +    },
>> +    returns => { type => 'null' },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $id = $param->{id};
>> +
>> +	cfs_lock_file('jobs.cfg', undef, sub {
>> +	    my $jobs = cfs_read_file('jobs.cfg');
>> +
>> +	    if (!defined($jobs->{ids}->{$id}) || $jobs->{ids}->{$id}->{type} ne 'realmsync') {
>> +		raise_param_exc({ id => "No such job '$id'" });
>> +	    }
>> +	    delete $jobs->{ids}->{$id};
>> +
>> +	    PVE::Jobs::remove_job($id, 'realmsync');> +
>> +	    cfs_write_file('jobs.cfg', $jobs);
>> +	    unlink "/etc/pve/priv/jobs/realmsync-$id.json";
> 
> should be contained in a sub, either save_state($id, undef) or explicit delete_state one (names
> are just examples)
> 
>> +	});
>> +	die "$@" if $@;
>> +
>> +	return undef;
>> +    }});
>> +
>> +1;
>> diff --git a/src/PVE/API2/AccessControl/Makefile b/src/PVE/API2/AccessControl/Makefile
>> new file mode 100644
>> index 0000000..8d3fb58
>> --- /dev/null
>> +++ b/src/PVE/API2/AccessControl/Makefile
>> @@ -0,0 +1,9 @@
>> +API2_SOURCES= 	\
>> +	Job.pm	\
>> +
>> +SUBDIRS=Job
>> +
>> +.PHONY: install
>> +install:
>> +	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/AccessControl/$$i; done
>> +	set -e && for i in ${SUBDIRS}; do ${MAKE} -C $$i $@; done
>> diff --git a/src/PVE/API2/Makefile b/src/PVE/API2/Makefile
>> index 2817f48..7d2be93 100644
>> --- a/src/PVE/API2/Makefile
>> +++ b/src/PVE/API2/Makefile
>> @@ -2,6 +2,7 @@
>>   API2_SOURCES= 		 	\
>>   	AccessControl.pm 	\
>>   	Domains.pm	 	\
>> +	RealmSync.pm	 	\
>>   	ACL.pm		 	\
>>   	Role.pm		 	\
>>   	Group.pm	 	\
>> @@ -9,6 +10,9 @@ API2_SOURCES= 		 	\
>>   	TFA.pm			\
>>   	OpenId.pm
>>   
>> +SUBDIRS=AccessControl
>> +
>>   .PHONY: install
>>   install:
>>   	for i in ${API2_SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/API2/$$i; done
>> +	set -e && for i in ${SUBDIRS}; do ${MAKE} -C $$i $@; done
>> diff --git a/src/PVE/Jobs/Makefile b/src/PVE/Jobs/Makefile
>> new file mode 100644
>> index 0000000..9eed1b2
>> --- /dev/null
>> +++ b/src/PVE/Jobs/Makefile
>> @@ -0,0 +1,6 @@
>> +SOURCES=RealmSync.pm
>> +
>> +.PHONY: install
>> +install: ${SOURCES}
>> +	install -d -m 0755 ${DESTDIR}${PERLDIR}/PVE/Jobs
>> +	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Jobs/$$i; done
>> diff --git a/src/PVE/Jobs/RealmSync.pm b/src/PVE/Jobs/RealmSync.pm
>> new file mode 100644
>> index 0000000..e8a86e3
>> --- /dev/null
>> +++ b/src/PVE/Jobs/RealmSync.pm
>> @@ -0,0 +1,193 @@
>> +package PVE::Jobs::RealmSync;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use JSON;
>> +
>> +use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::Cluster;
>> +use PVE::CalendarEvent;
>> +use PVE::Tools;
>> +
>> +use PVE::API2::Domains;
>> +
>> +use base qw(PVE::Job::Registry);
>> +
>> +sub type {
>> +    return 'realmsync';
>> +}
>> +
>> +my $props = get_standard_option('realm-sync-options', {
>> +    realm => get_standard_option('realm'),
>> +});
>> +
>> +sub properties {
>> +    return $props;
>> +}
>> +
>> +sub options {
>> +    my $options = {
>> +	enabled => { optional => 1 },
>> +	schedule => {},
>> +	comment => { optional => 1 },
>> +	node => { optional => 1 },
> 
> might want to drop that for now (as commented above in the api return schema)
> 
>> +	scope => {},
>> +    };
>> +    for my $opt (keys %$props) {
>> +	next if defined($options->{$opt});
>> +	if ($props->{$opt}->{optional}) {
>> +	    $options->{$opt} = { optional => 1 };
>> +	} else {
>> +	    $options->{$opt} = {};
>> +	}
>> +    }
>> +    $options->{realm}->{fixed} = 1;
>> +
>> +    return $options;
>> +}
>> +
>> +sub decode_value {
>> +    my ($class, $type, $key, $value) = @_;
>> +    return $value;
>> +}
>> +
>> +sub encode_value {
>> +    my ($class, $type, $key, $value) = @_;
>> +    return $value;
>> +}
>> +
>> +sub createSchema {
>> +    my ($class, $skip_type) = @_;
> 
> add extra newline here
> 
>> +    my $schema = $class->SUPER::createSchema($skip_type);
>> +
>> +    my $opts = $class->options();
>> +    for my $opt (keys $schema->{properties}->%*) {
>> +	next if defined($opts->{$opt}) || $opt eq 'id';
>> +	delete $schema->{properties}->{$opt};
>> +    }
>> +
>> +    # remove legacy props
> 
> bit confused, we never had sync jobs, so why should it matter?

we reuse the schema from the existing realm sync (where these
legacy options exist), but better would be to filter that out
in the 'options' sub, so that it never reaches it here

> 
>> +    delete $schema->{properties}->{full};
>> +    delete $schema->{properties}->{purge};
>> +
>> +    return $schema;
>> +}
>> +
>> +sub updateSchema {
>> +    my ($class, $skip_type) = @_;
>> +    my $schema = $class->SUPER::updateSchema($skip_type);
>> +
>> +    my $opts = $class->options();
>> +    for my $opt (keys $schema->{properties}->%*) {
>> +	next if defined($opts->{$opt});
>> +	next if $opt eq 'id' || $opt eq 'delete';
>> +	delete $schema->{properties}->{$opt};
>> +    }
>> +
>> +    # remove legacy props
> 
> same here
> 
>> +    delete $schema->{properties}->{full};
>> +    delete $schema->{properties}->{purge};
>> +
>> +    return $schema;
>> +}
>> +
>> +sub run {
>> +    my ($class, $conf, $id, $schedule) = @_;
>> +
>> +    my $node = $conf->{node};
>> +    for my $opt (keys %$conf) {
>> +	delete $conf->{$opt} if !defined($props->{$opt});
>> +    }
>> +
>> +    my $realm = $conf->{realm};
>> +    my $statedir = "/etc/pve/priv/jobs";
>> +    my $statefile = "$statedir/realmsync-$id.json";
> 
> s/realmsync/realm-sync/
> 
>> +
>> +    if (!defined($node)) {
>> +	# cluster synced
>> +	my $now = time();
>> +	my $nodename = PVE::INotify::nodename();
>> +
>> +	# check statefile in pmxcfs if we should start
>> +	my $shouldrun = PVE::Cluster::cfs_lock_domain($realm, undef, sub {
> 
> erm, using just the $realm as lock ID can go quite bad, e.g., if a realm is named
> "ha" it would interfere with the HA domain locks. Can we prefix this, or just use
> the general, non-realm specific "realm-sync"? Not much downside in practice and
> adding things like groups might be better, even if just for auditability, if not done
> in parallel anyway?
> 
> In anyway, a domain lock must never be (just) an $id.


ok sorry, i obviously misinterpreted the sub name, i thought it was for locking
'domains' (which we often use instead of 'realms') because it was
right below the 'cfs_lock_storage' sub in pve-cluster

sure, just using 'realm-sync' probably works (conflicting sync jobs must wait
for the the others, but that shouln't take too long)

> 
> 
>> +	    mkdir $statedir;
>> +	    my $raw = eval { PVE::Tools::file_get_contents($statefile) } // '';
> 
> 
> a non-node specific kv-store might be really nice, would avoid some DB/disk write
> churn (note that this specific one is adding that much extra churn, but in sum
> we start to have now quite a few places where it would help, but can me  moved to
> such a thing later on too)

i can look into that

> 
>> +	    my $members = PVE::Cluster::get_members();
>> +
>> +	    my $state = ($raw =~ m/^(\{.*\})$/) ? decode_json($1) : {};
>> +	    my $lastnode = $state->{node} // $nodename;
>> +	    my $lastupid = $state->{upid};
>> +	    my $lasttime = $state->{time};
> 
> nit: please stay consistent with variable name style (snake_case vs noneatall - prefer the
> former, at least if not for things where we used nostyle since pretty much always, e.g.,
> nodename, albeit just $node is often fitting for that anyway)
> 
>> +
>> +	    my $node_online = 0;
> 
> should this be $last_node_online?

yeah probably better

> 
>> +	    if ($lastnode ne $nodename) {
>> +		if (defined(my $member = $members->{$lastnode})) {
>> +		    $node_online = $member->{online};
>> +		}
>> +	    } else {
>> +		$node_online = 1;
>> +	    }
> 
> could be way shorter (untested though):
>   
> my $node_online = $last_node eq $nodename || ($members->{$last_node} // {})->{online};
> 
>> +
>> +	    if (defined($lastupid)) {
>> +		# first check if the next run is scheduled
>> +		if (my $parsed = PVE::Tools::upid_decode($lastupid, 1)) {
>> +		    my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);
> 
> same here w.r.t. variable naming style
> 
>> +		    my $next_sync = PVE::CalendarEvent::compute_next_event($calspec, $parsed->{starttime});
>> +		    return 0 if !defined($next_sync) || $now < $next_sync; # not yet its (next) turn
>> +		}
>> +		# check if still running and node is online
>> +		my $tasks = PVE::Cluster::get_tasklist();
>> +		for my $task (@$tasks) {
>> +		    next if $task->{upid} ne $lastupid;
>> +		    last if defined($task->{endtime}); # it's already finished
>> +		    last if !$node_online; # it's not finished but the node is offline
> 
> s/but/and/ ?

does not make really a difference in semantics here? but sure

> 
>> +		    return 0; # not finished and online
>> +		}
>> +	    } elsif (defined($lasttime) && ($lasttime+60) > $now && $node_online) {
>> +		# another node started in the last 60 seconds and is online
> 
> "another node started this job in the last 60 seconds and is still online"
> 
>> +		return 0;
>> +	    }
>> +
>> +	    # any of the following conditions should be true here:
>> +	    # * it was started on another node but that node is offline now
>> +	    # * it was started but either too long ago, or with an error
>> +	    # * the started task finished
>> +
>> +	    PVE::Tools::file_set_contents($statefile, encode_json({
>> +		node => $nodename,
>> +		time => $now,
>> +	    }));
> 
> I would slightly prefer to move this into a (just an example name) save_state method,
> even if really small it's IMO better to contain it there, same for getting the state above.
> 
>> +	    return 1;
>> +	});
>> +	die $@ if $@;
>> +
>> +	if ($shouldrun) {
>> +	    my $upid = eval { PVE::API2::Domains->sync($conf) };
>> +	    my $err = $@;
>> +	    PVE::Cluster::cfs_lock_domain($realm, undef, sub {
> 
> same here w.r.t. wrongly used domain locking
> 
>> +		if ($err && !$upid) {
>> +		    PVE::Tools::file_set_contents($statefile, encode_json({
>> +			node => $nodename,
>> +			time => $now,
>> +			error => $err,
>> +		    }));
>> +		    die "$err\n";
>> +		}
>> +
>> +		PVE::Tools::file_set_contents($statefile, encode_json({
>> +		    node => $nodename,
>> +		    upid => $upid,
>> +		}));
> 
> same here with state saving
> 
>> +	    });
>> +	    die $@ if $@;
>> +	    return $upid;
>> +	}
>> +
>> +	return "OK"; # all other cases should not run the sync on this node
> 
> possibly stupid question, but why return a string here?

thats currently the interface of the Jobs handler, was derived from
starting vzdump backups where the api returns that if the call
did not actually run when e.g. the vmid filter only selected
vms on a different node

we can change it there of course, but then we have to introduce some logic
there to filter out the OK on job run (not a problem, it just moves
the handling into the plugin)

> 
>> +    }
>> +
>> +    return PVE::API2::Domains->sync($conf);
>> +}
>> +
>> +1;
>> diff --git a/src/PVE/Makefile b/src/PVE/Makefile
>> index c839d8f..dfc5314 100644
>> --- a/src/PVE/Makefile
>> +++ b/src/PVE/Makefile
>> @@ -8,3 +8,4 @@ install:
>>   	install -D -m 0644 TokenConfig.pm ${DESTDIR}${PERLDIR}/PVE/TokenConfig.pm
>>   	make -C API2 install
>>   	make -C CLI install
>> +	make -C Jobs install
> 

[pve-devel] [PATCH widget-toolkit v2 1/1] RealmComboBox: add custom store filters for callers

[Dominik Csapak]

so that a user can filter the underlying store, e.g. for type

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/form/RealmComboBox.js | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/form/RealmComboBox.js b/src/form/RealmComboBox.js
index 5f61687..c57b52d 100644
--- a/src/form/RealmComboBox.js
+++ b/src/form/RealmComboBox.js
@@ -6,7 +6,12 @@ Ext.define('Proxmox.form.RealmComboBox', {
 	xclass: 'Ext.app.ViewController',
 
 	init: function(view) {
-	    view.store.on('load', this.onLoad, view);
+	    let store = view.getStore();
+	    if (view.storeFilter) {
+		store.setFilters(view.storeFilter);
+	    }
+	    store.on('load', this.onLoad, view);
+	    store.load();
 	},
 
 	onLoad: function(store, records, success) {
@@ -27,6 +32,9 @@ Ext.define('Proxmox.form.RealmComboBox', {
 	},
     },
 
+    // define custom filters for the underlying store
+    storeFilter: undefined,
+
     fieldLabel: gettext('Realm'),
     name: 'realm',
     queryMode: 'local',
@@ -52,6 +60,6 @@ Ext.define('Proxmox.form.RealmComboBox', {
 
     store: {
 	model: 'pmx-domains',
-	autoLoad: true,
+	autoLoad: false,
     },
 });
-- 
2.30.2

[pve-devel] [PATCH manager v2 1/3] ui: realm: sync: don't use realm defaults for remove-vanished

[Dominik Csapak]

if we don't manually set it to the empty string, it will use the
realm default, which might be unexpected. this way, the sync always
does what the user saw in the sync window.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/SyncWindow.js | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/www/manager6/dc/SyncWindow.js b/www/manager6/dc/SyncWindow.js
index c46dd7017..0ac109196 100644
--- a/www/manager6/dc/SyncWindow.js
+++ b/www/manager6/dc/SyncWindow.js
@@ -42,6 +42,8 @@ Ext.define('PVE.dc.SyncWindow', {
 	    });
 	    if (vanished_opts.length > 0) {
 		params['remove-vanished'] = vanished_opts.join(';');
+	    } else {
+		params['remove-vanished'] = 'none';
 	    }
 
 	    params['dry-run'] = is_preview ? 1 : 0;
-- 
2.30.2

Re: [pve-devel] [PATCH manager v2 1/3] ui: realm: sync: don't use realm defaults for remove-vanished

[Dominik Csapak]

On 12/6/22 12:06, Dominik Csapak wrote:
> if we don't manually set it to the empty string, it will use the
> realm default, which might be unexpected. this way, the sync always
> does what the user saw in the sync window.
> 

forgot to fix the commit message, should be

"if we don't manually set it to 'none'"

[pve-devel] applied: [PATCH manager v2 1/3] ui: realm: sync: don't use realm defaults for remove-vanished

[Thomas Lamprecht]

Am 06/12/2022 um 12:06 schrieb Dominik Csapak:
> if we don't manually set it to the empty string, it will use the
> realm default, which might be unexpected. this way, the sync always
> does what the user saw in the sync window.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/dc/SyncWindow.js | 2 ++
>  1 file changed, 2 insertions(+)
> 
>

applied this one already with fixed up commit message, thanks!

Re: [pve-devel] applied: [PATCH manager v2 1/3] ui: realm: sync: don't use realm defaults for remove-vanished

[Dominik Csapak]

On 12/13/22 15:15, Thomas Lamprecht wrote:
> Am 06/12/2022 um 12:06 schrieb Dominik Csapak:
>> if we don't manually set it to the empty string, it will use the
>> realm default, which might be unexpected. this way, the sync always
>> does what the user saw in the sync window.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   www/manager6/dc/SyncWindow.js | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>>
> 
> applied this one already with fixed up commit message, thanks!

seems i forgot to mention here in the patch that this requires the
1/2 of access-control to work, not a problem if we apply that one
too and bump accordingly before bumping pve-manager

[pve-devel] [PATCH manager v2 2/3] Jobs: add RealmSync Plugin and register it

[Dominik Csapak]

so that realmsync jobs get executed

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Jobs.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
index 86ce9f693..5636844a7 100644
--- a/PVE/Jobs.pm
+++ b/PVE/Jobs.pm
@@ -7,9 +7,11 @@ use JSON;
 use PVE::Cluster qw(cfs_lock_file cfs_read_file cfs_register_file);
 use PVE::Job::Registry;
 use PVE::Jobs::VZDump;
+use PVE::Jobs::RealmSync;
 use PVE::Tools;
 
 PVE::Jobs::VZDump->register();
+PVE::Jobs::RealmSync->register();
 PVE::Job::Registry->init();
 
 cfs_register_file(
-- 
2.30.2

[pve-devel] [PATCH manager v2 3/3] ui: add Realm Sync panel

[Dominik Csapak]

a typical CRUD panel for adding/editing/removing realm sync jobs

the edit window has the typical job options like enabled, schedule, etc.
and the sync options from the existing sync window

we explicitely load the defaults on create (but not on update) when the
realm changes.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/Makefile           |   1 +
 www/manager6/dc/Config.js       |   7 +
 www/manager6/dc/RealmSyncJob.js | 380 ++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 www/manager6/dc/RealmSyncJob.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 9786337bb..c005b7ea9 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -164,6 +164,7 @@ JSSRC= 							\
 	dc/MetricServerView.js				\
 	dc/UserTagAccessEdit.js				\
 	dc/RegisteredTagsEdit.js			\
+	dc/RealmSyncJob.js				\
 	lxc/CmdMenu.js					\
 	lxc/Config.js					\
 	lxc/CreateWizard.js				\
diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
index 13ded12e8..72a9bec13 100644
--- a/www/manager6/dc/Config.js
+++ b/www/manager6/dc/Config.js
@@ -140,6 +140,13 @@ Ext.define('PVE.dc.Config', {
 		iconCls: 'fa fa-address-book-o',
 		itemId: 'domains',
 	    },
+	    {
+		xtype: 'pveRealmSyncJobView',
+		title: gettext('Realm Sync'),
+		groups: ['permissions'],
+		iconCls: 'fa fa-refresh',
+		itemId: 'realmsyncjobs',
+	    },
 	    {
 		xtype: 'pveHAStatus',
 		title: 'HA',
diff --git a/www/manager6/dc/RealmSyncJob.js b/www/manager6/dc/RealmSyncJob.js
new file mode 100644
index 000000000..9fc77fc5c
--- /dev/null
+++ b/www/manager6/dc/RealmSyncJob.js
@@ -0,0 +1,380 @@
+Ext.define('PVE.dc.RealmSyncJobView', {
+    extend: 'Ext.grid.Panel',
+    alias: 'widget.pveRealmSyncJobView',
+
+    stateful: true,
+    stateId: 'grid-realmsyncjobs',
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	addRealmSyncJob: function(button) {
+	    let me = this;
+	    Ext.create(`PVE.dc.RealmSyncJobEdit`, {
+		autoShow: true,
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	},
+
+	editRealmSyncJob: function() {
+	    let me = this;
+	    let view = me.getView();
+	    let selection = view.getSelection();
+	    if (!selection || selection.length < 1) {
+		return;
+	    }
+
+	    Ext.create(`PVE.dc.RealmSyncJobEdit`, {
+		jobid: selection[0].data.id,
+		autoShow: true,
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	},
+
+	reload: function() {
+	    this.getView().getStore().load();
+	},
+    },
+
+    store: {
+	autoLoad: true,
+	id: 'realm-syncs',
+	proxy: {
+	    type: 'proxmox',
+	    url: '/api2/json/access/jobs/realm-sync',
+	},
+    },
+
+    columns: [
+	{
+	    header: gettext('Enabled'),
+	    width: 80,
+	    dataIndex: 'enabled',
+	    xtype: 'checkcolumn',
+	    sortable: true,
+	    disabled: true,
+	    disabledCls: 'x-item-enabled',
+	    stopSelection: false,
+	},
+	{
+	    text: gettext('Name'),
+	    flex: 1,
+	    dataIndex: 'id',
+	    hidden: true,
+	},
+	{
+	    header: gettext('Node'),
+	    width: 100,
+	    sortable: true,
+	    dataIndex: 'node',
+	    renderer: value => value || gettext('Any'),
+	},
+	{
+	    text: gettext('Realm'),
+	    width: 200,
+	    dataIndex: 'realm',
+	},
+	{
+	    header: gettext('Schedule'),
+	    width: 150,
+	    dataIndex: 'schedule',
+	},
+	{
+	    text: gettext('Next Run'),
+	    dataIndex: 'next-run',
+	    width: 150,
+	    renderer: PVE.Utils.render_next_event,
+	},
+	{
+	    header: gettext('Comment'),
+	    dataIndex: 'comment',
+	    renderer: Ext.htmlEncode,
+	    sorter: (a, b) => (a.data.comment || '').localeCompare(b.data.comment || ''),
+	    flex: 1,
+	},
+    ],
+
+    tbar: [
+	{
+	    text: gettext('Add'),
+	    handler: 'addRealmSyncJob',
+	},
+	{
+	    text: gettext('Edit'),
+	    xtype: 'proxmoxButton',
+	    handler: 'editRealmSyncJob',
+	    disabled: true,
+	},
+	{
+	    xtype: 'proxmoxStdRemoveButton',
+	    baseurl: `/api2/extjs/access/jobs/realm-sync`,
+	    callback: 'reload',
+	},
+    ],
+
+    listeners: {
+	itemdblclick: 'editRealmSyncJob',
+    },
+
+    initComponent: function() {
+	var me = this;
+
+	me.callParent();
+
+	Proxmox.Utils.monStoreErrors(me, me.getStore());
+    },
+});
+
+Ext.define('PVE.dc.RealmSyncJobEdit', {
+    extend: 'Proxmox.window.Edit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    subject: gettext('Realm Sync Job'),
+    onlineHelp: 'pveum_ldap_sync',
+
+    // don't focus the schedule field on edit
+    defaultFocus: 'field[name=id]',
+
+    cbindData: function() {
+	let me = this;
+	me.isCreate = !me.jobid;
+	me.jobid = me.jobid || "";
+	let url = '/api2/extjs/access/jobs/realm-sync';
+	me.url = me.jobid ? `${url}/${me.jobid}` : url;
+	me.method = me.isCreate ? 'POST' : 'PUT';
+	if (!me.isCreate) {
+	    me.subject = `${me.subject}: ${me.jobid}`;
+	}
+	return {};
+    },
+
+    submitUrl: function(url, values) {
+	return this.isCreate ? `${url}/${values.id}` : url;
+    },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	updateDefaults: function(_field, newValue) {
+	    let me = this;
+	    // only update on create
+	    if (!me.getView().isCreate) {
+		return;
+	    }
+	    Proxmox.Utils.API2Request({
+		url: `/access/domains/${newValue}`,
+		success: function(response) {
+		    // first reset the fields to their default
+		    ['acl', 'entry', 'properties'].forEach(opt => {
+			me.lookup(`remove-vanished-${opt}`)?.setValue(false);
+		    });
+		    me.lookup('enable-new')?.setValue('1');
+		    me.lookup('scope')?.setValue(undefined);
+
+		    let options = response?.result?.data?.['sync-defaults-options'];
+		    if (options) {
+			let parsed = PVE.Parser.parsePropertyString(options);
+			if (parsed['remove-vanished']) {
+			    let opts = parsed['remove-vanished'].split(';');
+			    for (const opt of opts) {
+				me.lookup(`remove-vanished-${opt}`)?.setValue(true);
+			    }
+			    delete parsed['remove-vanished'];
+			}
+			for (const [name, value] of Object.entries(parsed)) {
+			    me.lookup(name)?.setValue(value);
+			}
+		    }
+		},
+	    });
+	},
+    },
+
+    items: [
+	{
+	    xtype: 'inputpanel',
+
+	    cbind: {
+		isCreate: '{isCreate}',
+	    },
+
+	    onGetValues: function(values) {
+		let me = this;
+
+		let vanished_opts = [];
+		['acl', 'entry', 'properties'].forEach((prop) => {
+		    if (values[`remove-vanished-${prop}`]) {
+			vanished_opts.push(prop);
+		    }
+		    delete values[`remove-vanished-${prop}`];
+		});
+
+		if (!values.id && me.isCreate) {
+		    values.id = 'realmsync-' + Ext.data.identifier.Uuid.Global.generate().slice(0, 13);
+		}
+
+		if (vanished_opts.length > 0) {
+		    values['remove-vanished'] = vanished_opts.join(';');
+		} else {
+		    values['remove-vanished'] = 'none';
+		}
+
+		PVE.Utils.delete_if_default(values, 'node', '');
+
+		if (me.isCreate) {
+		    delete values.delete; // on create we cannot delete values
+		}
+
+		return values;
+	    },
+
+	    column1: [
+		{
+		    xtype: 'pmxDisplayEditField',
+		    editConfig: {
+			xtype: 'pmxRealmComboBox',
+			storeFilter: rec => rec.data.type === 'ldap' || rec.data.type === 'ad',
+		    },
+		    cbind: {
+			editable: '{isCreate}',
+		    },
+		    listeners: {
+			change: 'updateDefaults',
+		    },
+		    fieldLabel: gettext('Realm'),
+		    name: 'realm',
+		    reference: 'realm',
+		},
+		{
+		    xtype: 'pveCalendarEvent',
+		    fieldLabel: gettext('Schedule'),
+		    allowBlank: false,
+		    name: 'schedule',
+		    reference: 'schedule',
+		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    fieldLabel: gettext('Enable'),
+		    name: 'enabled',
+		    reference: 'enabled',
+		    uncheckedValue: 0,
+		    defaultValue: 1,
+		    checked: true,
+		},
+	    ],
+
+	    column2: [
+		{
+		    xtype: 'pveNodeSelector',
+		    name: 'node',
+		    fieldLabel: gettext('Node'),
+		    allowBlank: true,
+		    editable: true,
+		    autoSelect: false,
+		    emptyText: '-- ' + gettext('Any') + ' --',
+		},
+		{
+		    xtype: 'proxmoxKVComboBox',
+		    name: 'scope',
+		    reference: 'scope',
+		    fieldLabel: gettext('Scope'),
+		    value: '',
+		    emptyText: gettext('No default available'),
+		    deleteEmpty: false,
+		    allowBlank: false,
+		    comboItems: [
+			['users', gettext('Users')],
+			['groups', gettext('Groups')],
+			['both', gettext('Users and Groups')],
+		    ],
+		},
+		{
+		    xtype: 'proxmoxKVComboBox',
+		    value: '1',
+		    deleteEmpty: false,
+		    allowBlank: false,
+		    comboItems: [
+			['1', Proxmox.Utils.yesText],
+			['0', Proxmox.Utils.noText],
+		    ],
+		    name: 'enable-new',
+		    reference: 'enable-new',
+		    fieldLabel: gettext('Enable new'),
+		},
+	    ],
+
+	    columnB: [
+		{
+		    xtype: 'fieldset',
+		    title: gettext('Remove Vanished Options'),
+		    items: [
+			{
+			    xtype: 'proxmoxcheckbox',
+			    fieldLabel: gettext('ACL'),
+			    name: 'remove-vanished-acl',
+			    reference: 'remove-vanished-acl',
+			    boxLabel: gettext('Remove ACLs of vanished users and groups.'),
+			},
+			{
+			    xtype: 'proxmoxcheckbox',
+			    fieldLabel: gettext('Entry'),
+			    name: 'remove-vanished-entry',
+			    reference: 'remove-vanished-entry',
+			    boxLabel: gettext('Remove vanished user and group entries.'),
+			},
+			{
+			    xtype: 'proxmoxcheckbox',
+			    fieldLabel: gettext('Properties'),
+			    name: 'remove-vanished-properties',
+			    reference: 'remove-vanished-properties',
+			    boxLabel: gettext('Remove vanished properties from synced users.'),
+			},
+		    ],
+		},
+		{
+		    xtype: 'proxmoxtextfield',
+		    name: 'comment',
+		    fieldLabel: gettext('Job Comment'),
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+		    },
+		    autoEl: {
+			tag: 'div',
+			'data-qtip': gettext('Description of the job'),
+		    },
+		},
+		{
+		    xtype: 'displayfield',
+		    reference: 'defaulthint',
+		    value: gettext('Default sync options can be set by editing the realm.'),
+		    userCls: 'pmx-hint',
+		    hidden: true,
+		},
+	    ],
+	},
+    ],
+
+    initComponent: function() {
+	let me = this;
+	me.callParent();
+	if (me.jobid) {
+	    me.load({
+		success: function(response, options) {
+		    let values = response.result.data;
+
+		    if (values['remove-vanished']) {
+			let opts = values['remove-vanished'].split(';');
+			for (const opt of opts) {
+			    values[`remove-vanished-${opt}`] = 1;
+			}
+		    }
+		    me.down('inputpanel').setValues(values);
+		},
+	    });
+	}
+    },
+});
-- 
2.30.2