Is SYS_CONFIG_PATH code correct?

[fxn]

This is perhaps more a question than an issue.

I was reading today code related to SYS_CONFIG_PATH and saw:

if [ -z "$SYS_CONFIG_PATH" ]; then
    if [ -f "$RELEASE_CONFIG_DIR/sys.config" ]; then
        export SRC_SYS_CONFIG_PATH="$RELEASE_CONFIG_DIR/sys.config"
    else
        export SRC_SYS_CONFIG_PATH="$REL_DIR/sys.config"
    fi
else
    export SRC_SYS_CONFIG_PATH="$SYS_CONFIG_PATH"
fi
if [ "$SRC_SYS_CONFIG_PATH" != "$RELEASE_MUTABLE_DIR/sys.config" ]; then
    (echo "%% Generated - edit/create $RELEASE_CONFIG_DIR/sys.config instead."; \
    cat  "$SRC_SYS_CONFIG_PATH")                              \
        > "$RELEASE_MUTABLE_DIR/sys.config"
    export DEST_SYS_CONFIG_PATH="$RELEASE_MUTABLE_DIR"/sys.config
fi
if [ ! -z "$REPLACE_OS_VARS" ]; then
    _replace_os_vars "$DEST_SYS_CONFIG_PATH"
fi
export SYS_CONFIG_PATH="${SYS_CONFIG_PATH:-$DEST_SYS_CONFIG_PATH}"

in config.sh.

If SYS_CONFIG_PATH is set by the user, a bunch of things are done, and then the last assignment is:

export SYS_CONFIG_PATH="${SYS_CONFIG_PATH:-$DEST_SYS_CONFIG_PATH}"

That leaves SYS_CONFIG_PATH with its original value. I would expect -config to get the user's explictly configured config file, so that :init.get_argument(:config) matches. In principle the assignment seems fine to me. But then, why the else clause and the rest of the code?

[bitwalker]

Yes, this is correct, we only want to set it to $DEST_SYS_CONFIG_PATH if it wasn't explicitly set by the user.

[fxn]

Let me reword the question.

Let's suppose the user has set SYS_CONFIG_PATH.

If the value is different than $RELEASE_MUTABLE_DIR/sys.config, then the file is copied to $RELEASE_MUTABLE_DIR/sys.config and variables replaced there if needed.

Isn't that work discarded then by

export SYS_CONFIG_PATH="${SYS_CONFIG_PATH:-$DEST_SYS_CONFIG_PATH}"

? The scripts later pass SYS_CONFIG_PATH as -config right?

[bitwalker]

We don't ever want to override an explicit SYS_CONFIG_PATH - yes we will regenerate RELEASE_MUTABLE_DIR/sys.config if they are different, but that is simply because we set SRC_SYS_CONFIG_PATH so that we don't have to check if it is set or not - but we aren't going to use the generated config in this case, we will just use SYS_CONFIG_PATH. The generated config is used in cases where SYS_CONFIG_PATH is not set, and we are instead inferring the sys.config path based on either RELEASE_CONFIG_DIR (if that is set) or from the sys.config included in the release itself (i.e. REL_DIR/sys.config. In which case we generate a config from the source file, and reference the generated config rather than the source.

If anything should be changed, it should probably be to change:

if [ "$SRC_SYS_CONFIG_PATH" != "$RELEASE_MUTABLE_DIR/sys.config" ]; then

to:

if [ ! -z "$SRC_SYS_CONFIG_PATH" ] && [ "$SRC_SYS_CONFIG_PATH" != "$RELEASE_MUTABLE_DIR/sys.config" ]; then

and then remove the line which sets SRC_SYS_CONFIG_PATH when SYS_CONFIG_PATH is set. I haven't done this because it isn't actually necessary, but would be a reasonable clean up task to do.

[fxn]

I believe the whole code could be simplified to this:

if [ -z "$SYS_CONFIG_PATH" ]; then
    export SYS_CONFIG_PATH="$RELEASE_MUTABLE_DIR/sys.config"
    if [ -f "$RELEASE_CONFIG_DIR/sys.config" ]; then
        export SRC_SYS_CONFIG_PATH="$RELEASE_CONFIG_DIR/sys.config"
    else
        export SRC_SYS_CONFIG_PATH="$REL_DIR/sys.config"
    fi
    (echo "%% Generated - edit/create $RELEASE_CONFIG_DIR/sys.config instead."; \
    cat "$SRC_SYS_CONFIG_PATH") > "$SYS_CONFIG_PATH"
    if [ ! -z "$REPLACE_OS_VARS" ]; then
        _replace_os_vars "$SYS_CONFIG_PATH"
    fi
fi

Because:

• You said that $SRC_SYS_CONFIG_PATH does not need to be $SYS_CONFIG_PATH when set by the user, so we can save some branch.
• If $SYS_CONFIG_PATH is set nothing is done to that file in the original code. So this alternative does nothing in a way that is obvious to the reader.
• If $SRC_SYS_CONFIG_PATH is equal to $RELEASE_MUTABLE_DIR/sys.config then in the original code $DEST_SYS_CONFIG_PATH remains unset and the call to _replace_os_vars passes a blank variable (I guess it just won't work). So, it seems equivalent to move the replacement within the if.

Perhaps I overlooked something though... what do you think?

[bitwalker]

I think there are a few things you aren't factoring in, and it is definitely not your fault, but is one of the reasons why this code is so awfully complex:

• We cannot mutate source files, they must remain immutable so that any variable replacements done are not effectively permanent. This, combined with the fact that SYS_CONFIG_PATH is considered part of the public shell API, is why SRC_* and DEST_* versions of that variable exist. Once this code has executed, SYS_CONFIG_PATH will either be set to what the user provided, or to the "actual" sys.config file passed to the VM.
• If SYS_CONFIG_PATH is set, we still generate the RELEASE_MUTABLE_DIR/sys.config from it, assuming it is different from what is already there, what you thought was us not doing anything with it was actually just avoiding mutating it.
• Your point about the missing branch leaving DEST_SYS_CONFIG_PATH unset is a good catch, luckily, it appears that the bug is effectively ignored from what I can tell, which is probably why it has never been reported. I will fix that in master though now that you've pointed it out
• Lastly, you need to account for the hot upgrade process, which involves juggling the mutable versions of the configs with the source versions in the release. Because hooks can be executed during this process, we take some care to make sure things are set up properly for them.

This aspect of the management scripts is the single most complicated part of the whole thing, the rest of it is a cakewalk in comparison, because of all the implicit behavior accounted for. I really need to comment the code more heavily than it is today. In any case, I thank you for questioning what is there, both because you found a bug, but also because it forces me to double check everything :)

[fxn]

Fantastic @bitwalker! Thank you very much for the detailed explanation ❤️.