Skip to content
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.

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

Add logic to carma script to account for env var in carma-config #2409

Merged
merged 16 commits into from
Jun 21, 2024

Conversation

MishkaMN
Copy link
Contributor

@MishkaMN MishkaMN commented Jun 14, 2024

PR Details

Description

This PR fixes the carma script to be able to run from any directory without relying on .env file to exist in the same directory.
Depends on this PR: usdot-fhwa-stol/carma-config#352

The proposed change on a high level:

  • Bake the .env file into the carma-config container. Since .env file specifies carma version now, it makes sense to include it in the config, which is intended to track carma version.
  • Use COMPOSE_ENV_FILES functionality under the hood from docker which allows the user to specify multiple .env files (NOTE: needs docker-compose 2.23.0+ version). carma script now modifies that variable as below (highest to lowest precedence):
    • 1 user defined shell COMPOSE_ENV_FILES values
    • 2 local .env file where carma is called
    • 3 .env file specified in carma config currently set
      This way, carma script always has default values from carma-config wherever it's called. Additionally, user can still use the variable as docker intended.
      NOTE: docker actually uses local .env file only as the last resort such as if no .env file is specified, so it doesn't always use. Currently I implemented such that it uses .env as 2nd to last precedence as long as it exists, as it makes sense in my head. For anyone who is interested, here is the docker env file precedence.
  • Fixed previously broken feature of using extra_args in carma start all extra_args as input to docker-compose up command. Now it works as intended where the user can input extra flags --env-file or --progress etc.

Resulting user experience:
Most user experience stays the same, but here are specific breakdowns of the usage and logics under the hood. "New Behaviors" describe different actions required by the user, whereas "Old Behaviors" describe similar user experience as old.

  • NEW Behavior: docker-compose needed to be updated to latest (or more than 2.23.0) which is now 2.27.1

  • NEW Behavior: If multiple .env files are specified, such as local .env, .env files in COMPOSE_ENV_FILES etc, all of them are loaded including .env from carma-config. If overlapping values exist, above precedence is used to overwrite.
    NOTE: if --env-file argument is used in carma start all --env-file /path/to/env, docker ignores COMPOSE_ENV_FILES. It is still same behavior as that.

  • OLD Behavior: carma config edit uses carma-base that tries to use same version as specified in its .env inside. If fails, just uses most recent carma-base. In the end, it shouldn't matter which version of carma-base is used as it is only for editing volume

  • Old behavior: carma config install <image> tries to pull down images specified in .env of <config-image>. It doesn't make sense to use any other .env here as the user is trying to download specific version in <config-image>

  • OLD Behavior + fixes: carma exec

    • 1 first tries to open carma-platform or carma-messenger using version specified inside current .env in carma-config.
    • 2 If it fails, try to use most recent carma-platform or carma-messenger images.
      NOTE: Here, previously the script tried to use usdotfhwastol/carma-base:latest instead which often doesn't exist and script failed. This was undesirable such as when custom image like local/carma-platform:latest is used, then the script used to fail.
    • 3 if that also fails, try to use most recent carma-base image.
    • 4 if that fails, return by logging error
      NOTE: notice here that carma exec is uses only .env file from carma-config to open the container, which how it worked before. Using any other .env file is overengineering and not needed. carma exec has extra argument of -i to specify specific image anyways.
  • OLD Behavior: In most cases, if the user wants to change the image name/tags (or any other variable), they can simply modify the .env file and rebuild <config-image> or carma config edit and modify the /opt/carma/vehicle/config/.env file and restart. Or even hardcoding as before in the docker-compose.yml work as well without worrying about any .env files. However, they can now use different .env files now as mentioned before, which has higher precedence than the one in <carma-config>

  • OLD Behavior: carma script is still backwards compatible to <config-image> before (and including) carma-system-4.5.0, which didn't have .env files. I accomplished it by keeping old relevant logics in __get_image_from_config (most new ones are backwards compatible anyways).
    NOTE: I suppressed some potential error messages that would have occured due to older versions as they are not errors (such as not finding .env file in <config-image> and there will be other actual error messages if there was an actual issue).

Related GitHub Issue

NA

Related Jira Key

https://usdot-carma.atlassian.net/browse/ARC-82

Motivation and Context

During 4.5.0 release, we modifed the way DOCKER_ORG and DOCKER_TAG are specified by centralizing them into a single .env file in each carma-config folder for docker-compose.yml to load. However, currently, when a carma-config image is built by our GitHub Actions CI processes, the resulting image still contains the {DOCKER_ORG} and {DOCKER_TAG} environment variable placeholders in the docker-compose.yml and docker-compose-background.yml files. This breaks all of the carma * functionality of the carma shell script.

Workaround: If .env file exists in the same directory from where carma is called, then everything works fine. However, this is not a desired functionality as we want to call carma from anywhere.

