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 for 772 and 773 #774

Merged
merged 9 commits into from
Jan 22, 2024
Merged

Fix for 772 and 773 #774

merged 9 commits into from
Jan 22, 2024

Conversation

mbround18
Copy link
Owner

@mbround18 mbround18 commented Jan 14, 2024

Description

Contributions

Checklist

  • I added one or multiple labels which best describes this PR.
  • I have tested the changes locally.
  • This PR has a reviewer on it.
  • I have validated my changes in a docker container and on Ubuntu. (Only needed for Odin or Docker Changes)

kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 14, 2024
@mbround18 mbround18 added bug Something isn't working docker Tag if its related to docker minor Increment the minor version when merged dependencies Update one or more dependencies version rust Pull requests that update Rust code canary Use this to release a canary version for testing labels Jan 14, 2024
kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 14, 2024
@FooDeas
Copy link
Contributor

FooDeas commented Jan 16, 2024

Build logs of d243267:
log-build_odin.txt
log-build_valheim.txt

@mbround18
Copy link
Owner Author

:

Those look like successful logs, have you tried running the game server?

kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 17, 2024
@mbround18
Copy link
Owner Author

Found a new funny one, server only launches with old app id but old app id is dead. So i switch the app id right before launch. building canary now to test

@mbround18
Copy link
Owner Author

Can confirm my server shows up in server browser
image

@FooDeas
Copy link
Contributor

FooDeas commented Jan 17, 2024

I'm trying again with cd15943.

@mbround18
Copy link
Owner Author

I see server in ingame browser too with status
image

@FooDeas
Copy link
Contributor

FooDeas commented Jan 18, 2024

cd15943 starts now! But entering the console with gosu (and maybe the cronjob?) doesn't work:

steam@295714bb5ba6:~/valheim$ /usr/sbin/gosu steam /bin/bash /home/steam/scripts/auto_update.sh
error: failed switching to "steam": operation not permitted

There is another problem with the updater now:

steam@295714bb5ba6:~/valheim$ odin update
[ODIN][INFO]  - Checking for updates
thread 'main' panicked at src/odin/server/process.rs:38:10:
Failed to get process exe
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@mbround18
Copy link
Owner Author

thread 'main' panicked at src/odin/server/process.rs:38:10:

You shouldnt need gosu anymore, just exec into the container and it takes you in as steam user

@mbround18
Copy link
Owner Author

mbround18 commented Jan 18, 2024

Alright @FooDeas it was some formatting issues with the manifests, i fixed the brittleness with some regex. Arguably just as brittle but its the only option for right now. Sadly steam doesnt output json.

image

670f365 should be good to go :)

kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 18, 2024
@FooDeas
Copy link
Contributor

FooDeas commented Jan 18, 2024

You shouldnt need gosu anymore, just exec into the container and it takes you in as steam user

That's what I thought and did. But there is the cron setup in the entrypoint.sh that won't work anymore.

> "/etc/cron.d/${CRON_NAME}"
echo "" >> "/etc/cron.d/${CRON_NAME}"
| sudo tee "/etc/cron.d/${CRON_NAME}"
echo "" | sudo tee -a "/etc/cron.d/${CRON_NAME}"
Copy link
Owner Author

Choose a reason for hiding this comment

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

@FooDeas this line sets the cron script as root, the gosu should still work for the crons. ill setup some crons for tonight and let em run

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. I thought from the wrong user context.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You were right on the call out! im reworking the cron system rn

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I was confused by the "sudo tee" statements writing the cron files. Based on this, I wrongly concluded that it is now in the Steam user context. After your hint, I also would have expected that there would be no problems. 😮

kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 18, 2024
kodiakhq[bot]
kodiakhq bot previously approved these changes Jan 18, 2024
@mbround18
Copy link
Owner Author

Alrrirght @FooDeas thiissssss should work :) all features tested locally

@FooDeas
Copy link
Contributor

FooDeas commented Jan 18, 2024

Looks good! I can test it and give you feedback tomorrow, if you want.

@FooDeas
Copy link
Contributor

FooDeas commented Jan 20, 2024

It now works without any problems!

@mbround18 mbround18 merged commit 461b61e into main Jan 22, 2024
8 checks passed
@mbround18 mbround18 deleted the fix/772-and-773 branch January 22, 2024 15:57
@mbround18 mbround18 added the release Create a release when this pr is merged label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working canary Use this to release a canary version for testing dependencies Update one or more dependencies version docker Tag if its related to docker minor Increment the minor version when merged release Create a release when this pr is merged rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execing into docker fails with odin commands AppID Changed
2 participants