[crac] RFR: Allow users to pass new properties on restore

[Ashutosh Mehra]

On Thu, 28 Apr 2022 12:25:20 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> VM changes: To identify properties that can be modified on restore,
>> added a new bool field SystemProperty::_modifiable_on_restore.
>> All the jdk related properties are marked unmodifiable. Rest of the
>> properties are considered modifiable.
>> When the JVM is launched with -XX:CRaCRestoreFrom option, then the
>> properties prefixed with "-D" are maintained in a separate list in
>> Arguments::_system_properties_for_restore. This list is passed to the JVM
>> being restored by writing to a shared memory object.
>> When the JVM is restored, it reads the new properties from shared memory
>> object and updates its existing list of properties maintained in
>> Arguments::_system_properties.
>> 
>> JDK changes: System::props needs to be updated on restore to account for
>> new system properties. For this purpose j.l.System registers a new
>> JDKResource which queries new properties from the VM in afterRestore()
>> notification and updates System::props. The JDKResource registered by
>> j.l.System is given highest priority so it is the first resource to get
>> afterRestore() notification.
>> 
>> Signed-off-by: Ashutosh Mehra <asmehra at redhat.com>
>
>> VM changes: To identify properties that can be modified on restore, added a new bool field SystemProperty::_modifiable_on_restore. All the jdk-related properties are marked unmodifiable. The rest of the properties are considered modifiable.
> 
> On the java level, all properties can be changed with `System.setProperty`. Our property model is much closer to a program do e.g. after restore
> 
>         System.setProperty("sun.boot.library.path", "test");
> 
> There is no mechanism (except the deprecated SecurityManager) that prevents this from working, although no one promises the property value will be considered if set in this way. So I propose not to limit ourselves artificially. Who knows, maybe some JDK system property is OK to fix it on restore, e.g. if the checkpoint is done very early and a class that reads the property is not yet initialized.
> 
>> When the JVM is launched with -XX:CRaCRestoreFrom option, then the properties prefixed with "-D" are maintained in a separate list in Arguments::_system_properties_for_restore.
> 
> Have you considered how to make this change smaller? E.g adding an Explicit marker on the property? That's unfortunate that implicit properties are already added to the list before we get to the restore, and explicit properties cannot be distinguished from them. I'm fine with the change in Arguments::parse_each_vm_init_arg, but refactorings or adding a set of methods to manage the for_restore set is a bit overkill. Initializing a full VM just to be replaced by another restored one (as we do for now) does not make a lot of sense and needs fixing at some moment. So I'd like to avoid too many changes in the VM code that we'll have to revert.
> 
> Another way is to pass all properties from restoring VM to the one being restored, for simplicity and flexibility. And to filter properties in the being restored VM -- that will be the single point of responsibility. But here there is a drawback: VMs may have different versions, so implicit properties in the being restored VM will be implicitly overwritten.
> 
>> This list is passed to the JVM being restored by writing to a shared memory object. When the JVM is restored, it reads the new properties from shared memory object and updates its existing list of properties maintained in Arguments::_system_properties.
> 
> I could not find users of the _system_properties after VM was initialized and JDK pulled the set of properties from the VM. I think it's not necessary to do, just as a call to j.l.System.setProperty is not reflected in the VM.
> 
>> JDK changes: System::props needs to be updated on restore to account for new system properties. For this purpose j.l.System registers a new JDKResource which queries new properties from the VM in afterRestore() notification and updates System::props. The JDKResource registered by j.l.System is given highest priority so it is the first resource to get afterRestore() notification.
> 
> AFAIU, for the regular bootstrap procedure, "pulling" of properties is required as the VM does not know when j.l.System will be initialized, so instead, they are pulled at the j.l.System's initialization. But I think we can assume j.l.System is always initialized at the checkpoint, so we can avoid the resource, and make jdk.crac.Core just to set the new properties with the j.l.System.setProperty.

@AntonKozlov thanks for reviewing the patch.

> So I propose not to limit ourselves artificially. Who knows, maybe some JDK system property is OK to fix it on restore, e.g. if the checkpoint is done very early and a class that reads the property is not yet initialized.

My main intention of this patch is to allow users to specify different properties through command line when restoring the process. I am assuming in this scenario the user would want the new properties to have same effect as when starting a new JVM process. In this respect this mechanism is different from calling `System.setProperty()` for which, as you mentioned in your comment, no promise is made that they would be considered.
Now, for some jdk internal properties, we may be able to consider the new values, but I believe it would not be possible for every jdk internal property. This is the reason why I wanted to separate out the properties that can be modified from those which cannot be.

I intentionally chose to mark all jdk-related properties as unmodifiable because I am focusing mainly on application specific properties. However, this should not prevent us from marking a jdk-related property as modifiable in future, but it would have to be done on case-by-case basis as the need arises.
For example, if we want to allow the user to specify different directory for native libraries on restore, we can mark `java.library.path` as modifiable and handle its users at the Java level using the restore hooks.

> Have you considered how to make this change smaller? E.g adding an Explicit marker on the property? That's unfortunate that implicit properties are already added to the list before we get to the restore, and explicit properties cannot be distinguished from them. I'm fine with the change in Arguments::parse_each_vm_init_arg, but refactorings or adding a set of methods to manage the for_restore set is a bit overkill. Initializing a full VM just to be replaced by another restored one (as we do for now) does not make a lot of sense and needs fixing at some moment. So I'd like to avoid too many changes in the VM code that we'll have to revert.

> Another way is to pass all properties from restoring VM to the one being restored, for simplicity and flexibility. And to filter properties in the being restored VM -- that will be the single point of responsibility. But here there is a drawback: VMs may have different versions, so implicit properties in the being restored VM will be implicitly overwritten.

I agree this is an overkill. The reason why I didn't add an Explicit like marker is because it adds another bool field to each SystemProperty object, which has no use in the regular JVM. It will only be used in the initiating JVM, although I agree it would be a temporary change, provided we find another way to initiate the restore operation instead of using the full JVM.
I also feel adding explicit methods to handle properties for restore is much cleaner as it separates out the code for handling properties for restore operation from the regular processing. It makes it easier to follow code IMHO.

I don't have strong preference about either of these mechanisms, considering these are temporary changes and expected to be removed once we find a better mechanism to initiate restore operation. So if you feel adding Explicit marker is better I can introduce that change.

I also thought about passing all the properties and filtering them out in the restored VM, but dropped it for the reason you mentioned.

> I could not find users of the _system_properties after VM was initialized and JDK pulled the set of properties from the VM. I think it's not necessary to do, just as a call to j.l.System.setProperty is not reflected in the VM.

As I stated in my first comment, I believe this is exactly the difference between `System.getProperty()` and this mechanism of providing new properties on restore. We would want the VM view of the system properties to also be updated. Another reason for updating `_system_properties` is JVMTI api `GetSystemProperties()` which relies on `_system_properties`. After restore `GetSystemProperties()` should provide the system properties that take into account new properties specified on commnad line.

> But I think we can assume j.l.System is always initialized at the checkpoint, so we can avoid the resource, and make jdk.crac.Core just to set the new properties with the j.l.System.setProperty.

Fair point. I will make the change to update properties using `System.getProperty()` directly after restore from `jdk.crac.Core`.

-------------

PR: https://git.openjdk.java.net/crac/pull/21