How Has This Been Tested?

locally in VM.

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I also updated carma installation guide here to use the most recent version of docker-compose instead of hardcoding it

@MishkaMN MishkaMN requested a review from JonSmet June 14, 2024 23:05
@MishkaMN MishkaMN self-assigned this Jun 14, 2024
@MishkaMN MishkaMN added the anomaly Something isn't working label Jun 14, 2024
@MishkaMN MishkaMN marked this pull request as draft June 14, 2024 23:15
@MishkaMN MishkaMN marked this pull request as ready for review June 17, 2024 15:49
@MishkaMN MishkaMN marked this pull request as draft June 17, 2024 15:56
@MishkaMN MishkaMN marked this pull request as ready for review June 17, 2024 19:44
@MishkaMN MishkaMN requested review from JonSmet and removed request for JonSmet June 17, 2024 19:45
@paulbourelly999 paulbourelly999 self-requested a review June 18, 2024 12:29
@willjohnsonk
Copy link
Contributor

@MishkaMN https://github.com/usdot-fhwa-stol/carma-platform/blob/623f636dd6b2ccbacc6b9cd97546de9f1f92d7e9/engineering_tools/carma
OBE'd but updated carma script for dual compute. Should be mostly isolated from dual compute functionality

@MishkaMN MishkaMN requested review from adev4a June 18, 2024 15:14
@MishkaMN
Copy link
Contributor Author

Discussed offline with @paulbourelly999 and @adev4a to include an option for the user to specify the env file directory for the docker-compose

engineering_tools/carma Show resolved Hide resolved
engineering_tools/carma Show resolved Hide resolved
engineering_tools/carma Show resolved Hide resolved
engineering_tools/carma Outdated Show resolved Hide resolved
engineering_tools/carma Outdated Show resolved Hide resolved
@MishkaMN MishkaMN changed the title Simple Method: add logic to carma script to account for env var in carma-config Add logic to carma script to account for env var in carma-config Jun 19, 2024
engineering_tools/carma Outdated Show resolved Hide resolved
engineering_tools/carma Show resolved Hide resolved
engineering_tools/carma Outdated Show resolved Hide resolved
engineering_tools/carma Outdated Show resolved Hide resolved
engineering_tools/carma Show resolved Hide resolved
@MishkaMN MishkaMN requested a review from adev4a June 20, 2024 16:59
Copy link

sonarcloud bot commented Jun 20, 2024

@MishkaMN MishkaMN merged commit c17e906 into develop Jun 21, 2024
4 checks passed
@MishkaMN MishkaMN deleted the arc-82-carma-script-docker-compose-env-var-2 branch June 21, 2024 15:54
MishkaMN added a commit to usdot-fhwa-stol/carma-config that referenced this pull request Jun 21, 2024
<!-- Thanks for the contribution, this is awesome. -->

# PR Details
## Description
Supports usdot-fhwa-stol/carma-platform#2409
This change copies the hidden `.env` file specifically to the
carma-config image.
It also adds `cdasim_config` folder to necessary `.gitignore` files such
that the respective images can be built without any permission issue
common with cdasim_config folders.

## Related GitHub Issue
NA
<!--- This project only accepts pull requests related to open GitHub
issues or Jira Keys -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->
<!--- Please DO NOT name partially fixed issues, instead open an issue
specific to this fix -->
<!--- Please link to the issue here: -->

## Related Jira Key
https://usdot-carma.atlassian.net/browse/ARC-82
<!-- e.g. CAR-123 -->

## Motivation and Context
During 4.5.0 release, we modifed the way DOCKER_ORG and DOCKER_TAG are
specified by centralizing them into a single .env file in each
carma-config folder for docker-compose.yml to load. However, currnently,
when a carma-config image is built by our GitHub Actions CI processes,
the resulting image still contains the {DOCKER_ORG} and {DOCKER_TAG}
environment variable placeholders in the docker-compose.yml and
docker-compose-background.yml files. This breaks the carma config
install <carma-config image> functionality of the carma shell script.
The carma start all functionality breaks with this as well.

Workaround: If .env file exists in the same directory from where `carma`
is called, then everything works fine. However, this is not a desired
functionality as we want to call carma from anywhere

<!--- Why is this change required? What problem does it solve? -->

## How Has This Been Tested?
locally in VM. Results of each commands working on the main PR and its
comments

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [X] Defect fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that cause existing functionality
to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [X] I have added any new packages to the sonar-scanner.properties file
- [ ] My change requires a change to the documentation.
- [X] I have updated the documentation accordingly.
- [X] I have read the
[**CONTRIBUTING**](https://github.com/usdot-fhwa-stol/carma-platform/blob/develop/Contributing.md)
document.
- [X] I have added tests to cover my changes.
- [X] All new and existing tests passed.
@MishkaMN MishkaMN mentioned this pull request Jul 9, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants