Skip to content

Copy situational configuration using a temporary file and mv to improve concurrency #3359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 25, 2022
Merged

Copy situational configuration using a temporary file and mv to improve concurrency #3359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9414e03
Use mv while updating situational configuration files to improve conc…
rjeberhard Aug 25, 2022
0dec098
Fix script
rjeberhard Aug 25, 2022
69018ec
Fix script
rjeberhard Aug 25, 2022
ee3620b
Add explanatory comment
rjeberhard Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions operator/src/main/resources/scripts/livenessProbe.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/bin/bash

# Copyright (c) 2017, 2021, Oracle and/or its affiliates.
# Copyright (c) 2017, 2022, Oracle and/or its affiliates.
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl.

# Kubernetes periodically calls this liveness probe script to determine whether
Expand Down Expand Up @@ -35,9 +35,9 @@ if [ ! "${DYNAMIC_CONFIG_OVERRIDE:-notset}" = notset ]; then
tgt_file=$tgt_dir/$tgt_file # add back in tgt dir path
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment:

The 'copyIfChanged' looks like it already contains the diff/continue logic on lines 36 & the chmod logic on lines 39,40, and 41. I think you can you go ahead and delete these lines.

On the other hand, copyIfChanged is a little talkative - it traces even when it decides to skip the copy. This might be a little distracting since the liveness probe is frequently invoked.

[ -f "$tgt_file" ] && [ -z "$(diff $local_fname $tgt_file 2>&1)" ] && continue # nothing changed
trace "Copying file '$local_fname' to '$tgt_file'."
cp $local_fname $tgt_file # TBD ignore any error?
copyIfChanged $local_fname $tgt_file
Copy link

@tbarnes-us tbarnes-us Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that "copyIfChanged" exits with SEVERE if the copy or chmod fails. My assumption is that's the original reason why the function wasn't originally used here. Is this something we want to happen in the livenessprobe? IIRC, its copy was supposed to be more of a 'best effort'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm okay with the liveness probe failing and the server pod restarting when this copy fails. The other choice is that the server continues running with the old configuration for an unknown amount of time.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me.

if [ -O "$tgt_file" ]; then
chmod 770 $tgt_file # TBD ignore any error?
chmod 770 $tgt_file
fi
done
for local_fname in ${tgt_dir}/*.xml ; do
Expand Down
21 changes: 1 addition & 20 deletions operator/src/main/resources/scripts/startServer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/bin/bash

# Copyright (c) 2017, 2021, Oracle and/or its affiliates.
# Copyright (c) 2017, 2022, Oracle and/or its affiliates.
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl.

#
Expand Down Expand Up @@ -29,25 +29,6 @@ fi
exportInstallHomes

#
# Define helper fn to copy a file only if src & tgt differ
#

function copyIfChanged() {
[ ! -f "${1?}" ] && trace SEVERE "File '$1' not found." && exit 1
if [ ! -f "${2?}" ] || [ ! -z "`diff $1 $2 2>&1`" ]; then
trace "Copying '$1' to '$2'."
cp $1 $2
[ $? -ne 0 ] && trace SEVERE "failed cp $1 $2" && exitOrLoop
if [ -O "$2" ]; then
chmod 770 $2
[ $? -ne 0 ] && trace SEVERE "failed chmod 770 $2" && exitOrLoop
fi
else
trace "Skipping copy of '$1' to '$2' -- these files already match."
fi
}

#
# if the auxiliary image feature is active, verify the mount, and log mount information
#
checkAuxiliaryImage || exitOrLoop
Expand Down
27 changes: 26 additions & 1 deletion operator/src/main/resources/scripts/utils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2017, 2021, Oracle and/or its affiliates.
# Copyright (c) 2017, 2022, Oracle and/or its affiliates.
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl.

set -o pipefail
Expand All @@ -17,6 +17,31 @@ source ${SCRIPTPATH}/utils_base.sh
# [ $? -ne 0 ] && echo "[SEVERE] Missing file ${SCRIPTPATH}/utils.sh" && exit 1
#

#
# Define helper fn to copy a file only if src & tgt differ
#

function copyIfChanged() {
[ ! -f "${1?}" ] && trace SEVERE "File '$1' not found." && exit 1
if [ ! -f "${2?}" ] || [ ! -z "`diff $1 $2 2>&1`" ]; then
trace "Copying '$1' to '$2'."
# Copy the source file to a temporary file in the same directory as the target and then
# move the temporary file to the target. This is done because the target file may be read by another
# process during the copy operation, such as can happen for situational configuration files stored in a
# domain home on a persistent volume.
tmp_file=$(mktemp -p $(dirname $2))
cp $1 $tmp_file
mv $tmp_file $2
Copy link

@tbarnes-us tbarnes-us Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment:

Copying to a tmp file and then moving the file is a little subtle. Perhaps add a comment about why we're doing this?

[ $? -ne 0 ] && trace SEVERE "failed cp $1 $2" && exitOrLoop
if [ -O "$2" ]; then
chmod 770 $2
[ $? -ne 0 ] && trace SEVERE "failed chmod 770 $2" && exitOrLoop
fi
else
trace "Skipping copy of '$1' to '$2' -- these files already match."
fi
}

# exportInstallHomes
# purpose: export MW_HOME, WL_HOME, ORACLE_HOME
# with defaults as needed
Expand Down