diff mbox series

[2/5] tests/00createnames enhance

Message ID 20240418102321.95384-3-xni@redhat.com (mailing list archive)
State Changes Requested
Headers show
Series mdadm tests fix and enhance | expand

Commit Message

Xiao Ni April 18, 2024, 10:23 a.m. UTC
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(-)

Comments

Mariusz Tkaczyk April 19, 2024, 7:20 a.m. UTC | #1
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
Mariusz Tkaczyk April 19, 2024, 9:47 a.m. UTC | #2
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
Xiao Ni April 22, 2024, 6:56 a.m. UTC | #3
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
>
Xiao Ni April 22, 2024, 6:57 a.m. UTC | #4
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
>
Mariusz Tkaczyk April 22, 2024, 7:23 a.m. UTC | #5
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
Mariusz Tkaczyk April 22, 2024, 10:15 a.m. UTC | #6
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
Xiao Ni April 22, 2024, 1:31 p.m. UTC | #7
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 mbox series

Patch

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