| Message ID | 20240418102321.95384-3-xni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mdadm tests fix and enhance | expand |
On Thu, 18 Apr 2024 18:23:18 +0800 Xiao Ni <xni@redhat.com> wrote: > Now 00createnames doesn't check Create names=yes config. Without this > config, mdadm creates /dev/md127 device node when mdadm --create > /dev/md/test. With this config, it creates /dev/md_test. This patch > only adds the check. If it has this config, it returns directly > without error. Hi Xiao, Thanks for patches I will review them all later (probably next week). About this one: The proposed change is not complete as config may be read from both /etc/mdadm.conf and /etc/mdadm/mdadm.conf. Ideally, you should check them both in the approach you proposed. There is also possible to have /etc/mdadm.d/ directory - it is always checked and read if exists and it cannot be disabled. See load_conffile() and CONFFILEFLAGS in makefile for details. Test relies on the global configuration and user may forgot that it is set. That will give us positive test result because test was not run due to configuration issue. This is risky, I would prefer fail to indicate that something is wrong. User can skip this test. What about adding empty mdadm config to the command `-c ./mdadm_empty.conf`? I see it as the best option for now. That save use from checking 2 config locations and any user defined behaviors. Do you see any disadvantages? As config directory is not configurable we have to accept the risk that something could be there. Ideally, you can propose patch with confdir customization to apply same solution as for conffile (just set it to empty directory) but as it is probably rarely used we can accept risk here for now (unless somebody reported). I give it up to you as it not having confdir customization is more like new feature. Another possible solution would be to learn mdadm print it's configuration and print it before running test and fail if not compatible setting detected. I did not realize that it would be a problem, that for catching! Thanks, Mariusz
On Thu, 18 Apr 2024 18:23:18 +0800 Xiao Ni <xni@redhat.com> wrote: > + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep > MD_METADATA=1.2)" You can also limit this test to super-1.2 it may depend on config, so we can specify metadata directly in create command (if it is not specified). Mariusz
On Fri, Apr 19, 2024 at 3:20 PM Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote: > > On Thu, 18 Apr 2024 18:23:18 +0800 > Xiao Ni <xni@redhat.com> wrote: > > > Now 00createnames doesn't check Create names=yes config. Without this > > config, mdadm creates /dev/md127 device node when mdadm --create > > /dev/md/test. With this config, it creates /dev/md_test. This patch > > only adds the check. If it has this config, it returns directly > > without error. > > Hi Xiao, > Thanks for patches I will review them all later (probably next week). > > About this one: > > The proposed change is not complete as config may be read from both > /etc/mdadm.conf and /etc/mdadm/mdadm.conf. Ideally, you should check them > both in the approach you proposed. > > There is also possible to have /etc/mdadm.d/ directory - it is always checked > and read if exists and it cannot be disabled. See load_conffile() and > CONFFILEFLAGS in makefile for details. Hi Mariusz Yes, you're right. Thanks for this. > > Test relies on the global configuration and user may forgot that it is set. > That will give us positive test result because test was not run due to > configuration issue. This is risky, I would prefer fail to indicate > that something is wrong. User can skip this test. Ok, we can keep it the way it is. > > What about adding empty mdadm config to the command `-c ./mdadm_empty.conf`? I > see it as the best option for now. That save use from checking 2 config > locations and any user defined behaviors. Do you see any disadvantages? You mean specifying config file in test case when creating raid? > > As config directory is not configurable we have to accept the risk that > something could be there. > Ideally, you can propose patch with confdir customization to apply same > solution as for conffile (just set it to empty directory) but as it is probably > rarely used we can accept risk here for now (unless somebody reported). I give > it up to you as it not having confdir customization is more like new > feature. I'll remove the adding check in the case. I think few people use the config too. > > Another possible solution would be to learn mdadm print it's configuration and > print it before running test and fail if not compatible setting detected. > > I did not realize that it would be a problem, that for catching! Regards Xiao > > Thanks, > Mariusz >
On Fri, Apr 19, 2024 at 5:47 PM Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote: > > On Thu, 18 Apr 2024 18:23:18 +0800 > Xiao Ni <xni@redhat.com> wrote: > > > + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep > > MD_METADATA=1.2)" > > You can also limit this test to super-1.2 it may depend on config, so we can > specify metadata directly in create command (if it is not specified). Could you explain more? I don't catch you here. Regards Xiao > > Mariusz >
On Mon, 22 Apr 2024 14:56:17 +0800 Xiao Ni <xni@redhat.com> wrote: > > > > What about adding empty mdadm config to the command `-c > > ./mdadm_empty.conf`? I see it as the best option for now. That save use > > from checking 2 config locations and any user defined behaviors. Do you see > > any disadvantages? > > You mean specifying config file in test case when creating raid? Yes, with that default config won't be read. Mariusz
On Mon, 22 Apr 2024 14:57:16 +0800 Xiao Ni <xni@redhat.com> wrote: > On Fri, Apr 19, 2024 at 5:47 PM Mariusz Tkaczyk > <mariusz.tkaczyk@linux.intel.com> wrote: > > > > On Thu, 18 Apr 2024 18:23:18 +0800 > > Xiao Ni <xni@redhat.com> wrote: > > > > > + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep > > > MD_METADATA=1.2)" > > > > You can also limit this test to super-1.2 it may depend on config, so we can > > specify metadata directly in create command (if it is not specified). > > Could you explain more? I don't catch you here. Default metadata could be customized. At first glance, I thought that you added this because we metadata is not specified so I checked: if [[ -z "$NAME" ]]; then mdadm -CR "$DEVNAME" -l0 -n 1 $dev0 --force else mdadm -CR "$DEVNAME" --name="$NAME" --metadata=1.2 -l0 -n 1 $dev0 --force fi It seems that I forgot to add metadata specifier in first create command. If you have different metadata than 1.2 you can add --metadata=1.2 and then: > > > + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep > > > MD_METADATA=1.2)" won't be needed. Do I miss something? Thanks, Mariusz
On Mon, Apr 22, 2024 at 6:15 PM Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote: > > On Mon, 22 Apr 2024 14:57:16 +0800 > Xiao Ni <xni@redhat.com> wrote: > > > On Fri, Apr 19, 2024 at 5:47 PM Mariusz Tkaczyk > > <mariusz.tkaczyk@linux.intel.com> wrote: > > > > > > On Thu, 18 Apr 2024 18:23:18 +0800 > > > Xiao Ni <xni@redhat.com> wrote: > > > > > > > + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep > > > > MD_METADATA=1.2)" > > > > > > You can also limit this test to super-1.2 it may depend on config, so we can > > > specify metadata directly in create command (if it is not specified). > > > > Could you explain more? I don't catch you here. > > Default metadata could be customized. At first glance, I thought > that you added this because we metadata is not specified so I checked: > if [[ -z "$NAME" ]]; then > mdadm -CR "$DEVNAME" -l0 -n 1 $dev0 --force > else > mdadm -CR "$DEVNAME" --name="$NAME" --metadata=1.2 -l0 -n 1 > $dev0 --force > fi > > > It seems that I forgot to add metadata specifier in first create command. If > you have different metadata than 1.2 you can add --metadata=1.2 and then: > > > > > + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep > > > > MD_METADATA=1.2)" > > won't be needed. Do I miss something? Ah, yes, you are right. It doesn't need to check metadata. I thought there were other metadata possibilities. Thanks for pointing this problem out :) Best Regards Xiao > > Thanks, > Mariusz >
diff --git a/tests/00createnames b/tests/00createnames index a95e7d2bb085..2eb3d9331dda 100644 --- a/tests/00createnames +++ b/tests/00createnames @@ -3,6 +3,15 @@ set -x -e # Test how <devname> and --name= are handled for create mode. +create_with_name=`cat /etc/mdadm.conf | grep "^Create*.names=yes"` + +if [ -n "$create_with_name" ]; then + exit 0 +fi + +#The following cases don't consider the names=yes setting in mdadm.conf +#It needs to add the respecting cases which consider names=yes config + # The most trivial case. names_create "/dev/md/name" names_verify "/dev/md127" "name" "name" diff --git a/tests/templates/names_template b/tests/templates/names_template index 1b6cd14bf51d..4c7ff8c27f73 100644 --- a/tests/templates/names_template +++ b/tests/templates/names_template @@ -30,6 +30,7 @@ function names_verify() { local DEVNODE_NAME="$1" local WANTED_LINK="$2" local WANTED_NAME="$3" + local EXPECTED="" local RES="$(mdadm -D --export $DEVNODE_NAME | grep MD_DEVNAME)" if [[ "$?" != "0" ]]; then @@ -46,13 +47,25 @@ function names_verify() { exit 1 fi + if [ -b /dev/md/$WANTED_LINK ]; then + echo "/dev/md/$WANTED_LINK doesn't exit" + fi + local RES="$(mdadm -D --export $DEVNODE_NAME | grep MD_NAME)" if [[ "$?" != "0" ]]; then echo "Cannot get metadata from $dev0." exit 1 fi - local EXPECTED="MD_NAME=$(hostname):$WANTED_NAME" + #If the lenght of hostname is >= 32, super1 doesn't use hostname + #in metadata + local is_super1="$(mdadm -D --export $DEVNODE_NAME | grep MD_METADATA=1.2)" + hostname=$(hostname) + if [ -n "$is_super1" -a `expr length $(hostname)` -lt 32 ]; then + EXPECTED="MD_NAME=$(hostname):$WANTED_NAME" + else + EXPECTED="MD_NAME=$WANTED_NAME" + fi if [[ "$RES" != "$EXPECTED" ]]; then echo "$RES doesn't match $EXPECTED." exit 1
Now 00createnames doesn't check Create names=yes config. Without this config, mdadm creates /dev/md127 device node when mdadm --create /dev/md/test. With this config, it creates /dev/md_test. This patch only adds the check. If it has this config, it returns directly without error. And this case has an error. For super1, if the length of hostname is >= 32, it doesn't add hostname in metadata name. Fix this problem by checking the length of hostname. And this patch adds a check if device node exists. Signed-off-by: Xiao Ni <xni@redhat.com> --- tests/00createnames | 9 +++++++++ tests/templates/names_template | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-)