I suggest to add a comment attribute on the queue. We have the comment attribute on the node, on the job and even on the server. Queue is missing this attribute. I think it is very useful to add it on the queue too. It can be used for example for description of the queue, which can be very convenient, especially if you use many queues.
What do you think?
@vchlum Thanks for writing the document.
In your document, it would be helpful if you could add some more details about -
- Under what privileges one can set/unset this attribute?
- Who all will have permissions to read this attribute?
- Will this attribute be saved to the database?
- What is the default value of this attribute?
@arungrover OK, I added requested details into the document page.
@vchlum Thanks for updating the document page. Your design proposal looks good to me.
@vchlum This is something that I have wanted for a while. The design looks good. My feedback is that this should be settable by a PBS manager instead of administrator. The difference is that administrator requires you to be root to set it vs a pbs manager. The difference is that administrator can do everything where a pbs manager can do most everything except create/modify hooks.
If you make that change to your EDD I will sign off.
I agree with Jon, it must be settable by a manager and not an operator. Please have a look at the following design with respect to the format of the document: https://pbspro.atlassian.net/wiki/display/PD/PP-578%3A+server+memory+leak+for+hooks+load+test
Visibility should be specified as public, and change control should be stable.
I disagree that manager privilege is required to set the comment. The manager role is someone who sets policy. The operator role is someone who does the day to day maintenance of PBS. This strikes me as something the operator should be able to set.
In any case, the node comment is able to be set by the operator. Either way we go, we should be consistent between the two.
@mkaro I adjusted the documentation according to suggested page. Please have a look.
@mkaro , @bhroam I think it should be consistent with the node comment, which can be set by manager, operator and administrator. This is pretty the same case.
I added a statement indicating the attribute is a string and is settable by an operator or manager. I’m fine with the document as it stands now.
well @vchlum the document on confluence states that the attribute will get stored in the database but having “ATR_DFLAG_NOSAVM” flag wouldn’t let that happen when the attribute is modified, right?
Now that this is the same case with node comment too, I’d suggest that the document undergoes a change to reflect that or at least mention that attribute is not saved in db upon modification.
What do you think?
The queue comment attribute is defined exactly the same way as the node comment attribute. One would think that ATR_DFLAG_NOSAVM would prevent the attribute from being stored in the database. I performed a quick test by setting a comment on a node and restarting PBS, and the comment was still present after the restart. I’m not sure if this is a bug or by design, but the proposed changes are consistent with the existing node comment attribute.
@arungrover You are right. Documentation mentions this fact about the node comment (RG-386 and AG-65). I added similar statement to the page.
@mkaro It is not a bug. As I understand it means: If the attribute is explicitly set, it will not be modified by the server.
“ATF_DFLAG_NOSAVM” is used to restrict PBS server to not write a value of attribute to db every time it is modified.
@mkaro I think what this means is that the attribute is not preserved in db upon modification but when server is going down at that time it gets serialized into database.
Now when server starts and if someone modifies the attribute and after some time server goes down abruptly then the modified value can not be recovered.
@vchlum Since this a new attribute on queue. PBS server in it’s current form does not set any comment on queue by itself. What I meant was that if you can modify “Attribute is stored in the database.” to something like “All modifications done to comment are not saved in database, it is only saved to db when PBS server shuts down”
I had a look at the code where we modify the queue attribute, Unfortunately I could not find any place where we check for this flag before saving the modified queue into the db. We just save all of the queue attributes as soon as one of them is modified.
@arungrover Document modified.
@vchlum Thanks for making the change!
It looks good to me.
Yes @arungrover, that flag means the attribute might not get preserved, but in reality it gets saved in specific cases (as u mentioned), just no guarantees that it will be serialized.
@vchlum, your current design looks good to me.
@vchlum – Thanks for the enhancement!
A request and a suggestion: would you set the Target Version on the JIRA ticket (to 17.0) – it is really useful to maintain a coherent list of all changes that can easily be found using JIRA searches. (I believe we are using “17.0” to represent the current master branch; someone please correct if I am mistaken).
Also, I suggest the last sentence in the design on Confluence be deleted (as it slightly contradicts the prior text and doesn’t appear to add anything, and it will be helpful to be really clear when the PBS Pro doc people review it when putting together the docs for v17.)
@billnitzberg No problem. Done.