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

fix: use temp file to write config #174

Merged
merged 2 commits into from
Oct 8, 2023

Conversation

ryota-sakamoto
Copy link
Contributor

@ryota-sakamoto ryota-sakamoto commented Sep 28, 2023

Issue #, if available:

#172

Description of changes:

We use temp file to generate default config. Because while fs::write is writing the contents to default config path, serde_yaml::from_str return EndOfStream.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ryota-sakamoto ryota-sakamoto force-pushed the fix-generate-config branch 3 times, most recently from f15cb45 to 6ff6b8a Compare September 30, 2023 03:48
@ryota-sakamoto ryota-sakamoto changed the title fix: use temp file to generate default config fix: use temp file to write config Sep 30, 2023
@ryota-sakamoto ryota-sakamoto marked this pull request as ready for review September 30, 2023 04:13
@StoneDot StoneDot self-requested a review October 4, 2023 15:29
Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

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

Thank you for creating PR. I left nits comments. Would you take a look at them?

tests/auxiliary.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
@ryota-sakamoto ryota-sakamoto force-pushed the fix-generate-config branch 2 times, most recently from 8e14437 to 01887ac Compare October 5, 2023 07:58
@StoneDot
Copy link
Contributor

StoneDot commented Oct 5, 2023

I came up with the case where the /home directory uses a different disk. So, I tried the following. Its result indicates that we should create a temporary file in the same directory as config uses. Could you change the code to use the same directory instead of NamedTempFile?

  1. Download Ubuntu Server 22.04.3 LTS ISO image.
  2. Create a virtual machine using (A).
  3. Connect VM using virt-viewer dynein_check and follow (B).
  4. Detach media after the installation using virsh change-media dynein_check sda --eject.
  5. VM goes shutdown.
  6. Restart vm using virsh start dynein_check and connect virt-viewer dynein_check.
  7. Run test for dynein (C)

(A)

virt-install -n dynein_check -r 8192 \
  --disk path=$HOME/vm/dynein_root.img,bus=virtio,size=20 \
  --disk path=$HOME/vm/dynein_home.img,bus=virtio,size=20 \
  -c ubuntu-22.04.3-live-server-amd64.iso \
  --network network=default,model=virtio \
  --graphics vnc,listen=0.0.0.0 --noautoconsole -v --vcpus=2

(B)
Install default Ubuntu Server with custom storage layout.

  • /dev/vda
    • partition 1 new, BIOS group spacer 1.000M
    • partition 2 new, to be formatted as ext4, mounted at /
  • /dev/vdb
    • partition 1 new, to be formatted as ext4, mounted at /home

(C)

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
# Use default installation
source "$HOME/.cargo/env"
sudo apt-get update
sudo apt install git gcc pkg-config libssl-dev docker -y
sudo install -m 0755 -d /etc/apt/keyrings
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo gpg --dearmor -o /etc/apt/keyrings/docker.gpg
sudo chmod a+r /etc/apt/keyrings/docker.gpg
echo "deb [arch=amd64 signed-by=/etc/apt/keyrings/docker.gpg] https://download.docker.com/linux/ubuntu   jammy stable" | \
  sudo tee /etc/apt/sources.list.d/docker.list > /dev/null
sudo apt-get update
sudo apt-get install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin -y
git clone https://github.com/awslabs/dynein.git
cd dynein
sudo docker run -p 8000:8000 -d amazon/dynamodb-local
export RUST_BACKTRACE=1
export AWS_ACCESS_KEY_ID=test
export AWS_SECRET_ACCESS_KEY=test
cargo run ls -r local
cargo run admin create table test1 --keys pk -r local
cargo run use test -r local
# These should succeed
git remote add ryota-sakamoto https://github.com/ryota-sakamoto/dynein.git
git fetch --all
git checkout ryota-sakamoto/fix-generate-config
cargo run ls -r local
cargo run admin create table test2 --keys pk -r local
cargo run use test2 -r local

This causes the following error;

Error: IO(Os { code: 18, kind: CrossesDevices, message: "Invalid cross-device link" })

@ryota-sakamoto
Copy link
Contributor Author

@StoneDot
I have fixed the point you commented. I am using NamedTempFile::new_in instead of NamedTempFile::new to generate temporary file into dynein dir.

Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

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

Thank you for the improvement. LGTM

@StoneDot StoneDot merged commit 4ca1a7c into awslabs:main Oct 8, 2023
1 check passed
@ryota-sakamoto ryota-sakamoto deleted the fix-generate-config branch October 9, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants