DAOS-9724 IO500 example improvements #15

markaolson · 2022-02-08T06:05:46Z

Remove Intel OneAPI MPI from daos-client images

Build IO500 specific daos-client images

Removed installation steps for Intel OneAPI MPI, mpifileutils, and IO500-SC21 from run_io500-sc21.sh.

Ability to create daos-server and daos-client images with different versions of DAOS.

Add multiple config files in IO500 example.

Signed-off-by: Mark A. Olson [email protected]

Remove Intel OneAPI MPI from daos-client images Build IO500 specific daos-client images Removed the installation steps for Intel OneAPI MPI, mpifileutils, and IO500-SC21 from run_io500-sc21.sh. Ability to create daos-server and daos-client images with different versions of DAOS. Add multiple config files in IO500 example. Signed-off-by: Mark A. Olson <[email protected]>

Kmannth · 2022-02-08T18:28:16Z

images/daos-client-image.json

-    },
+  "variables": {
+    "daos_version": "{{env `DAOS_VERSION`}}"
+  },


@markaolson How is the value for DAOS_VERSION set? We don't wan Users messing around with this dev feature. We want our users on Intel/GCP Validated machines images not a choose your own adventure.

The version is no longer hard coded in each of the packer templates.
The default version is now in a single file images/daos_version.sh.
That file contains the default version used when no version is specified in the -v --version option of images/build_images.sh

Kmannth · 2022-02-08T18:30:42Z

images/daos_version.sh

+
+# Default DAOS version to be installed in images
+export DEFAULT_DAOS_VERSION="2.0.1"
+


@markaolson This makes is very user accessible. Can you put this in a less easy spot to find?

Why is it a problem for the user to see which version of DAOS will be used as the default when they build their images? What other spot do you suggest?

Kmannth · 2022-02-08T18:34:03Z

images/scripts/install_daos.sh

+    OR
+
+    export DAOS_INSTALL_TYPE="server"
+    export DAOS_VERSION="2.0.0"


This is is very user accessible. We don't want users doing this. "export DAOS_VERSION="2.0.0"" Can you better hid this dev feature.

I'm not understanding the reasoning behind trying to hide how the version number is set.

Are you asking me to just remove that from the help output or remove the ability to pass the version in an environment variable?

Probably deserves an offline discussion.

@markaolson After some thought lets just leave this as an undocumented feature.

The core issue is we do not want DAOS GCP USERs playing around with DAOS Versions. This should be an Experts only type activity. As you can just see there are DAOS 2.0 Lib fabric changes getting landed. If a USE decide DAOS 1.2 or 1.0 would be fun to try bad things would happen.

Kmannth · 2022-02-08T18:35:11Z

images/scripts/setup_server.sh

@@ -0,0 +1,46 @@
+#!/bin/bash
+# Copyright 2021 Google LLC


@markaolson we work for Intel?

I didn't write this file. I ran it through shellcheck and fixed the issues that shellcheck identified. I also renamed it from setup-server.sh to setup_server.sh so that the word separator was consistent with the other bash scripts. That's why it's showing up as a new file. I will remove the copyright notice.

You can just add a 2nd intel copyright line as I understand.

Kmannth · 2022-02-08T18:37:36Z

terraform/examples/io500/build_daos_io500_images.sh

@@ -0,0 +1,371 @@
+#!/bin/bash


@markaolson I am not a big fan of this but ok.

Thanks for letting me know.

Kmannth · 2022-02-08T18:39:06Z

terraform/examples/io500/config/config.sh

@@ -67,7 +65,7 @@ export TF_VAR_server_template_name="${DAOS_SERVER_BASE_NAME}"
 export TF_VAR_server_mig_name="${DAOS_SERVER_BASE_NAME}"
 export TF_VAR_server_machine_type="${DAOS_SERVER_MACHINE_TYPE}"
 export TF_VAR_server_os_project="${TF_VAR_project_id}"
