Ticket 13562

Summary: Field 'max_tres_pj' doesn't have a default value
Product: Slurm Reporter: Kilian Cavalotti <kilian>
Component: DatabaseAssignee: Chad Vizino <chad>
Status: RESOLVED INFOGIVEN QA Contact:
Severity: 4 - Minor Issue    
Priority: --- CC: bas.vandervlies, mcmullan
Version: 21.08.5   
Hardware: Linux   
OS: Linux   
Site: Stanford Alineos Sites: ---
Atos/Eviden Sites: --- Confidential Site: ---
Coreweave sites: --- Cray Sites: ---
DS9 clusters: --- HPCnow Sites: ---
HPE Sites: --- IBM Sites: ---
NOAA SIte: --- OCF Sites: ---
Recursion Pharma Sites: --- SFW Sites: ---
SNIC sites: --- Linux Distro: ---
Machine Name: CLE Version:
Version Fixed: Target Release: ---
DevPrio: --- Emory-Cloud Sites: ---

Description Kilian Cavalotti 2022-03-03 19:35:51 MST
Hi SchedMD!

We recently upgraded our MariaDB version to 10.7, and it looks like since then, we can't delete associations anymore. 

A command like this results in the following error

$ sacctmgr delete userA pumiaoy where account=acctA
sacctmgr: accounting_storage/slurmdbd: acct_storage_p_remove_assocs: No error
 Nothing deleted

and this is what's logged by slurmdbd:


Mar 03 18:30:51 sh03-sl01.int slurmdbd[26481]: error: mysql_query failed: 1364 Field 'max_tres_pj' doesn't have a default value
                                               update "sherlock_assoc_table" as t1 set mod_time=1646361051, deleted=1, def_qos_id=DEFAULT, shares=DEFAULT, max_jobs=DEFAULT, max_jobs_accrue=DEFAULT, min_prio_thresh=DEFAULT, max_submit_jobs=DEFAULT, max_wall_pj=DEFAULT, max_tres_pj=DEFAULT, max_tres_pn=DEFAULT, max_tres_mins_pj=DEFAULT, max_tres_run_mins=DEFAULT, grp_jobs=DEFAULT, grp_submit_jobs=DEFAULT, grp_jobs_accrue=DEFAULT, grp_wall=DEFAULT, grp_tres=DEFAULT


Indeed, it looks like the default value for max_tres_pj in that table is NULL, but that NULL is not allowed:

MariaDB [slurm_acct_db]> desc sherlock_assoc_table;
+-------------------+---------------------+------+-----+---------+----------------+
| Field             | Type                | Null | Key | Default | Extra          |
+-------------------+---------------------+------+-----+---------+----------------+
| creation_time     | bigint(20) unsigned | NO   |     | NULL    |                |
| mod_time          | bigint(20) unsigned | NO   |     | 0       |                |
| deleted           | tinyint(4)          | NO   |     | 0       |                |
| is_def            | tinyint(4)          | NO   |     | 0       |                |
| id_assoc          | int(10) unsigned    | NO   | PRI | NULL    | auto_increment |
| user              | tinytext            | NO   | MUL | NULL    |                |
| acct              | tinytext            | NO   | MUL | NULL    |                |
| partition         | tinytext            | NO   |     | NULL    |                |
| parent_acct       | tinytext            | NO   |     | NULL    |                |
| lft               | int(11)             | NO   | MUL | NULL    |                |
| rgt               | int(11)             | NO   |     | NULL    |                |
| shares            | int(11)             | NO   |     | 1       |                |
| max_jobs          | int(11)             | YES  |     | NULL    |                |
| max_jobs_accrue   | int(11)             | YES  |     | NULL    |                |
| min_prio_thresh   | int(11)             | YES  |     | NULL    |                |
| max_submit_jobs   | int(11)             | YES  |     | NULL    |                |
| max_tres_pj       | text                | NO   |     | NULL    |                |
| max_tres_pn       | text                | NO   |     | NULL    |                |
| max_tres_mins_pj  | text                | NO   |     | NULL    |                |
| max_tres_run_mins | text                | NO   |     | NULL    |                |
| max_wall_pj       | int(11)             | YES  |     | NULL    |                |
| grp_jobs          | int(11)             | YES  |     | NULL    |                |
| grp_jobs_accrue   | int(11)             | YES  |     | NULL    |                |
| grp_submit_jobs   | int(11)             | YES  |     | NULL    |                |
| grp_tres          | text                | NO   |     | NULL    |                |
| grp_tres_mins     | text                | NO   |     | NULL    |                |
| grp_tres_run_mins | text                | NO   |     | NULL    |                |
| grp_wall          | int(11)             | YES  |     | NULL    |                |
| priority          | int(10) unsigned    | YES  |     | NULL    |                |
| def_qos_id        | int(11)             | YES  |     | NULL    |                |
| qos               | blob                | NO   |     | NULL    |                |
| delta_qos         | blob                | NO   |     | NULL    |                |
+-------------------+---------------------+------+-----+---------+----------------+
32 rows in set (0.001 sec)

Is it expected?

Thanks!
--
Kilian
Comment 1 Bas van der Vlies 2022-03-04 01:08:04 MST
Hi Kilian,

 we had the same problem see issue https://bugs.schedmd.com/show_bug.cgi?id=12947
for the resolution.

Regards

Bas
Comment 2 Kilian Cavalotti 2022-03-04 08:57:19 MST
Hi Bas,

(In reply to Bas van der Vlies from comment #1)
>  we had the same problem see issue
> https://bugs.schedmd.com/show_bug.cgi?id=12947
> for the resolution.

Ah yes, great, thank for pointing that out.

I updated our DB as suggested in bug #12947, and that indeed solved the problem, thank you!



Now, to SchedMD DB schema experts, the suggested fix was to set the default values on those fields to an empty string (''), like this:

ALTER TABLE <cluster>_assoc_table MODIFY max_tres_pj text DEFAULT '';

The other possibility is to allow NULL for the field and set the default value to NULL:

ALTER TABLE <cluster>_assoc_table MODIFY max_tres_pj text DEFAULT NULL NULL;

Not sure how the code handles default/empty values for those fields. Should they be set to empty strings or NULL values?

Cheers,
--
Kilian
Comment 3 Chad Vizino 2022-03-04 10:54:59 MST
(In reply to Kilian Cavalotti from comment #2)
> Now, to SchedMD DB schema experts, the suggested fix was to set the default
> values on those fields to an empty string (''), like this:
> 
> ALTER TABLE <cluster>_assoc_table MODIFY max_tres_pj text DEFAULT '';
> 
> The other possibility is to allow NULL for the field and set the default
> value to NULL:
> 
> ALTER TABLE <cluster>_assoc_table MODIFY max_tres_pj text DEFAULT NULL NULL;
> 
> Not sure how the code handles default/empty values for those fields. Should
> they be set to empty strings or NULL values?
Hi. The assoc table schema for max_tres_pj (and similar text fields) is listed in create_cluster_assoc_table() in src/plugins/accounting_storage/mysql/accounting_storage_mysql.c this way:

>{ "max_tres_pj", "text not null default ''" },
So using the first case you list would restore the table to the way it was supposedly created.

Interestingly, that schema is not new--it was introduced in Slurm 15.08 in 2015 (circa MariaDB 5.5):

>https://github.com/SchedMD/slurm/blame/a824918597908cc044b60c077561c2fec35796a7/src/plugins/accounting_storage/mysql/accounting_storage_mysql.c#L1183
It's not clear to me why the alter is necessary (in your and some others' case) but it seems to have something to do with the way the tables were created under an older db engine and apparently affected by subsequent engine upgrades.
Comment 4 Bas van der Vlies 2022-03-04 10:55:13 MST
I am on holidays from 4-Mar-2022 till 13-Mar-2022
Comment 5 Kilian Cavalotti 2022-03-04 11:07:48 MST
Hi Chad, 

(In reply to Chad Vizino from comment #3)
> Hi. The assoc table schema for max_tres_pj (and similar text fields) is
> listed in create_cluster_assoc_table() in
> src/plugins/accounting_storage/mysql/accounting_storage_mysql.c this way:
> 
> >{ "max_tres_pj", "text not null default ''" },
> So using the first case you list would restore the table to the way it was
> supposedly created.

Got it, thanks for pointing this out!

> Interestingly, that schema is not new--it was introduced in Slurm 15.08 in
> 2015 (circa MariaDB 5.5):
> 
> >https://github.com/SchedMD/slurm/blame/a824918597908cc044b60c077561c2fec35796a7/src/plugins/accounting_storage/mysql/accounting_storage_mysql.c#L1183
> It's not clear to me why the alter is necessary (in your and some others'
> case) but it seems to have something to do with the way the tables were
> created under an older db engine and apparently affected by subsequent
> engine upgrades.

Yes, I assume so.

It may be worth adding a check and a potential ALTER TABLE statement to check and update those default values in the DB conversion steps for the next major Slurm version?

Thanks!
--
Kilian
Comment 6 Chad Vizino 2022-03-04 12:58:30 MST
(In reply to Kilian Cavalotti from comment #5)
> It may be worth adding a check and a potential ALTER TABLE statement to
> check and update those default values in the DB conversion steps for the
> next major Slurm version?
That's a reasonable suggestion and we are considering it. What version of MariaDB did you upgrade from? The key for us is that we'd like to be able to reproduce the use case.
Comment 7 Kilian Cavalotti 2022-03-04 13:08:06 MST
(In reply to Chad Vizino from comment #6)
> That's a reasonable suggestion and we are considering it. What version of
> MariaDB did you upgrade from? The key for us is that we'd like to be able to
> reproduce the use case.

We noticed the issue when upgrading MariaDB from 10.1 to 10.7.

We started using MariaDB 10.1 in 2017, I think, although the initial Slurm DB may have been created with earlier versions (most likely MySQL 5.5 an Slurm 14.11).

But since 2017, we basically went like this:

MariaDB 10.1 -------------------------------------------------------------> 10.7
Slurm  17.02 --> 17.11 --> 18.08 --> 19.05 --> 20.02 --> 20.11 --> 21.08 - issue

Hope this helps!

Cheers,
--
Kilian
Comment 21 Chad Vizino 2022-03-21 18:26:18 MDT
Hi Kilian. Thanks for what you sent in comment 7--very helpful. And we are still looking into the suggestion from comment 5. An internal ticket has been opened to pursue it.

Here's a longer explanation of what's going on. There's a bit more that is suggested for you to do on your end to clean things up. I'm sure others will read this and will want to consider it also.

We've studied the condition that lead to the problem you and some others have had--it centers around newer versions (>= 10.2.1) of MariaDB that allow a text/blob field to be assigned a DEFAULT value (see link below). These versions also set an actual default ‘’ value when tables with such fields are created. However, earlier versions and all MySQL versions (true for 8.0 and assume so for earlier versions) can produce an error when the DEFAULT value is to be assigned--they also set a NULL for the default value for the field in the table when created. The tricky part is that earlier MariaDB versions and all MySQL versions allow a DEFAULT value to be set without error when strict_trans_tables sql mode is disabled for the session with the db server. On the Slurm side, it does disable strict_trans_tables when the initial connection to the db server is made (see mysql_db_get_db_connection()--introduced in Slurm 14.11--where only ANSI_QUOTES and NO_ENGINE_SUBSTITUTION are set). The versions here are important but realize this makes for eye-crossing reading!

Statements about default values for text/blob fields for MariaDB and MySQL are here:

>https://mariadb.com/kb/en/mariadb-1021-release-notes
>https://dev.mysql.com/doc/refman/8.0/en/blob.html
What this means is that upgrading to a newer MariaDB version can be problematic for sites where their Slurm tables were created with earlier MariaDB versions or (it would appear) any version of MySQL (using a dump–see below). In this case the tables (ex. assoc table) that contain fields that were created with “not null default ‘’” actually have a NULL default. When an attempt is made to set a DEFAULT value on one of the affected fields, an error is produced (what you are seeing). The workaround is to alter the default value for these fields which changes them from NULL to ‘’ (see below).

Also, earlier MariaDB versions and MySQL versions do not include the “default ‘’” clause in their mysqldump output even if tables had been created using it. However, the newer versions of MariaDB do include it. So, if a site upgrades to a newer MariaDB version and restores the Slurm database from a dump created under an older MariaDB version or any MySQL version, this will cause the same problem and the related tables will need to be altered to fix them.

Knowing all this should help us with an automated fix in the future (when newer versions of MariaDB are being used, we just need to look for text/blob fields that have table field descriptions with (Null==NO && Default==NULL) and alter them so Default==’’).

For now I have reviewed Slurm tables with text/blob fields that have "not null default ''" set when created (definitions are in src/plugins/accounting_storage/mysql/accounting_storage_mysql.c). To be safe since you are running a newer MariaDB version, it would be good to do the "alter" on these (besides the assoc table you already know about) to make them be as they were intended at create time and to avoid current/future problems where one of the text/blob fields could be set to "DEFAULT" in the Slurm code.

Looking for cases where this currently happens, they are limited to the assoc and qos tables in the code (same source file as previously mentioned) and specifically in the function remove_common(). This function is ultimately called by these: 

>as_mysql_remove_accts()
>as_mysql_remove_assocs()
>as_mysql_remove_clusters()
>as_mysql_remove_federations()
>as_mysql_remove_qos()
>as_mysql_remove_res()
>as_mysql_remove_users()
>as_mysql_remove_coord()
>as_mysql_remove_wckeys()
So removing any of the entities taken from the function name (accts, assocs, etc.) implies a potential failure similar to the one you already experienced when deleting a user (comment 0). This means that you will probably want to alter at least the qos_table fields (as you did with the assoc table) to avoid similar problems.

But again, I think it would be good to alter all the affected tables to be safe. The whole set looks like this (customized for your cluster sherlock). I've included the assoc table one for completeness even though you've already taken care of it:

>ALTER TABLE sherlock_assoc_table MODIFY `delta_qos` blob not null default '', MODIFY `grp_tres_mins` text not null default '', MODIFY `grp_tres_run_mins` text not null default '', MODIFY `grp_tres` text not null default '', MODIFY `max_tres_mins_pj` text not null default '', MODIFY `max_tres_pj` text not null default '', MODIFY `max_tres_pn` text not null default '', MODIFY `max_tres_run_mins` text not null default '', MODIFY `parent_acct` tinytext not null default '', MODIFY `partition` tinytext not null default '', MODIFY `qos` blob not null default '', MODIFY `user` tinytext not null default '';
>ALTER TABLE sherlock_event_table MODIFY `cluster_nodes` text not null default '', MODIFY `node_name` tinytext default '' not null, MODIFY `tres` text not null default '';
>ALTER TABLE sherlock_job_table MODIFY `constraints` text default '', MODIFY `gres_used` text not null default '', MODIFY `mcs_label` tinytext default '', MODIFY `tres_alloc` text not null default '', MODIFY `tres_req` text not null default '', MODIFY `wckey` tinytext not null default '', MODIFY `work_dir` text not null default '';
>ALTER TABLE sherlock_resv_table MODIFY `assoclist` text not null default '', MODIFY `node_inx` text not null default '', MODIFY `nodelist` text not null default '', MODIFY `tres` text not null default '';
>ALTER TABLE sherlock_step_table MODIFY `tres_alloc` text not null default '', MODIFY `tres_usage_in_ave` text not null default '', MODIFY `tres_usage_in_max_nodeid` text not null default '', MODIFY `tres_usage_in_max_taskid` text not null default '', MODIFY `tres_usage_in_max` text not null default '', MODIFY `tres_usage_in_min_nodeid` text not null default '', MODIFY `tres_usage_in_min_taskid` text not null default '', MODIFY `tres_usage_in_min` text not null default '', MODIFY `tres_usage_in_tot` text not null default '', MODIFY `tres_usage_out_ave` text not null default '', MODIFY `tres_usage_out_max_nodeid` text not null default '', MODIFY `tres_usage_out_max_taskid` text not null default '', MODIFY `tres_usage_out_max` text not null default '', MODIFY `tres_usage_out_min_nodeid` text not null default '', MODIFY `tres_usage_out_min_taskid` text not null default '', MODIFY `tres_usage_out_min` text not null default '', MODIFY `tres_usage_out_tot` text not null default '';
>ALTER TABLE sherlock_wckey_table MODIFY `wckey_name` tinytext not null default '';
>ALTER TABLE cluster_table MODIFY `control_host` tinytext not null default '', MODIFY `features` text not null default '';
>ALTER TABLE qos_table MODIFY `grp_tres_mins` text not null default '', MODIFY `grp_tres_run_mins` text not null default '', MODIFY `grp_tres` text not null default '', MODIFY `max_tres_mins_pj` text not null default '', MODIFY `max_tres_pa` text not null default '', MODIFY `max_tres_pj` text not null default '', MODIFY `max_tres_pn` text not null default '', MODIFY `max_tres_pu` text not null default '', MODIFY `max_tres_run_mins_pa` text not null default '', MODIFY `max_tres_run_mins_pu` text not null default '', MODIFY `min_tres_pj` text not null default '', MODIFY `preempt` text not null default '';
>ALTER TABLE tres_table MODIFY `name` tinytext not null default '';
>ALTER TABLE txn_table MODIFY `cluster` tinytext not null default '';
Finally, we plan to get this information out to affected sites who have filed similar tickets so they can do the same. Hopefully, we can have something in place in a future Slurm version to avoid this manual step.
Comment 22 Kilian Cavalotti 2022-03-21 19:18:44 MDT
Hi Chad, 

Thanks a lot for the very thorough and insightful reply, this is definitely not an easy topic and the details are very much appreciated!

I've altered our DB tables as per your recommendations.

Thanks again!

Cheers,
--
Kilian
Comment 23 Chad Vizino 2022-03-22 09:49:01 MDT
(In reply to Kilian Cavalotti from comment #22)
> Thanks a lot for the very thorough and insightful reply, this is definitely
> not an easy topic and the details are very much appreciated!
> 
> I've altered our DB tables as per your recommendations.
Great--you're welcome. I think you should be in good shape now so will close the ticket.