Design for importing hook config file in PTL

Are you sure? for mom hooks, shouldn’t they be inside mom_priv of the host running the mom daemon?

Hi Ravi,

Thanks for bringing up this, I have added an extra API in mom class where we can call modify_hook_config from mom class.

I disagree, it should all be in the server class. Hooks are cluster-wide, it doesn’t make sense for the MoM class to have a modify hook config, since it will be a global change.

Instead, use the server class, and export the configuration so you can edit it. For example, to print the cgroups configuration to stdout: qmgr -c "export hook pbs_cgroups application/x-config default"

This is the documented way of editing a hook configuration file.

My bad, I didn’t know that mom hooks are also cluster-wide, it does make sense that they are now that I think about it.
@shilpakodli sorry for misleading you, just the one interface in server should be enough.

@vstumpf Thanks for the info. I have reverted my changes to old one. Please have a look at it @agrawalravi90

Looks fine to me, thanks.

Thanks everyone for the Sign off.

@shilpakodli and All,
The design looks good, but I think we need to follow the documented way of editing the hook configuration file, whose methods are not present in PTL right now. I see it this way:
New methods needed:
export_hook_config()
import_hook_config()
modify_hook_config() can then use these methods and modify the hook config. Please suggest your views on it.

@saritakh Thanks for coming up with this Sarita.
Yes, you are absolutely correct. We need to have different API for “export_hook_config” as it gives hook config output. I have done some debugging on server.manager method where it returns either True or False and we cannot include export hook functionality in “manager” method as we need to return output of “export hook config file”.
It is better to have different methods for import and export which can be used in test scripts as well.

@Reviewers, please post your views on this.

@shilpakodli : Since PBS supports different format for hook config file(json, ini , xml…etc). User has better information about config file. Instead of PTL handling add/delete/modify of config entries, I suggest lets have only 2 APIs.

  1. import_hook_config
  2. export_hook_config

So let user do import of complete updated config whenever he wants to modify any add/modify/delete hook config entries.

Thanks @kjakkali for the suggestion.
I have modified EDD for the same. Please re-review EDD and let me know if any changes need to be done.

@visheshh @RKORavi @agrawalravi90 Please revisit EDD page and let me know your thoughts on this.

Sorry for the delay on this. Some comments:

  • import_hook_config takes a ‘body’ arg, but in the example, you’ve mentioned ‘attrs’, so i think you need to change the example.
  • instead of ‘import’ and ‘export’, how about ‘set’ and ‘get’ ? I think that’ll be more intuitive
  • it seems like import_hook_config raises an exception but export_hook_config doesn’t. I feel that we should make them consistent. Maybe make them both raise a new, hook related exception if they fail?

Thanks @agrawalravi90. I have addressed your review comments except renaming method names.
I feel import and export will sound more good as we are using same commands using qmgr.

Hi @shilpakodli,
Sorry for the delay,

Separate import and export as 2 different Interfaces in the design doc and have in examples in the sections accordingly.

Interface 1:
import
example
Interface 2:
export
example

Nitpicking : (body: str) should be type body.

Thanks,
Vishesh

hook_body = """

{

cpus : {

           enabled: True

           }

}"""

I’m not sure I understand this. Isn’t hook body supposed to be the contents of the hook? i.e - the python code of the hook ?

This is config hook body, not python hook body. We are overwriting contents of hook.CF file and not hook.PY file.

thanks for clarifying shilpa. I suggest that you rename ‘body’/‘hook_body’ to ‘config’/‘conf’/‘hook_conf’ to avoid confusion.

Thanks Vishesh for the review. I have addressed your review comments. Please have a look at it.

I have changed to hook_conf from hook_body. Thanks.