-export TF_VAR_server_os_family="daos-server-centos-7"
+export TF_VAR_server_os_family="daos-server-io500-centos-7"


@markaolson Why do we need a daos-server-io500-centos-7 image?

IO500 is a client application a special server should not be needed.

When there are multiple people working in the same project one person might be testing IO500 on DAOS 2.0.0 and another person may be testing the terraform/examples/full_cluster_setup or testing images with new versions in some other way. This actually happened with Johann and I a couple of times where my images messed up what he was doing. This is the first step in fixing the issue. When you run the IO500 the server and client images need to match. If you run the --version and pass a new version with the --force option you do not want to blow away your main daos-server-centos-7 image. So this just ensures that the images you are using are specifically built for the IO500 example and don't interfere with anything else. We still have a larger issue we need to address in a future PR. We need a way to tag/label the images with the version number so that the scripts can check for existing images with a specific version. Topic for another day.

@markaolson had a good chat today. I better understand what is going on.

Kmannth · 2022-02-08T18:41:53Z

terraform/examples/io500/config/config_1c_1s_8d.sh

@@ -0,0 +1,79 @@
+#!/bin/bash
+


I can see having 1 config per DAOS Server config.

If a user wants more clients or server they can do that at the Terraform layer.

This is is way too many scripts here.

Kmannth · 2022-02-08T18:42:31Z

terraform/examples/io500/config/config_4c_2s_16d.sh

@@ -0,0 +1,78 @@
+#!/bin/bash


Too many copies of the same basic thing.

Kmannth · 2022-02-08T18:45:10Z

terraform/examples/io500/configure_daos.sh

+  sed -i "s/^\(\s*\)targets:.*/\1targets: ${DAOS_SERVER_DISK_COUNT}/g" "${SCRIPT_DIR}/daos_server.yml"
+  sed -i "s/^\(\s*\)scm_size:.*/\1scm_size: ${DAOS_SERVER_SCM_SIZE}/g" "${SCRIPT_DIR}/daos_server.yml"
+
+  # Copy daos_server.yml to all servers


@markaolson This type of things needs to be apart of the Server code not of any IO500 Example. Please refactor this logic into Server code

I sent you an e-mail about this. You may have not seen it yet. The TODO comment at the top of the file was added because, yes, everything in this file this needs to move to terraform/startup scripts. However, it's too big of a change to include in this PR. Although this is a new file this code already existed in different files. I didn't add this feature. I just moved it to this file and called clush to deploy the .yml files instead of looping in a for loop and calling SSH for each server. I looked for code that manipulated daos_*.yml files in different scripts in the terraform/examples/io500 directory and then consolidated it into this single file. That way when we actually do move the configuration to terraform/startup scripts it should be easy to yank this out by deleting this one file and then removing the line in start.sh that calls it. I think the refactoring you are requesting should be done in another PR to focus specifically on those changes rather than make this PR larger. @cboneti and @johannlombardi do you agree?

@markaolson I agree this patch is huge. Breaking things into smaller pieces. helps.

Can we open an issue to track IO500 work somewhere like we have been talking about for few weeks? If so then I can agree to a fix in future version type way.

Kmannth · 2022-02-08T18:54:56Z

terraform/examples/io500/stop.sh

+
+log_section "Destroying DAOS Servers & Clients"
+
+pushd "${SCRIPT_DIR}/../full_cluster_setup"
 terraform destroy -auto-approve


@markaolson can you help me understand all the ACTIVE_CONFIG logic?

What is the use case?

The config/active_config.sh symlink is created by the start.sh script. It points to the config that was sourced when the start script ran terraform to provision the instances. The symlink is then used by the stop.sh script to know which config to source when it runs terraform to tear down the environment. Without the symlink you you have to manually pass the last config you used to the stop.sh script every time you run it.

stop and start should just be + ansible start + ansible destroy.

What happens if I spawn 2 different DAOS system for 2 difference clusters from the same project?

After I create the 2nd how do a I get back to the 1st?

Kmannth

The is a huge patch with many things happening in it.

On first pass it look mostly good.
A few things that stood out to me:

Exposing DAOS_VERSION to users does not seem like a good step. I understand is very helpful for DEV but having a specific config file and full docs for it too much for me. We don't want GCP DAOS users mixing and match non validated DAOS release on GCP. I 100% support having this functionally as a Exports only type thing.
There is a big IO500 refactor.
We need one example per Server configs.
Users should update the tfvars for number of clients/servers. There is a lot of redundant scripts.
No no no to more updating the server.yml from IO500 scripts. Maybe drop out the I0500 running stuff for now while it can be fixed.

Changed values of os_family vars to match the names in the packer json files. Signed-off-by: Mark A. Olson <[email protected]>

This file was supposed to show up in the git history as being renamed to setup_server.sh. Changed the word separator to an underscore to be consistent with other multi-word files in the same directory. Signed-off-by: Mark A. Olson <[email protected]>

Removed version information in help Removed invalid config_4c_2s_16d.sh config Signed-off-by: Mark A. Olson <[email protected]>

Kmannth

I agree to land this in it's current state.

lsitkiew

Code looks good to me

Mark A. Olson added 2 commits February 7, 2022 21:58

Merge branch 'master' into DAOS-9724_2

0efc6ec

markaolson requested review from lsitkiew, cboneti, johannlombardi and Kmannth February 8, 2022 06:34

Kmannth reviewed Feb 8, 2022

View reviewed changes

Kmannth suggested changes Feb 8, 2022

View reviewed changes

Mark A. Olson added 3 commits February 8, 2022 11:58

DAOS-9724 Updated os_family vars in examples

38dfc65

Changed values of os_family vars to match the names in the packer json files. Signed-off-by: Mark A. Olson <[email protected]>

DAOS-9724 Removed setup-server.sh

527bd12

This file was supposed to show up in the git history as being renamed to setup_server.sh. Changed the word separator to an underscore to be consistent with other multi-word files in the same directory. Signed-off-by: Mark A. Olson <[email protected]>

DAOS-9724 Updates after review.

a0f2faf

Removed version information in help Removed invalid config_4c_2s_16d.sh config Signed-off-by: Mark A. Olson <[email protected]>

Kmannth approved these changes Feb 9, 2022

View reviewed changes

cboneti approved these changes Feb 10, 2022

View reviewed changes

lsitkiew approved these changes Feb 10, 2022

View reviewed changes

johannlombardi merged commit 7616dad into daos-stack:master Feb 10, 2022

markaolson deleted the DAOS-9724_2 branch February 10, 2022 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-9724 IO500 example improvements #15

DAOS-9724 IO500 example improvements #15

markaolson commented Feb 8, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 9, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 8, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 9, 2022

Kmannth Feb 8, 2022

Kmannth Feb 8, 2022

Kmannth Feb 8, 2022 •

edited

Loading

markaolson Feb 8, 2022

Kmannth Feb 8, 2022

Kmannth Feb 8, 2022

markaolson Feb 8, 2022

Kmannth Feb 8, 2022

Kmannth left a comment

Kmannth left a comment

lsitkiew left a comment •

edited

Loading


		# Default DAOS version to be installed in images
		export DEFAULT_DAOS_VERSION="2.0.1"

DAOS-9724 IO500 example improvements #15

DAOS-9724 IO500 example improvements #15

Conversation

markaolson commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kmannth Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kmannth left a comment

Choose a reason for hiding this comment

Kmannth left a comment

Choose a reason for hiding this comment

lsitkiew left a comment • edited Loading

Choose a reason for hiding this comment

Kmannth Feb 8, 2022 •

edited

Loading

lsitkiew left a comment •

edited

Loading