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

LIU-439: Use translator avahi approach for engine. #302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

myxie
Copy link
Collaborator

@myxie myxie commented Dec 27, 2024

JIRA Ticket

LIU-439

Type

  • [ ] Feature (addition)
  • Bug fix
  • [ ] Refactor (change)
  • Documentation

Problem/Issue

I have experienced unreliability with the use of dlg-engine as an address when running the docker files on Ubuntu machines. Previously (after spending a day wondering what had broken) this was chalked up to my Ubuntu 22.04 installation being a slightly broken, as @awicenec's and my personal popOS laptops were fine. However, @omaranwar85 also had issues using the host names as well when running on another Ubuntu system, so something was problematic.

Solution

It looks like sending service avahi start whilst avahi is already started causes issues. I have never had any issues with the dlg-trans address resolving on the daliuge-translator docker container, and it turns out the Dockerfile stops the avahi-daemon before starting it, which is not in the engine dockerfile. Copying the translator approach appears to resolve the resolution issues.

Also added quickstart for README.rst to help people get started ASAP, based on user feedback.

Checklist

  • [ ] Unittests added
    • Dockerfile scripts installation, not tested/able
  • Documentation added
    • I have updated the README with a quickstart guide to help people get something up and running nice and quickly, based on some of the recent user feedback.

Summary by Sourcery

Update Dockerfiles to resolve engine address resolution issues and add a quickstart guide to the README.

Bug Fixes:

  • Fixed engine address resolution issues on Ubuntu by restarting the avahi-daemon service in the Dockerfile.

Documentation:

  • Added a quickstart guide to the README to help users get started quickly.

Also added quickstart for README.rst to help people get started ASAP, based on user feedback.
Copy link
Contributor

sourcery-ai bot commented Dec 27, 2024

Reviewer's Guide by Sourcery

This pull request fixes an issue with the dlg-engine address resolution by using the translator avahi approach. It also adds a quickstart guide to the README.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Fixed engine address resolution issues.
  • Stopped and restarted the avahi-daemon in the Dockerfiles to resolve address resolution issues.
daliuge-engine/docker/Dockerfile.dev
daliuge-engine/docker/Dockerfile.devall
daliuge-engine/docker/Dockerfile.ray
Added quickstart guide to README.
  • Added a quickstart guide to help new users get started quickly.
README.rst

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @myxie - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding a basic smoke test to verify hostname resolution works correctly after container startup. This would help validate the avahi-daemon changes across different environments.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

README.rst Outdated Show resolved Hide resolved
Sourcery updates.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@@ -5,10 +5,16 @@ FROM icrar/daliuge-common:${VCS_TAG:-latest}
# RUN sudo apt-get update && sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends tzdata \
# gcc g++ gdb casacore-dev clang-tidy-10 clang-tidy libboost1.71-all-dev libgsl-dev

RUN apt install -y git python3.9-dev
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We no longer support Python 3.8.

@coveralls
Copy link

Coverage Status

coverage: 79.803%. remained the same
when pulling d8e8f48 on LIU-439
into 24cd5d5 on master.

@myxie
Copy link
Collaborator Author

myxie commented Jan 20, 2025

@awicenec may you please review these?

The component palette test failures appear to be a result of an expired secret key/token for authentication. I don't have admin access to EAGLE-graph-repo so cannot make a new token. If you're able to make me an admin for that repository as well I can update the token.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants